[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Thu, Mar 03, 2011 at 08:14:06AM -0700, Eric Blake wrote:
> 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.

In the volume streams patch series I introduce a properly
fork+exec()ble helper program for I/O:

  http://www.redhat.com/archives/libvir-list/2011-February/msg00968.html

It'd be desirable to try and get something like this to handle
the virFileOperation use cases too, so we avoid this entire
issue of async signal safety.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]