[Libvir] [PATCH] Implement virDomainBlockStats for QEMU/KVM
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. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
"Richard W.M. Jones" <rjones@redhat.com> 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.
Looks good. +1 ...
+ struct qemud_driver *driver = + (struct qemud_driver *)dom->conn->privateData; + char *info, *p, *dummy, *eol; + char qemu_dev_name[32]; + int len; + struct qemud_vm *vm = qemudFindVMByID(driver, dom->id);
A couple suggestions: change type of len to "size_t", since it holds strlen value, and make a few pointers "const". Doing that exposed a couple of const-incorrect interfaces, so this fixes those, too. Plus, add a few "%s" before format-string-without-% to avoid warnings when building with --disable-nls. --- src/qemu_conf.h | 4 ++-- src/qemu_driver.c | 21 +++++++++++---------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 9f09ec1..735da48 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -318,13 +318,13 @@ struct qemud_driver { static inline int -qemudIsActiveVM(struct qemud_vm *vm) +qemudIsActiveVM(const struct qemud_vm *vm) { return vm->id != -1; } static inline int -qemudIsActiveNetwork(struct qemud_network *network) +qemudIsActiveNetwork(const struct qemud_network *network) { return network->active; } diff --git a/src/qemu_driver.c b/src/qemu_driver.c index f97ef18..d9a7aca 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1315,8 +1315,9 @@ static void qemudDispatchVMEvent(int fd, int events, void *opaque) { qemudDispatchVMFailure(driver, vm, fd); } -static int qemudMonitorCommand(struct qemud_driver *driver ATTRIBUTE_UNUSED, - struct qemud_vm *vm, +static int qemudMonitorCommand(const struct qemud_driver *driver + ATTRIBUTE_UNUSED, + const struct qemud_vm *vm, const char *cmd, char **reply) { int size = 0; @@ -2520,12 +2521,12 @@ qemudDomainBlockStats (virDomainPtr dom, const char *path, struct _virDomainBlockStats *stats) { - struct qemud_driver *driver = - (struct qemud_driver *)dom->conn->privateData; - char *info, *p, *dummy, *eol; + const struct qemud_driver *driver = (void *)dom->conn->privateData; + char *dummy, *info; + const char *p, *eol; char qemu_dev_name[32]; - int len; - struct qemud_vm *vm = qemudFindVMByID(driver, dom->id); + size_t len; + const struct qemud_vm *vm = qemudFindVMByID(driver, dom->id); if (!vm) { qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, @@ -2534,7 +2535,7 @@ qemudDomainBlockStats (virDomainPtr dom, } if (!qemudIsActiveVM (vm)) { qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("domain is not running")); + "%s", _("domain is not running")); return -1; } @@ -2564,7 +2565,7 @@ qemudDomainBlockStats (virDomainPtr dom, if (qemudMonitorCommand (driver, vm, "info blockstats", &info) < 0) { qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("'info blockstats' command failed")); + "%s", _("'info blockstats' command failed")); return -1; } @@ -2578,7 +2579,7 @@ qemudDomainBlockStats (virDomainPtr dom, if (STREQLEN (info, "info ", 5)) { free (info); qemudReportError (dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, - _("'info blockstats' not supported by this qemu")); + "%s", _("'info blockstats' not supported by this qemu")); return -1; } -- 1.5.4.3.221.gf57a
On Tue, Feb 26, 2008 at 03:38:56PM +0100, Jim Meyering wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index f97ef18..d9a7aca 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1315,8 +1315,9 @@ static void qemudDispatchVMEvent(int fd, int events, void *opaque) { qemudDispatchVMFailure(driver, vm, fd); }
-static int qemudMonitorCommand(struct qemud_driver *driver ATTRIBUTE_UNUSED, - struct qemud_vm *vm, +static int qemudMonitorCommand(const struct qemud_driver *driver + ATTRIBUTE_UNUSED, + const struct qemud_vm *vm,
I don't like having annotations for a param on a separate line, because it doesn't read naturally. If we really need to wrap this, then lets follow the style of putting the return type on a line before , eg static int qemudMonitorCommand(const struct qemud_driver *driver ATTRIBUTE_UNUSED, const struct qemud_vm *vm, If it is still over 80 characters with this convention, then so be it. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Tue, Feb 26, 2008 at 03:38:56PM +0100, Jim Meyering wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index f97ef18..d9a7aca 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1315,8 +1315,9 @@ static void qemudDispatchVMEvent(int fd, int events, void *opaque) { qemudDispatchVMFailure(driver, vm, fd); }
-static int qemudMonitorCommand(struct qemud_driver *driver ATTRIBUTE_UNUSED, - struct qemud_vm *vm, +static int qemudMonitorCommand(const struct qemud_driver *driver + ATTRIBUTE_UNUSED, + const struct qemud_vm *vm,
I don't like having annotations for a param on a separate line, because it doesn't read naturally. If we really need to wrap this, then lets follow the style of putting the return type on a line before , eg
static int qemudMonitorCommand(const struct qemud_driver *driver ATTRIBUTE_UNUSED, const struct qemud_vm *vm,
Good idea. I prefer that anyhow, because it's what I'm used to and since some naive start-of-function-finding tools work better that way.
If it is still over 80 characters with this convention, then so be it.
Thanks everyone. I've committed that with the suggested changes. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering -
Richard W.M. Jones