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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list