On 03/25/2016 05:05 AM, Hui Yiqun wrote:
getting err using virGetLastError() and then retrieving message from err make developer have to test the value of err and err->message and default to self-defined unkown error message.
It's better to avoid it and using uniform virGetLastErrorMessage
Thanks for the patch! There's several compiler warnings after this though, which by default are errors for libvirt. They are of the form: error: unused variable 'err' [-Werror=unused-variable] . I won't tell you where :) Check the patch for any places that the 'virErrorPtr err' definition wasn't deleted, and if there's no other uses in the function, delete the definition. Make sure you aren't passing --disable-werror to ./configure (--enable-werror is the default) Also make sure to run 'make syntax-check', it will flag a couple style issues you need to fix. A few more comments below
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 3d38a46..e526e55 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1273,12 +1273,8 @@ int main(int argc, char **argv) { /* Read the config file if it exists*/ if (remote_config_file && daemonConfigLoadFile(config, remote_config_file, implicit_conf) < 0) { - virErrorPtr err = virGetLastError(); - if (err && err->message) - VIR_ERROR(_("Can't load config file: %s: %s"), - err->message, remote_config_file); - else - VIR_ERROR(_("Can't load config file: %s"), remote_config_file); + VIR_ERROR(_("Can't load config file: %s: %s"), + virGetLastErrorMessage(), remote_config_file); exit(EXIT_FAILURE); }
This and (and the other places like it) changes the error message slightly, but it's a good change. Just wanted to point it out
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 348bbfb..4daba3a 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -2290,12 +2290,8 @@ static int lxcContainerChild(void *data)
if (ret != 0) { VIR_DEBUG("Tearing down container"); - virErrorPtr err = virGetLastError(); - if (err && err->message) - fprintf(stderr, "%s\n", err->message); - else - fprintf(stderr, "%s\n", - _("Unknown failure in libvirt_lxc startup")); + fprintf(stderr, "Failure in libvirt_lxc startup%s\n", + virGetLastErrorMessage()); }
virCommandFree(cmd);
You dropped _() which ensures the string is translated. And you need a separator between virGetLastErrorMessage() and the root string: _("Unknown failure in libvirt_lxc startup: %s\n")
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 8b5ec4c..b20a46f 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -2731,12 +2731,7 @@ int main(int argc, char *argv[])
cleanup: if (rc < 0) { - virErrorPtr err = virGetLastError(); - if (err && err->message) - fprintf(stderr, "%s\n", err->message); - else - fprintf(stderr, "%s\n", - _("Unknown failure in libvirt_lxc startup")); + fprintf(stderr, "Failure in libvirt_lxc startup: %s\n", virGetLastErrorMessage()); }
Similarly this dropped the _() usage, please re-add it
diff --git a/src/util/iohelper.c b/src/util/iohelper.c index 8a3c377..9fe0f81 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -310,12 +310,6 @@ main(int argc, char **argv) return 0;
error: - err = virGetLastError(); - if (err) { - fprintf(stderr, "%s: %s\n", program_name, err->message); - } else { - fprintf(stderr, _("%s: unknown failure with %s\n"), - program_name, path); - } + fprintf(stderr, "%s: %s\n", program_name, virGetLastErrorMessage()); exit(EXIT_FAILURE); }
This one is a bit unclear but I'd suggest _("%s: failure with %s: %s"), program_name, path, virGetLastErrorMessage()
diff --git a/src/util/virpci.c b/src/util/virpci.c index f7921f8..2349d7f 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -989,12 +989,10 @@ virPCIDeviceReset(virPCIDevicePtr dev, ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs);
if (ret < 0) { - virErrorPtr err = virGetLastError(); virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to reset PCI device %s: %s"), dev->name, - err ? err->message : - _("no FLR, PM reset or bus reset available")); + virGetLastErrorMessage()); }
Hmm this case is kind of legitimate. The pattern is a bit weird though, maybe it makes more sense to clean up the code above it. I suggest dropping this change from the patch since it may deserve a bit more work. Thanks, Cole