8:32 a.m.
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