[libvirt] [PATCH] Don't overwrite virRun error messages

[Cole Robinson]

On 03/08/2011 10:27 PM, Eric Blake wrote:
> On 03/08/2011 11:50 AM, Cole Robinson wrote:
>> virRun gives pretty useful error output, let's not overwrite it unless there
>> is a good reason. Some places were providing more information about what
>> the commands were _attempting_ to do, however that's usually less useful from
>> a debugging POV than what actually happened.
>> ---
>>  src/lxc/veth.c                        |   45 +++------------------------------
>>  src/openvz/openvz_driver.c            |   20 --------------
>>  src/qemu/qemu_driver.c                |    4 ---
>>  src/storage/storage_backend.c         |    3 --
>>  src/storage/storage_backend_logical.c |    3 --
>>  src/vmware/vmware_driver.c            |    2 -
>>  6 files changed, 4 insertions(+), 73 deletions(-)
> 
> Hmm, I wonder if we should be using virCommand instead of virRun.  But
> that's a question for a separate patch.  Meanwhile, I agree with your
> argument that virRun's diagnosis is usually better.
> 
>> @@ -122,13 +121,7 @@ int vethCreate(char** veth1, char** veth2)
>>      argv[8] = *veth2;
>>  
>>      VIR_DEBUG("veth1: %s veth2: %s", *veth1, *veth2);
>> -    rc = virRun(argv, &cmdResult);
>> -
>> -    if (rc != 0 ||
>> -        (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
>> -        vethError(VIR_ERR_INTERNAL_ERROR,
>> -                  _("Failed to create veth device pair '%s', '%s': %d"),
>> -                  *veth1, *veth2, WEXITSTATUS(cmdResult));
>> +    if (virRun(argv, NULL) < 0) {
> 
> Not to mention that it fixes real bugs! Here, in the old code, rc can be
> 0 (we successfully waited on the child), while WIFEXITED(cmdResult) is
> false (such as if the child command had a bug, and died due to a signal
> such as SIGSEGV [that never happens, right?]) - but in that situation we
> didn't report vethError or take the cleanup path.  In the new code,
> virRun() refuses to return 0 if the child does not exit normally, so we
> always diagnose failure, whether due to non-zero status or to a signal.
> 
> ACK!
> 

Pushed now.

Thanks,
Cole