[libvirt] [PATCH] virDomainDiskDef: Turn @device into enum
It's used as enum everywhere, so why store its value in an int? Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 9 +++------ src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 ++ src/vmx/vmx.c | 6 +++--- src/vmx/vmx.h | 2 +- src/xenconfig/xen_sxpr.c | 4 +++- 6 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6df1618..8ccce89 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7211,11 +7211,13 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, device = virXMLPropString(node, "device"); if (device) { - if ((def->device = virDomainDiskDeviceTypeFromString(device)) < 0) { + int dev = virDomainDiskDeviceTypeFromString(device); + if (dev < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk device '%s'"), device); goto error; } + def->device = dev; } else { def->device = VIR_DOMAIN_DISK_DEVICE_DISK; } @@ -18944,11 +18946,6 @@ virDomainDiskDefFormat(virBufferPtr buf, _("unexpected disk type %d"), def->src->type); return -1; } - if (!device) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk device %d"), def->device); - return -1; - } if (!bus) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected disk bus %d"), def->bus); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f043554..fe16c78 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -686,7 +686,7 @@ struct _virDomainDiskDef { virObjectPtr privateData; - int device; /* enum virDomainDiskDevice */ + virDomainDiskDevice device; int bus; /* enum virDomainDiskBus */ char *dst; int tray_status; /* enum virDomainDiskTray */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 25f57f2..781fc4a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10135,6 +10135,8 @@ qemuBuildCommandLine(virConnectPtr conn, bootindex = bootDisk; bootDisk = 0; break; + case VIR_DOMAIN_DISK_DEVICE_LAST: + break; } virCommandAddArg(cmd, "-drive"); diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 70cda2a..6d519b3 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1926,8 +1926,8 @@ virVMXParseSCSIController(virConfPtr conf, int controller, bool *present, int virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr conf, - int device, int busType, int controllerOrBus, int unit, - virDomainDiskDefPtr *def, virDomainDefPtr vmdef) + virDomainDiskDevice device, int busType, int controllerOrBus, + int unit, virDomainDiskDefPtr *def, virDomainDefPtr vmdef) { /* * device = {VIR_DOMAIN_DISK_DEVICE_DISK, @@ -3251,7 +3251,7 @@ virVMXFormatConfig(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virDomainDe break; - default: + case VIR_DOMAIN_DISK_DEVICE_LAST: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported disk device type '%s'"), virDomainDiskDeviceTypeToString(def->disks[i]->device)); diff --git a/src/vmx/vmx.h b/src/vmx/vmx.h index e986124..16b616b 100644 --- a/src/vmx/vmx.h +++ b/src/vmx/vmx.h @@ -88,7 +88,7 @@ int virVMXParseSCSIController(virConfPtr conf, int controller, bool *present, int *virtualDev); int virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, - virConfPtr conf, int device, int busType, + virConfPtr conf, virDomainDiskDevice device, int busType, int controllerOrBus, int unit, virDomainDiskDefPtr *def, virDomainDefPtr vmdef); diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c index 1d43ec1..1b2ddc6 100644 --- a/src/xenconfig/xen_sxpr.c +++ b/src/xenconfig/xen_sxpr.c @@ -2357,7 +2357,9 @@ xenFormatSxpr(virConnectPtr conn, virBufferEscapeSexpr(&buf, "'%s')", src); break; - default: + case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: + case VIR_DOMAIN_DISK_DEVICE_LAST: break; } } -- 2.4.6
On 09/17/2015 11:37 AM, Michal Privoznik wrote:
It's used as enum everywhere, so why store its value in an int?
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 9 +++------ src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 2 ++ src/vmx/vmx.c | 6 +++--- src/vmx/vmx.h | 2 +- src/xenconfig/xen_sxpr.c | 4 +++- 6 files changed, 13 insertions(+), 12 deletions(-)
Seems reasonable - ACK John Seems to be safe too
On 09/17/2015 11:37 AM, Michal Privoznik wrote:
It's used as enum everywhere, so why store its value in an int?
There are a lot of these. I *think* the only reason this was done anywhere in the first place was because the size of an enum isn't guaranteed to be the same across different platforms, so you can't use an enum type in any wire protocol (e.g. libvirt client to server), and somehow this usage leaked over into the internal-only data structures; after that the general "that works! Copy it!" cargo cult generation of new code meant that we now have lots of instances of this (there may also be cases where someone chose to use -1 to indicate "not specified" rather than 0/the first enum value). Or it could be there's some other reason for using int instead of an enum type in the internal config objects that I've forgotten.
On Mon, Sep 28, 2015 at 12:35:31PM -0400, Laine Stump wrote:
On 09/17/2015 11:37 AM, Michal Privoznik wrote:
It's used as enum everywhere, so why store its value in an int?
There are a lot of these. I *think* the only reason this was done anywhere in the first place was because the size of an enum isn't guaranteed to be the same across different platforms, so you can't use an enum type in any wire protocol (e.g. libvirt client to server), and somehow this usage leaked over into the internal-only data structures; after that the general "that works! Copy it!" cargo cult generation of new code meant that we now have lots of instances of this (there may also be cases where someone chose to use -1 to indicate "not specified" rather than 0/the first enum value).
Or it could be there's some other reason for using int instead of an enum type in the internal config objects that I've forgotten.
IIRC, it was because enum fields can be unsigned & we use -1 as the error code for virEnumFromString(). So if you have enum foo; if ((foo = virEnumFromString("blah")) < 0) ...error handling... your error handling would not get run, as the -1 would get turned into a high positive value. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange -
John Ferlan -
Laine Stump -
Michal Privoznik