Re: [Libvir] [PATCH] Implement virDomainBlockStats for QEMU/KVM
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/