[libvirt] [PATCH/GSoC] Use virGetLastErrorMessage() rather than open code it

[Cole Robinson]

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