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

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!