Re: [PATCH] Implement virDomainBlockStats for QEMU/KVM

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

 



On Tue, Feb 26, 2008 at 11:32:20AM +0000, Richard W.M. Jones wrote:
> 
> This little patch just implements the virDomainBlockStats call for
> qemu & kvm.  It does this by using the new 'info blockstats' monitor
> command which I added to qemu & KVM upstream some months back, and
> (hopefully) it does the right thing if this command is not available.

  sounds good, just a few comments

> +/* This uses the 'info blockstats' monitor command which was
> + * integrated into both qemu & kvm in late 2007.  If the command is
> + * not supported we detect this and return the appropriate error.
> + */
> +static int
> +qemudDomainBlockStats (virDomainPtr dom,
[...]
> +    if (STREQLEN (path, "hd", 2) && path[2] >= 'a' && path[2] <= 'z')

  Please fully parenthesize binary expressions relying just on
the priority of operators is more risky and IMHO harder to check/understand

> +        snprintf (qemu_dev_name, sizeof (qemu_dev_name),
> +                  "ide0-hd%d", path[2] - 'a');
> +    else if (STREQ (path, "cdrom"))
> +        strcpy (qemu_dev_name, "ide1-cd0");

  safe but strcpy turns my paranoid mode on :-)

[...]
> +    while (*p) {
> +        if (STREQLEN (p, qemu_dev_name, len)
> +            && p[len] == ':' && p[len+1] == ' ') {

              (STREQLEN (p, qemu_dev_name, len) &&
	       (p[len] == ':') && (p[len+1] == ' '))

   looks so much more readable to me


> +            eol = strchr (p, '\n'); if (!eol) eol = p + strlen (p);

  one statement per line, please, plus going to the line, did you changed
editors recently :-) ?

   +1

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard@xxxxxxxxxx  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]