hpc / hpc/mpifileutils

daos: dcp: always cleanup all resources

Open
#379 3 comments 0 reactions 1 assignee View on GitHub

@daltonbohning is already working on this.

Since Sep 19, 2020.

daos
Dominant language
C
Stars
200
Forks
85
Avg merge
3d 21h
Merged PRs (30d)
2

Description

Probably a good idea to do this along with #391, since the code paths will likely need to be retested again anyway.

dcp has some early exits for various errors, invalid arguments, etc.
Some of these are DAOS-related, and some are DAOS-agnostic.
In either case, it would be good practice to always free all memory and resource that have been allocated.
Essentially, anywhere dcp.main() has a return, it would be a good idea to try to cleanup any resources that were in use at the time. Some of these may be harmless, such as strings not being deallocated properly, which the OS might cleanup automatically upon exiting main(), but for the DAOS dfs mounts in particular, there could be other resources in use at the time.

For DAOS-agnostic code
This means paths, file list, copy options, etc. The tricky part is to avoid necessarily "copy-pasting" all of the cleanup, since there are currently ~8 mfu_free and *_delete lines at the end of dcp.c, plus the mfu_finalize and MPI_Finalize, and not all of the variables are in scope at every point of return. A possible solution could be to declare all of these variables at the top of main so they are always in scope. Then, we could either have a function that deletes all of these, or use a goto.

For DAOS-specific code
This means anything in daos_cleanup. The tricky part here is that we might not know which parts need to be cleaned up at any given time. For example, if we fail to connect to the source pool, we will print an error and exit. But if we simply call daos_cleanup here, it might also try to unmount the destination dfs, disconnect from the destination pool, and close the destination container. But if we couldn't connect to the source pool, then all three of these things might fail, which would result in (1) extra time spent trying to free something that was never allocated (network calls) and (2) extraneous error messages for each failure, which might clutter the error output. Unfortunately, not all of these things can simply use a NULL check. For example, daos_handle_t dst_coh is never NULL, as the corresponding daos_* function expects it to not be NULL.
Possible solutions for this sort of thing could be:
(1) If, for example, we fail to connect to the source pool, then set the source pool handle to NULL. Then daos_cleanup can perform a NULL check. The drawback here is that we would have to be careful with when/how we set these things to NULL, and it should be done in a clean way.
(2) We could store a bool for each variable/structure that has been successfully allocated, assuming a failed daos_* call means it is NOT allocated, and a successful daos_* call means it WAS allocated. Then, we could check these bools in daos_cleanup.

I am more partial to (1), since it would potentially be a cleaner, more standard way to keep track of what is and is not allocated.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.