Re: [libvirt] [PATCH] util: correct retry path in virFileOperation

On 03/02/2011 06:17 PM, Laine Stump wrote: > On 03/02/2011 06:30 PM, Eric Blake wrote: >> In virFileOperation, the parent does a fallback to a non-fork >> attempt if it detects that the child returned EACCES. However, >> the child was calling _exit(-EACCESS), which does _not_ appear >> as EACCES in the parent. >> >> * src/util/util.c (virFileOperation): Correctly pass EACCES from >> child to parent. >> --- >> src/util/util.c | 9 +++++++++ >> 1 files changed, 9 insertions(+), 0 deletions(-) >> >> diff --git a/src/util/util.c b/src/util/util.c >> index bac71c8..0fe1c41 100644 >> --- a/src/util/util.c >> +++ b/src/util/util.c >> @@ -1559,6 +1559,15 @@ parenterror: >> goto childerror; >> } >> childerror: >> + /* Hook sets ret to -errno on failure, but exit must be positive. >> + * If we exit with EACCES, then parent tries again. */ >> + /* XXX This makes assumptions about errno being< 255, which is >> + * not true on Hurd. */ >> + ret = -ret; > > Maybe just a matter of taste, but I think I would prefer if everywhere > in virFileOperation set ret = errno (instead of -errno), and when hook > is called, do "ret = - hook(...)". Then you don't need the extra "ret = > -ret". Well, I thought about that, even before your comment. But considering that parent does 'return ret', and was tracking ret = -errno everywhere, I thought that tracking -errno in the parent and +errno in the child half of the same function looked even weirder than just inverting errno at the last second before _exit in the child. > > ACK either way, though. I decided to keep it as-is, and pushed. > >> + if ((ret& 0xff) != ret) { >> + VIR_WARN("unable to pass desired return value %d", ret); Technically, calling VIR_WARN in a fork increases the chance of a deadlock (if the virFork() was called while some other thread held the malloc lock); but this is no worse than the fact that virFork is already unsafe in the same manner; not to mention that this warning should never trigger on Linux, where errno should always be in _exit() range. That is, at some future point, we'll need to audit all uses of virFork for async-signal safety, not just this point.