[libvirt] [PATCH] util: correct retry path in virFileOperation
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; + if ((ret & 0xff) != ret) { + VIR_WARN("unable to pass desired return value %d", ret); + ret = 0xff; + } _exit(ret); } -- 1.7.4
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". ACK either way, though.
+ if ((ret& 0xff) != ret) { + VIR_WARN("unable to pass desired return value %d", ret); + ret = 0xff; + } _exit(ret);
}
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. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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 :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump