Re: [PATCH] use virGetLastErrorMessage instead of virGetLastError to check for NULL in qemu

[Cole Robinson <crobinso@xxxxxxxxxx>]

Thanks for the patch. Another commit message tip: keep all lines under 80
characters long, and even try to keep the first line under 60 so it's more
readable for various mail clients. For this patch I'd use a commit message like

  qemu: Don't duplicate virGetLastErrorMessage

  These uses of virGetLastError message are just duplicating
  virGetLastErrorMessage.

(not perfect... I'm not great at commit messages myself)

More comments below

On 03/17/2016 03:01 PM, Jovanka Gulicoska wrote:
> ---
>  src/qemu/qemu_capabilities.c | 18 ++++++++----------
>  src/qemu/qemu_driver.c       | 12 +++++-------
>  2 files changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index b223837..af35f89 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3089,10 +3089,9 @@ virQEMUCapsInitCached(virQEMUCapsPtr qemuCaps, const char *cacheDir)
>  
>      if (virQEMUCapsLoadCache(qemuCaps, capsfile, &qemuctime, &selfctime,
>                               &selfvers) < 0) {
> -        virErrorPtr err = virGetLastError();
> +        const char *err = virGetLastErrorMessage();
>          VIR_WARN("Failed to load cached caps from '%s' for '%s': %s",
> -                 capsfile, qemuCaps->binary, err ? NULLSTR(err->message) :
> -                 _("unknown error"));
> +                 capsfile, qemuCaps->binary, err);
>          virResetLastError();
>          ret = 0;
>          virQEMUCapsReset(qemuCaps);

These usages can be simplified even more. For example on top of this patch:

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index af35f89..7f91730 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3089,9 +3089,8 @@ virQEMUCapsInitCached(virQEMUCapsPtr qemuCaps, const
char *cacheDir)

     if (virQEMUCapsLoadCache(qemuCaps, capsfile, &qemuctime, &selfctime,
                              &selfvers) < 0) {
-        const char *err = virGetLastErrorMessage();
         VIR_WARN("Failed to load cached caps from '%s' for '%s': %s",
-                 capsfile, qemuCaps->binary, err);
+                 capsfile, qemuCaps->binary, virGetLastErrorMessage());
         virResetLastError();
         ret = 0;
         virQEMUCapsReset(qemuCaps);


So please use that pattern instead and resubmit.

Thanks,
Cole

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list