[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: fix text block info parsing



On Wed, Oct 12, 2011 at 06:30:08PM -0600, Eric Blake wrote:
> Detected by Coverity.  p (the pointer to the string) is always true;
> when in reality, we wanted to know whether the integer value of the
> just-parsed string is '0' or '1'.  Logic bug since commit b1b5b51.
> 
> * src/qemu/qemu_monitor_text.c (qemuMonitorTextGetBlockInfo): Set
> results to proper value.
> ---
> 
> Second bug introduced by the same commit.  And I think I reviewed
> it originally.  Sorry for not catching it earlier.

  Absolutely no need to be sorry, 1/ you're the one ACK'ing the most
patches 2/ I think you filter out bugs way better than me for example
statistics will play against you, but nobody will complain !

>  src/qemu/qemu_monitor_text.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> index 1eb9846..49c469b 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -817,19 +817,19 @@ int qemuMonitorTextGetBlockInfo(qemuMonitorPtr mon,
>                      if (virStrToLong_i(p, &dummy, 10, &tmp) == -1)
>                          VIR_DEBUG("error reading removable: %s", p);
>                      else
> -                        info->removable = p != NULL;
> +                        info->removable = tmp != 0;

  I would be tempted to put braces 
                           info->removable = (tmp != 0);
to make it obvious that the second part is computation, not cascaded
affectations 

>                  } else if (STRPREFIX(p, "locked=")) {
>                      p += strlen("locked=");
>                      if (virStrToLong_i(p, &dummy, 10, &tmp) == -1)
>                          VIR_DEBUG("error reading locked: %s", p);
>                      else
> -                        info->locked = p ? true : false;
> +                        info->locked = tmp != 0;
>                  } else if (STRPREFIX(p, "tray_open=")) {
>                      p += strlen("tray_open=");
>                      if (virStrToLong_i(p, &dummy, 10, &tmp) == -1)
>                          VIR_DEBUG("error reading tray_open: %s", p);
>                      else
> -                        info->tray_open = p ? true : false;
> +                        info->tray_open = tmp != 0;
>                  } else {
>                      /* ignore because we don't parse all options */
>                  }

  But it's cosmetic, ACK

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]