[libvirt] [PATCH] qemu: fix block stats for virtio and scsi
Hi, qemudDomainBlockStats needs to map between qemu and libvirt device naming as well. Possible patch attached. Cheers, -- Guido
On Thu, Oct 02, 2008 at 09:56:51PM +0200, Guido G?nther wrote:
Hi, qemudDomainBlockStats needs to map between qemu and libvirt device naming as well. Possible patch attached. Cheers, -- Guido
From 57b8c8e66abfcc57c2f05d8b8364de6ecc05dcf9 Mon Sep 17 00:00:00 2001 From: Guido Guenther <agx@sigxcpu.org> Date: Thu, 2 Oct 2008 21:12:20 +0200 Subject: [PATCH] support virtio and scsi disks in qemudDomainBlockStats
--- src/qemu_driver.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index a1e7285..2a7c555 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -3416,6 +3416,8 @@ qemudDomainBlockStats (virDomainPtr dom, * hd[a-] to ide0-hd[0-] * cdrom to ide1-cd0 * fd[a-] to floppy[0-] + * vd[a-] to virtio[0-] + * sd[a-] to scsi0-hd[0-] */ if (STRPREFIX (path, "hd") && c_islower(path[2])) snprintf (qemu_dev_name, sizeof (qemu_dev_name), @@ -3425,6 +3427,12 @@ qemudDomainBlockStats (virDomainPtr dom, else if (STRPREFIX (path, "fd") && c_islower(path[2])) snprintf (qemu_dev_name, sizeof (qemu_dev_name), "floppy%d", path[2] - 'a'); + else if (STRPREFIX (path, "vd") && c_islower(path[2])) + snprintf (qemu_dev_name, sizeof (qemu_dev_name), + "virtio%d", path[2] - 'a'); + else if (STRPREFIX (path, "sd") && c_islower(path[2])) + snprintf (qemu_dev_name, sizeof (qemu_dev_name), + "scsi0-hd%d", path[2] - 'a');
This isn't correct - it is assuming only a single letter after the prefix - while true for IDE & Floppy, it doesn't hold true for SCSI which can have more than 26 devices, so goes to double letters. We also need to consider Xen style xvdXX devices for Xenner. I think this needs to use the virDiskNameToIndex() method to extract the index instead. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Guido Günther wrote:
On Thu, Oct 02, 2008 at 09:06:25PM +0100, Daniel P. Berrange wrote:
I think this needs to use the virDiskNameToIndex() method to extract the index instead. Corrected pach attached. I don't know how devices are called when using xenner so I left that out. -- Guido
There is actually a function in qemu_driver.c called qemudDiskDeviceName that handles a lot of this already, it's used for generating the connect/eject cdrom names. It's probably best to reuse that. Thanks, Cole
Guido Günther wrote:
On Thu, Oct 02, 2008 at 09:06:25PM +0100, Daniel P. Berrange wrote:
I think this needs to use the virDiskNameToIndex() method to extract the index instead. Corrected pach attached. I don't know how devices are called when using xenner so I left that out. -- Guido
There is actually a function in qemu_driver.c called qemudDiskDeviceName that handles a lot of this already, it's used for generating the connect/eject cdrom names. It's probably best to reuse that. Yes this makes sense. After fixing things up a little in virDiskNameToBusDeviceIndex and qemudDiskDeviceName this works. I'll
On Tue, Oct 07, 2008 at 03:03:10PM -0400, Cole Robinson wrote: post this as 3 tiny patches since they might be helpful on their own and can be applied in case there's still something wrong with the blockstats. -- Guido
Hi, 0 looks like a valid index as returned by virDiskNameToIndex, so we should accept it. Also the disk argument can be const. -- Guido
Guido Günther wrote:
Hi, 0 looks like a valid index as returned by virDiskNameToIndex, so we should accept it. Also the disk argument can be const. -- Guido
ACK, needed to accommodate recent change to virDiskNameToIndex at the least. Thanks, Cole
On Fri, Oct 10, 2008 at 05:25:28PM +0200, Guido G?nther wrote:
Hi, 0 looks like a valid index as returned by virDiskNameToIndex, so we should accept it. Also the disk argument can be const.
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Oct 10, 2008 at 05:25:28PM +0200, Guido Günther wrote:
Hi, 0 looks like a valid index as returned by virDiskNameToIndex, so we should accept it. Also the disk argument can be const.
Okidoc, applied, thanks ! 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/
Hi, qemuDiskDeviceName treat's all IDE and SCSI devices as cdroms, make it also handle disks. Make funcion arguments const. -- Guido
On Fri, Oct 10, 2008 at 05:27:24PM +0200, Guido G?nther wrote:
Hi, qemuDiskDeviceName treat's all IDE and SCSI devices as cdroms, make it also handle disks. Make funcion arguments const.
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Oct 10, 2008 at 05:27:24PM +0200, Guido Günther wrote:
Hi, qemuDiskDeviceName treat's all IDE and SCSI devices as cdroms, make it also handle disks. Make funcion arguments const.
Applied too to CVS, thanks ! 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/
Hi, use qemudDiskDeviceName to determine the block device name (as suggested by Cole). -- Guido
Guido Günther wrote:
Hi, use qemudDiskDeviceName to determine the block device name (as suggested by Cole). -- Guido
From 6985fee585561b04942036d283632e7cb2bb708f Mon Sep 17 00:00:00 2001 From: Guido Guenther <agx@sigxcpu.org> Date: Thu, 2 Oct 2008 21:12:20 +0200 Subject: [PATCH] support virtio and scsi disks in qemudDomainBlockStats
--- src/qemu_driver.c | 56 ++++++++++++++++++++++++---------------------------- 1 files changed, 26 insertions(+), 30 deletions(-)
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index b2ea0d1..5ffaf21 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -3397,11 +3397,13 @@ qemudDomainBlockStats (virDomainPtr dom, { const struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; - char *dummy, *info; + char *dummy, *info = NULL; const char *p, *eol; - char qemu_dev_name[32]; + const char *qemu_dev_name = NULL; size_t len; + int ret = -1; const virDomainObjPtr vm = virDomainFindByID(driver->domains, dom->id); + virDomainDiskDefPtr disk;
if (!vm) { qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, @@ -3414,34 +3416,31 @@ qemudDomainBlockStats (virDomainPtr dom, return -1; }
- /* - * QEMU internal block device names are different from the device - * names we use in libvirt, so we need to map between them: - * - * hd[a-] to ide0-hd[0-] - * cdrom to ide1-cd0 - * fd[a-] to floppy[0-] - */ - if (STRPREFIX (path, "hd") && c_islower(path[2])) - 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"); - else if (STRPREFIX (path, "fd") && c_islower(path[2])) - snprintf (qemu_dev_name, sizeof (qemu_dev_name), - "floppy%d", path[2] - 'a'); - else { + disk = vm->def->disks; + while (disk) { + if (STREQ(disk->dst, path)) + break; + disk = disk->next; + } + + if (!disk) { qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG, _("invalid path: %s"), path); return -1; }
+ qemu_dev_name = qemudDiskDeviceName(dom, disk); + if (!qemu_dev_name) { + qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG, + _("unknown path %s"), path); + return -1; + }
You should probably just return -1 here, since qemudDiskDeviceName sets its own error message. The rest looks fine to me. Thanks, Cole
On Fri, Oct 10, 2008 at 05:28:54PM +0200, Guido G?nther wrote:
Hi, use qemudDiskDeviceName to determine the block device name (as suggested by Cole). -- Guido
+ disk = vm->def->disks; + while (disk) { + if (STREQ(disk->dst, path)) + break; + disk = disk->next; + }
Sorry to mess up your patch, but I just committed the code to turn all linked lists into arrays. So you'll need to tweak this to do for (i = 0 ; i < vm->def->ndisks ; i++) if (STREQ(vm->def->disks[i]->dst)) break; Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Oct 10, 2008 at 06:05:37PM +0100, Daniel P. Berrange wrote:
Sorry to mess up your patch, but I just committed the code to turn all linked lists into arrays. So you'll need to tweak this to do
for (i = 0 ; i < vm->def->ndisks ; i++) if (STREQ(vm->def->disks[i]->dst)) break; Fixed version attached. This one also addresses Cole's comment on the error message. -- Guido
Guido Günther wrote:
On Fri, Oct 10, 2008 at 06:05:37PM +0100, Daniel P. Berrange wrote:
Sorry to mess up your patch, but I just committed the code to turn all linked lists into arrays. So you'll need to tweak this to do
for (i = 0 ; i < vm->def->ndisks ; i++) if (STREQ(vm->def->disks[i]->dst)) break; Fixed version attached. This one also addresses Cole's comment on the error message. -- Guido
ACK. Thanks, Cole
On Tue, Oct 14, 2008 at 09:29:38AM -0400, Cole Robinson wrote:
Guido Günther wrote:
On Fri, Oct 10, 2008 at 06:05:37PM +0100, Daniel P. Berrange wrote:
Sorry to mess up your patch, but I just committed the code to turn all linked lists into arrays. So you'll need to tweak this to do
for (i = 0 ; i < vm->def->ndisks ; i++) if (STREQ(vm->def->disks[i]->dst)) break; Fixed version attached. This one also addresses Cole's comment on the error message. -- Guido
ACK. So can the be applied (together with the other two fixups)? -- Guido
On Thu, Oct 16, 2008 at 05:29:58PM +0200, Guido Günther wrote:
On Tue, Oct 14, 2008 at 09:29:38AM -0400, Cole Robinson wrote:
Guido Günther wrote:
On Fri, Oct 10, 2008 at 06:05:37PM +0100, Daniel P. Berrange wrote:
Sorry to mess up your patch, but I just committed the code to turn all linked lists into arrays. So you'll need to tweak this to do
for (i = 0 ; i < vm->def->ndisks ; i++) if (STREQ(vm->def->disks[i]->dst)) break; Fixed version attached. This one also addresses Cole's comment on the error message. -- Guido
ACK. So can the be applied (together with the other two fixups)?
Sure, all 3 commited now, thanks a lot ! 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/
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard -
Guido Günther