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

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110303/af4d1067/attachment-0001.sig>