Re: [re-send][PATCH 2/3] use WIFEXITED macro to see exit status of child

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

 



On 10/08/2012 08:51 PM, li guang wrote:
> 在 2012-10-08一的 20:05 -0600,Eric Blake写道:
>> On 10/08/2012 07:51 PM, liguang wrote:
>>> this usage was suggested by man-page of waitpid,
>>> returns  true  if  the  child terminated normally
>>
>> NACK.  virCommandRun already did this for you.
> 
> right, but not exactly, 
> virCommandRun will leave raw exit-status out of there,

Ah, after re-reading the code, so it does (I had thought we changed it
to guarantee a return of -1 on any !WIFEXITED() exit, and a sanitized
WEXITSTATUS() value, rather than making all the callers do that, but I
guess not).

> so if the waited-command exit with a code '1',
> then the caller of virCommandRun will see exit-status
> value 0x100, and report an odd '256' exit-status. 

That depends on your OS.  There are some systems out there where normal
exit is in the low-order rather than high-order byte.  But that's why we
have virProcessTranslateStatus(), to do the work correctly.

>>>      ret = virCommandRun(cmd, &exitstatus);
>>> -    if (ret == 0 && exitstatus != 0) {
>>> +    if (ret == 0 && WIFEXITED(exitstatus) == 0) {

You have a logic bug - the old code was checking for non-zero status,
and you switched it to check for zero status.

>>>          virReportError(VIR_ERR_HOOK_SCRIPT_FAILED,
>>>                         _("Hook script %s %s failed with error code %d"),
>>>                         path, drvstr, exitstatus);

This is not a correct error message - if you ever use WIFEXITED, you
must also use WEXITSTATUS, otherwise you have a mismatch.  And for this
particular error message, I think we are losing useful information; I
argue that we'd probably get a much better error message if we changed
this to let virCommandRun do ALL the work:

ret = virCommandRun(cmd, NULL);

-- 
Eric Blake   eblake@xxxxxxxxxx    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]