6:34 p.m.
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 :|
3:31 a.m.
"Daniel P. Berrange" <berrange(a)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"))
6:54 a.m.
On Tue, May 13, 2008 at 10:31:10AM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)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 :|
8:02 a.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Tue, May 13, 2008 at 10:31:10AM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange(a)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.
10:11 a.m.
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(a)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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
6035
days inactive
6038
days old
4 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering