[libvirt] PATCH: Fix default bus type selection for disks
We recently added a new bus type attribute to disks to select between IDE, SCSI, etc. This attribute is optional and many tools won't set it. In such cases we default to IDE, which is clearly wrong. This patch updates it to pick the default bus based on the prefix of the disk target name, so that it DWIM, eg hdXXX == IDE, sdXXX == SCSI, xvdXXX == Xen, etc. qemu_conf.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) Dan diff -r 68901592c1ce src/qemu_conf.c --- a/src/qemu_conf.c Sat May 10 13:18:35 2008 -0400 +++ b/src/qemu_conf.c Sat May 10 13:55:59 2008 -0400 @@ -698,10 +698,20 @@ } if (!bus) { - if (disk->device == QEMUD_DISK_FLOPPY) + if (disk->device == QEMUD_DISK_FLOPPY) { disk->bus = QEMUD_DISK_BUS_FDC; - else - disk->bus = QEMUD_DISK_BUS_IDE; + } else { + if (STRPREFIX((const char *)target, "hd")) + disk->bus = QEMUD_DISK_BUS_IDE; + else if (STRPREFIX((const char *)target, "sd")) + disk->bus = QEMUD_DISK_BUS_SCSI; + else if (STRPREFIX((const char *)target, "vd")) + disk->bus = QEMUD_DISK_BUS_VIRTIO; + else if (STRPREFIX((const char *)target, "xvd")) + disk->bus = QEMUD_DISK_BUS_XEN; + else + disk->bus = QEMUD_DISK_BUS_IDE; + } } else if (STREQ((const char *)bus, "ide")) disk->bus = QEMUD_DISK_BUS_IDE; else if (STREQ((const char *)bus, "fdc")) -- |: Red Hat, Engineering, Boston -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 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
We recently added a new bus type attribute to disks to select between IDE, SCSI, etc. This attribute is optional and many tools won't set it. In such cases we default to IDE, which is clearly wrong. This patch updates it to pick the default bus based on the prefix of the disk target name, so that it DWIM, eg hdXXX == IDE, sdXXX == SCSI, xvdXXX == Xen, etc.
qemu_conf.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
Dan
diff -r 68901592c1ce src/qemu_conf.c --- a/src/qemu_conf.c Sat May 10 13:18:35 2008 -0400 +++ b/src/qemu_conf.c Sat May 10 13:55:59 2008 -0400 @@ -698,10 +698,20 @@ }
if (!bus) { - if (disk->device == QEMUD_DISK_FLOPPY) + if (disk->device == QEMUD_DISK_FLOPPY) { disk->bus = QEMUD_DISK_BUS_FDC; - else - disk->bus = QEMUD_DISK_BUS_IDE; + } else { + if (STRPREFIX((const char *)target, "hd")) + disk->bus = QEMUD_DISK_BUS_IDE; + else if (STRPREFIX((const char *)target, "sd")) + disk->bus = QEMUD_DISK_BUS_SCSI; + else if (STRPREFIX((const char *)target, "vd")) + disk->bus = QEMUD_DISK_BUS_VIRTIO; + else if (STRPREFIX((const char *)target, "xvd")) + disk->bus = QEMUD_DISK_BUS_XEN; + else + disk->bus = QEMUD_DISK_BUS_IDE; + } } else if (STREQ((const char *)bus, "ide")) disk->bus = QEMUD_DISK_BUS_IDE; else if (STREQ((const char *)bus, "fdc"))
ACK, in spite of the proliferation of casts -- That's not good for readability/maintainability. What do you think of this? static inline char *xml2char(xmlChar *x) { return (char *) x; } The uses are still ugly, but at least they're safer: (note that the parameter cannot be a "const" pointer because the incoming xmlChar* is almost always non-const, as it must be, since it's going to be freed).
if (!bus) { - if (disk->device == QEMUD_DISK_FLOPPY) + if (disk->device == QEMUD_DISK_FLOPPY) { disk->bus = QEMUD_DISK_BUS_FDC; - else - disk->bus = QEMUD_DISK_BUS_IDE; + } else { + if (STRPREFIX(xml2char(target), "hd")) + disk->bus = QEMUD_DISK_BUS_IDE; + else if (STRPREFIX(xml2char(target), "sd")) + disk->bus = QEMUD_DISK_BUS_SCSI; + else if (STRPREFIX(xml2char(target), "vd")) + disk->bus = QEMUD_DISK_BUS_VIRTIO; + else if (STRPREFIX(xml2char(target), "xvd")) + disk->bus = QEMUD_DISK_BUS_XEN; + else + disk->bus = QEMUD_DISK_BUS_IDE; + } } else if (STREQ(xml2char(bus), "ide")) disk->bus = QEMUD_DISK_BUS_IDE; else if (STREQ(xml2char(bus), "fdc"))
On Tue, May 13, 2008 at 10:31:10AM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
in spite of the proliferation of casts -- That's not good for readability/maintainability.
What do you think of this?
static inline char *xml2char(xmlChar *x) { return (char *) x; }
The uses are still ugly, but at least they're safer: (note that the parameter cannot be a "const" pointer because the incoming xmlChar* is almost always non-const, as it must be, since it's going to be freed).
I'd suggest going one better and defining a thing wrapper around the xmlNodeGetProp method char *virXMLGetProp(xmlNodePtr *node, const char *name) { return (char *)xmlNodeGetProp(node, BAD_CAST name); } That should let us get rid of all these casts throughout the code The only potential issue would be that xmlChar * is technically supposed to be free via the xmlFree() method, rather than free(), but I believe they're defined to be identical unless special debug allocators are registered ? Dan. -- |: Red Hat, Engineering, Boston -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 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Tue, May 13, 2008 at 10:31:10AM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
in spite of the proliferation of casts -- That's not good for readability/maintainability.
What do you think of this?
static inline char *xml2char(xmlChar *x) { return (char *) x; }
The uses are still ugly, but at least they're safer: (note that the parameter cannot be a "const" pointer because the incoming xmlChar* is almost always non-const, as it must be, since it's going to be freed).
I'd suggest going one better and defining a thing wrapper around the xmlNodeGetProp method
char *virXMLGetProp(xmlNodePtr *node, const char *name) { return (char *)xmlNodeGetProp(node, BAD_CAST name); }
That should let us get rid of all these casts throughout the code
Good idea. Do you want to do it? or shall I...
The only potential issue would be that xmlChar * is technically supposed to be free via the xmlFree() method, rather than free(), but I believe they're defined to be identical unless special debug allocators are registered ?
Right. When I removed some useless if-before-xmlFree tests, Daniel Veillard assured us that xmlFree-in-libvirt should always be "free", so this is ok.
On Tue, May 13, 2008 at 12:54:58PM +0100, Daniel P. Berrange wrote:
On Tue, May 13, 2008 at 10:31:10AM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
in spite of the proliferation of casts -- That's not good for readability/maintainability.
What do you think of this?
static inline char *xml2char(xmlChar *x) { return (char *) x; }
The uses are still ugly, but at least they're safer: (note that the parameter cannot be a "const" pointer because the incoming xmlChar* is almost always non-const, as it must be, since it's going to be freed).
I'd suggest going one better and defining a thing wrapper around the xmlNodeGetProp method
char *virXMLGetProp(xmlNodePtr *node, const char *name) { return (char *)xmlNodeGetProp(node, BAD_CAST name); }
That should let us get rid of all these casts throughout the code
The explanation for the casts is that libxml2 uses xmlChar * to indicate that the target string is UTF-8, i.e. you can actually know what's inside without guessing ...
The only potential issue would be that xmlChar * is technically supposed to be free via the xmlFree() method, rather than free(), but I believe they're defined to be identical unless special debug allocators are registered ?
Right, 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/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering