[libvirt] [PATCH 0/4] more enum cleanups
Inspired by the cleanups contributed by Julio Faracco, I did some more cleanups of my own. My end goal is to turn on a syntax-check rule that forbids 'enum vir' (since we should always be declaring 'typedef enum { ... } vir...;'), but there's still a lot of violations in the code base, even after this series. Eric Blake (4): vbox: fix stale comment about vdi storage type maint: use enum typedef for virstorageencryption.h maint: shorten 'TypeType' function names maint: prefer enum over int for virstoragefile structs src/conf/domain_conf.c | 14 +++++++------- src/conf/domain_conf.h | 2 +- src/conf/secret_conf.c | 8 ++++---- src/conf/secret_conf.h | 4 ++-- src/conf/storage_conf.c | 14 +++++++------- src/conf/storage_conf.h | 6 +++--- src/libvirt_private.syms | 10 +++++----- src/lxc/lxc_controller.c | 2 +- src/qemu/qemu_command.c | 8 ++++++++ src/qemu/qemu_driver.c | 8 ++++---- src/storage/storage_backend_disk.c | 2 +- src/util/virstorageencryption.c | 6 +++--- src/util/virstorageencryption.h | 10 +++++----- src/util/virstoragefile.h | 19 ++++++++++--------- src/vbox/vbox_tmpl.c | 15 ++++----------- tools/virsh-secret.c | 4 ++-- 16 files changed, 67 insertions(+), 65 deletions(-) -- 1.9.0
The code had some todo's about adding 'vdi' to the list of virStorageType, but we've already done that. * src/vbox/vbox_tmpl.c (vboxStorageVolCreateXML) (vboxStorageVolGetXMLDesc): Use enum value for vdi type. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/vbox/vbox_tmpl.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index a305fe2..e124e69 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -8763,13 +8763,8 @@ static virStorageVolPtr vboxStorageVolCreateXML(virStoragePoolPtr pool, (def->type != VIR_STORAGE_VOL_FILE)) goto cleanup; - /* TODO: for now only the vmdk, vpc and vdi type harddisk - * variants can be created, also since there is no vdi - * type in enum virStorageFileFormat {} the default - * will be to create vdi if nothing is specified in - * def->target.format - */ - + /* For now only the vmdk, vpc and vdi type harddisk + * variants can be created. For historical reason, we default to vdi */ if (def->target.format == VIR_STORAGE_FILE_VMDK) { VBOX_UTF8_TO_UTF16("VMDK", &hddFormatUtf16); } else if (def->target.format == VIR_STORAGE_FILE_VPC) { @@ -9175,13 +9170,11 @@ static char *vboxStorageVolGetXMLDesc(virStorageVolPtr vol, unsigned int flags) def.target.format = VIR_STORAGE_FILE_VMDK; else if (STRCASEEQ("vhd", hddFormatUtf8)) def.target.format = VIR_STORAGE_FILE_VPC; + else if (STRCASEEQ("vdi", hddFormatUtf8)) + def.target.format = VIR_STORAGE_FILE_VDI; else def.target.format = VIR_STORAGE_FILE_RAW; - /* TODO: need to add vdi to enum virStorageFileFormat {} - * and then add it here - */ - VBOX_UTF8_FREE(hddFormatUtf8); } -- 1.9.0
Continuing the work of consistent enum cleanups; this time in virstorageencryption.h. * src/util/virstorageencryption.h (virStorageEncryptionFormat): Convert to typedef, renaming to avoid collision with function. (virStorageEncryptionSecret, virStorageEncryption): Directly use enums. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstorageencryption.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/virstorageencryption.h b/src/util/virstorageencryption.h index bf83d34..f146249 100644 --- a/src/util/virstorageencryption.h +++ b/src/util/virstorageencryption.h @@ -39,23 +39,23 @@ VIR_ENUM_DECL(virStorageEncryptionSecretType) typedef struct _virStorageEncryptionSecret virStorageEncryptionSecret; typedef virStorageEncryptionSecret *virStorageEncryptionSecretPtr; struct _virStorageEncryptionSecret { - int type; /* enum virStorageEncryptionSecretType */ + virStorageEncryptionSecretType type; unsigned char uuid[VIR_UUID_BUFLEN]; }; -enum virStorageEncryptionFormat { +typedef enum { /* "default" is only valid for volume creation */ VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT = 0, VIR_STORAGE_ENCRYPTION_FORMAT_QCOW, /* Both qcow and qcow2 */ VIR_STORAGE_ENCRYPTION_FORMAT_LAST, -}; +} virStorageEncryptionFormatType; VIR_ENUM_DECL(virStorageEncryptionFormat) typedef struct _virStorageEncryption virStorageEncryption; typedef virStorageEncryption *virStorageEncryptionPtr; struct _virStorageEncryption { - int format; /* enum virStorageEncryptionFormat */ + virStorageEncryptionFormatType format; size_t nsecrets; virStorageEncryptionSecretPtr *secrets; -- 1.9.0
The VIR_ENUM_DECL/VIR_ENUM_IMPL helper macros already append 'Type' to the enum name being converted; it looks silly to have functions with 'TypeType' in their name. Even though some of our enums have to have a 'Type' suffix, the corresponding string conversion functions do not. * src/conf/secret_conf.h (VIR_ENUM_DECL): Rename virSecretUsageType. * src/conf/storage_conf.h (VIR_ENUM_DECL): Rename virStoragePoolAuthType, virStoragePoolSourceAdapterType, virStoragePartedFsType. * src/conf/domain_conf.c (virDomainDiskDefParseXML) (virDomainFSDefParseXML, virDomainFSDefFormat): Update callers. * src/conf/secret_conf.c (virSecretDefParseUsage) (virSecretDefFormatUsage): Likewise. * src/conf/storage_conf.c (virStoragePoolDefParseAuth) (virStoragePoolDefParseSource, virStoragePoolSourceFormat): Likewise. * src/lxc/lxc_controller.c (virLXCControllerSetupLoopDevices): Likewise. * src/storage/storage_backend_disk.c (virStorageBackendDiskPartFormat): Likewise. * src/util/virstorageencryption.c (virStorageEncryptionSecretParse) (virStorageEncryptionSecretFormat): Likewise. * tools/virsh-secret.c (cmdSecretList): Likewise. * src/libvirt_private.syms (secret_conf.h, storage_conf.h): Export corrected names. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 10 +++++----- src/conf/domain_conf.h | 2 +- src/conf/secret_conf.c | 8 ++++---- src/conf/secret_conf.h | 4 ++-- src/conf/storage_conf.c | 14 +++++++------- src/conf/storage_conf.h | 6 +++--- src/libvirt_private.syms | 10 +++++----- src/lxc/lxc_controller.c | 2 +- src/storage/storage_backend_disk.c | 2 +- src/util/virstorageencryption.c | 6 +++--- src/util/virstorageencryption.h | 2 +- tools/virsh-secret.c | 4 ++-- 12 files changed, 35 insertions(+), 35 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6c3bdad..e65b62b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -329,7 +329,7 @@ VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST, "ram", "bind") -VIR_ENUM_IMPL(virDomainFSDriverType, VIR_DOMAIN_FS_DRIVER_TYPE_LAST, +VIR_ENUM_IMPL(virDomainFSDriver, VIR_DOMAIN_FS_DRIVER_TYPE_LAST, "default", "path", "handle", @@ -5375,7 +5375,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } auth_secret_usage = - virSecretUsageTypeTypeFromString(usageType); + virSecretUsageTypeFromString(usageType); if (auth_secret_usage < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("invalid secret type %s"), @@ -5537,7 +5537,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if (auth_secret_usage != -1 && auth_secret_usage != expected_secret_usage) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid secret type '%s'"), - virSecretUsageTypeTypeToString(auth_secret_usage)); + virSecretUsageTypeToString(auth_secret_usage)); goto error; } @@ -6313,7 +6313,7 @@ virDomainFSDefParseXML(xmlNodePtr node, } if (fsdriver) { - if ((def->fsdriver = virDomainFSDriverTypeTypeFromString(fsdriver)) <= 0) { + if ((def->fsdriver = virDomainFSDriverTypeFromString(fsdriver)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown fs driver type '%s'"), fsdriver); goto error; @@ -15327,7 +15327,7 @@ virDomainFSDefFormat(virBufferPtr buf, { const char *type = virDomainFSTypeToString(def->type); const char *accessmode = virDomainFSAccessModeTypeToString(def->accessmode); - const char *fsdriver = virDomainFSDriverTypeTypeToString(def->fsdriver); + const char *fsdriver = virDomainFSDriverTypeToString(def->fsdriver); const char *wrpolicy = virDomainFSWrpolicyTypeToString(def->wrpolicy); if (!type) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9c80096..bde303c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2554,7 +2554,7 @@ VIR_ENUM_DECL(virDomainControllerModelPCI) VIR_ENUM_DECL(virDomainControllerModelSCSI) VIR_ENUM_DECL(virDomainControllerModelUSB) VIR_ENUM_DECL(virDomainFS) -VIR_ENUM_DECL(virDomainFSDriverType) +VIR_ENUM_DECL(virDomainFSDriver) VIR_ENUM_DECL(virDomainFSAccessMode) VIR_ENUM_DECL(virDomainFSWrpolicy) VIR_ENUM_DECL(virDomainNet) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 58e12c0..d85bb4e 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -1,7 +1,7 @@ /* * secret_conf.c: internal <secret> XML handling * - * Copyright (C) 2009, 2011, 2013, 2014 Red Hat, Inc. + * Copyright (C) 2009-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -36,7 +36,7 @@ VIR_LOG_INIT("conf.secret_conf"); -VIR_ENUM_IMPL(virSecretUsageType, VIR_SECRET_USAGE_TYPE_LAST, +VIR_ENUM_IMPL(virSecretUsage, VIR_SECRET_USAGE_TYPE_LAST, "none", "volume", "ceph", "iscsi") void @@ -82,7 +82,7 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt, _("unknown secret usage type")); return -1; } - type = virSecretUsageTypeTypeFromString(type_str); + type = virSecretUsageTypeFromString(type_str); if (type < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown secret usage type %s"), type_str); @@ -249,7 +249,7 @@ virSecretDefFormatUsage(virBufferPtr buf, { const char *type; - type = virSecretUsageTypeTypeToString(def->usage_type); + type = virSecretUsageTypeToString(def->usage_type); if (type == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected secret usage type %d"), diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 54d7e76..9c13f05 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -1,7 +1,7 @@ /* * secret_conf.h: internal <secret> XML handling API * - * Copyright (C) 2009-2010, 2013 Red Hat, Inc. + * Copyright (C) 2009-2010, 2013-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -26,7 +26,7 @@ # include "internal.h" # include "virutil.h" -VIR_ENUM_DECL(virSecretUsageType) +VIR_ENUM_DECL(virSecretUsage) typedef struct _virSecretDef virSecretDef; typedef virSecretDef *virSecretDefPtr; diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 9769b19..8b6fd79 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -87,18 +87,18 @@ VIR_ENUM_IMPL(virStorageVolFormatDisk, "linux-lvm", "linux-raid", "extended") -VIR_ENUM_IMPL(virStoragePartedFsType, +VIR_ENUM_IMPL(virStoragePartedFs, VIR_STORAGE_PARTED_FS_TYPE_LAST, "ext2", "ext2", "fat16", "fat32", "linux-swap", "ext2", "ext2", "extended") -VIR_ENUM_IMPL(virStoragePoolSourceAdapterType, +VIR_ENUM_IMPL(virStoragePoolSourceAdapter, VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_LAST, "default", "scsi_host", "fc_host") -VIR_ENUM_IMPL(virStoragePoolAuthType, +VIR_ENUM_IMPL(virStoragePoolAuth, VIR_STORAGE_POOL_AUTH_LAST, "none", "chap", "ceph") @@ -514,7 +514,7 @@ virStoragePoolDefParseAuth(xmlXPathContextPtr ctxt, } if ((source->authType = - virStoragePoolAuthTypeTypeFromString(authType)) < 0) { + virStoragePoolAuthTypeFromString(authType)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown auth type '%s'"), authType); @@ -657,7 +657,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, if ((adapter_type = virXPathString("string(./adapter/@type)", ctxt))) { if ((source->adapter.type = - virStoragePoolSourceAdapterTypeTypeFromString(adapter_type)) <= 0) { + virStoragePoolSourceAdapterTypeFromString(adapter_type)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown pool adapter type '%s'"), adapter_type); @@ -1100,7 +1100,7 @@ virStoragePoolSourceFormat(virBufferPtr buf, if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST || src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) virBufferAsprintf(buf, "<adapter type='%s'", - virStoragePoolSourceAdapterTypeTypeToString(src->adapter.type)); + virStoragePoolSourceAdapterTypeToString(src->adapter.type)); if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { virBufferEscapeString(buf, " parent='%s'", @@ -1141,7 +1141,7 @@ virStoragePoolSourceFormat(virBufferPtr buf, if (src->authType == VIR_STORAGE_POOL_AUTH_CHAP || src->authType == VIR_STORAGE_POOL_AUTH_CEPHX) { virBufferAsprintf(buf, "<auth type='%s' ", - virStoragePoolAuthTypeTypeToString(src->authType)); + virStoragePoolAuthTypeToString(src->authType)); virBufferEscapeString(buf, "username='%s'>\n", (src->authType == VIR_STORAGE_POOL_AUTH_CHAP ? src->auth.chap.username : diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index b10d4f2..04d99eb 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -113,7 +113,7 @@ typedef enum { VIR_STORAGE_POOL_AUTH_LAST, } virStoragePoolAuthType; -VIR_ENUM_DECL(virStoragePoolAuthType) +VIR_ENUM_DECL(virStoragePoolAuth) typedef struct _virStoragePoolAuthSecret virStoragePoolAuthSecret; typedef virStoragePoolAuthSecret *virStoragePoolAuthSecretPtr; @@ -204,7 +204,7 @@ typedef enum { VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_LAST, } virStoragePoolSourceAdapterType; -VIR_ENUM_DECL(virStoragePoolSourceAdapterType) +VIR_ENUM_DECL(virStoragePoolSourceAdapter) typedef struct _virStoragePoolSourceAdapter virStoragePoolSourceAdapter; struct _virStoragePoolSourceAdapter { @@ -513,7 +513,7 @@ typedef enum { VIR_STORAGE_PARTED_FS_TYPE_EXTENDED, VIR_STORAGE_PARTED_FS_TYPE_LAST, } virStoragePartedFsType; -VIR_ENUM_DECL(virStoragePartedFsType) +VIR_ENUM_DECL(virStoragePartedFs) # define VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ACTIVE \ (VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE | \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0a5fb14..c3332c9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -671,8 +671,8 @@ virSecretDefFormat; virSecretDefFree; virSecretDefParseFile; virSecretDefParseString; -virSecretUsageTypeTypeFromString; -virSecretUsageTypeTypeToString; +virSecretUsageTypeFromString; +virSecretUsageTypeToString; # conf/snapshot_conf.h @@ -701,7 +701,7 @@ virDomainSnapshotUpdateRelations; # conf/storage_conf.h -virStoragePartedFsTypeTypeToString; +virStoragePartedFsTypeToString; virStoragePoolDefFormat; virStoragePoolDefFree; virStoragePoolDefParseFile; @@ -724,8 +724,8 @@ virStoragePoolObjLock; virStoragePoolObjRemove; virStoragePoolObjSaveDef; virStoragePoolObjUnlock; -virStoragePoolSourceAdapterTypeTypeFromString; -virStoragePoolSourceAdapterTypeTypeToString; +virStoragePoolSourceAdapterTypeFromString; +virStoragePoolSourceAdapterTypeToString; virStoragePoolSourceClear; virStoragePoolSourceDeviceClear; virStoragePoolSourceFindDuplicate; diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index c05dfec..5521c6e 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -523,7 +523,7 @@ static int virLXCControllerSetupLoopDevices(virLXCControllerPtr ctrl) } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("fs driver %s is not supported"), - virDomainFSDriverTypeTypeToString(fs->fsdriver)); + virDomainFSDriverTypeToString(fs->fsdriver)); goto cleanup; } } diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index b8e69bb..8e12974 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -484,7 +484,7 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool, size_t i; if (pool->def->source.format == VIR_STORAGE_POOL_DISK_DOS) { const char *partedFormat; - partedFormat = virStoragePartedFsTypeTypeToString(vol->target.format); + partedFormat = virStoragePartedFsTypeToString(vol->target.format); if (partedFormat == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid partition type")); diff --git a/src/util/virstorageencryption.c b/src/util/virstorageencryption.c index 9089278..1306490 100644 --- a/src/util/virstorageencryption.c +++ b/src/util/virstorageencryption.c @@ -37,7 +37,7 @@ #define VIR_FROM_THIS VIR_FROM_STORAGE -VIR_ENUM_IMPL(virStorageEncryptionSecretType, +VIR_ENUM_IMPL(virStorageEncryptionSecret, VIR_STORAGE_ENCRYPTION_SECRET_TYPE_LAST, "passphrase") VIR_ENUM_IMPL(virStorageEncryptionFormat, @@ -88,7 +88,7 @@ virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt, _("unknown volume encryption secret type")); goto cleanup; } - type = virStorageEncryptionSecretTypeTypeFromString(type_str); + type = virStorageEncryptionSecretTypeFromString(type_str); if (type < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown volume encryption secret type %s"), @@ -209,7 +209,7 @@ virStorageEncryptionSecretFormat(virBufferPtr buf, const char *type; char uuidstr[VIR_UUID_STRING_BUFLEN]; - type = virStorageEncryptionSecretTypeTypeToString(secret->type); + type = virStorageEncryptionSecretTypeToString(secret->type); if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unexpected volume encryption secret type")); diff --git a/src/util/virstorageencryption.h b/src/util/virstorageencryption.h index f146249..0a9bf44 100644 --- a/src/util/virstorageencryption.h +++ b/src/util/virstorageencryption.h @@ -34,7 +34,7 @@ typedef enum { VIR_STORAGE_ENCRYPTION_SECRET_TYPE_LAST } virStorageEncryptionSecretType; -VIR_ENUM_DECL(virStorageEncryptionSecretType) +VIR_ENUM_DECL(virStorageEncryptionSecret) typedef struct _virStorageEncryptionSecret virStorageEncryptionSecret; typedef virStorageEncryptionSecret *virStorageEncryptionSecretPtr; diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index bbe83cf..add2c09 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -1,7 +1,7 @@ /* * virsh-secret.c: Commands to manage secret * - * Copyright (C) 2005, 2007-2013 Red Hat, Inc. + * Copyright (C) 2005, 2007-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -539,7 +539,7 @@ cmdSecretList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) for (i = 0; i < list->nsecrets; i++) { virSecretPtr sec = list->secrets[i]; int usageType = virSecretGetUsageType(sec); - const char *usageStr = virSecretUsageTypeTypeToString(usageType); + const char *usageStr = virSecretUsageTypeToString(usageType); char uuid[VIR_UUID_STRING_BUFLEN]; if (virSecretGetUUIDString(sec, uuid) < 0) { -- 1.9.0
For internal structs, we might as well be type-safe and let the compiler help us with less typing required on our part (getting rid of casts is always nice). In trying to use enums directly, I noticed two problems in virstoragefile.h that can't be fixed without more invasive refactoring: virStorageSource.format is used as more of a union of multiple enums in storage volume code (so it has to remain an int), and virStorageSourcePoolDef refers to pooltype whose enum is declared in src/conf, but where src/util can't pull in headers from src/conf. * src/util/virstoragefile.h (virStorageNetHostDef) (virStorageSourcePoolDef, virStorageSource): Use enums instead of int for fields of internal types. * src/qemu/qemu_command.c (qemuParseCommandLine): Cover all values. * src/conf/domain_conf.c (virDomainDiskSourceParse) (virDomainDiskSourceFormat): Simplify clients. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateSingleDiskActive) (qemuDomainSnapshotPrepareDiskExternalBackingInactive) (qemuDomainSnapshotPrepareDiskExternalOverlayActive) (qemuDomainSnapshotPrepareDiskInternal): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 4 ++-- src/qemu/qemu_command.c | 8 ++++++++ src/qemu/qemu_driver.c | 8 ++++---- src/util/virstoragefile.h | 19 ++++++++++--------- 4 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e65b62b..e5ae7c6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4972,7 +4972,7 @@ virDomainDiskSourceParse(xmlNodePtr node, memset(&host, 0, sizeof(host)); - switch ((virStorageType)src->type) { + switch (src->type) { case VIR_STORAGE_TYPE_FILE: src->path = virXMLPropString(node, "file"); break; @@ -14847,7 +14847,7 @@ virDomainDiskSourceFormat(virBufferPtr buf, startupPolicy = virDomainStartupPolicyTypeToString(policy); if (src->path || src->nhosts > 0 || src->srcpool || startupPolicy) { - switch ((virStorageType)src->type) { + switch (src->type) { case VIR_STORAGE_TYPE_FILE: virBufferAddLit(buf, "<source"); virBufferEscapeString(buf, " file='%s'", src->path); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cfd1bcf..9ae1a96 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -11065,6 +11065,14 @@ qemuParseCommandLine(virCapsPtr qemuCaps, goto error; break; + case VIR_STORAGE_NET_PROTOCOL_HTTP: + case VIR_STORAGE_NET_PROTOCOL_HTTPS: + case VIR_STORAGE_NET_PROTOCOL_FTP: + case VIR_STORAGE_NET_PROTOCOL_FTPS: + case VIR_STORAGE_NET_PROTOCOL_TFTP: + case VIR_STORAGE_NET_PROTOCOL_LAST: + /* ignored for now */ + break; } } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 52ca47c..0c91106 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12368,7 +12368,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk) return 0; case VIR_STORAGE_TYPE_NETWORK: - switch ((virStorageNetProtocol) disk->src.protocol) { + switch (disk->src.protocol) { case VIR_STORAGE_NET_PROTOCOL_NBD: case VIR_STORAGE_NET_PROTOCOL_RBD: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: @@ -12430,7 +12430,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d return 0; case VIR_STORAGE_TYPE_NETWORK: - switch ((virStorageNetProtocol) disk->src.protocol) { + switch (disk->src.protocol) { case VIR_STORAGE_NET_PROTOCOL_GLUSTER: return 0; @@ -12575,7 +12575,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn, return 0; case VIR_STORAGE_TYPE_NETWORK: - switch ((virStorageNetProtocol) disk->src.protocol) { + switch (disk->src.protocol) { case VIR_STORAGE_NET_PROTOCOL_NBD: case VIR_STORAGE_NET_PROTOCOL_RBD: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: @@ -12801,7 +12801,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, VIR_STRDUP(persistSource, snap->src.path) < 0) goto cleanup; - switch ((virStorageType)snap->src.type) { + switch (snap->src.type) { case VIR_STORAGE_TYPE_BLOCK: reuse = true; /* fallthrough */ diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index e32389e..0a19603 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -150,7 +150,7 @@ typedef virStorageNetHostDef *virStorageNetHostDefPtr; struct _virStorageNetHostDef { char *name; char *port; - int transport; /* virStorageNetHostTransport */ + virStorageNetHostTransport transport; char *socket; /* path to unix socket */ }; @@ -182,10 +182,10 @@ typedef struct _virStorageSourcePoolDef virStorageSourcePoolDef; struct _virStorageSourcePoolDef { char *pool; /* pool name */ char *volume; /* volume name */ - int voltype; /* virStorageVolType, internal only */ - int pooltype; /* virStoragePoolType, internal only */ - int actualtype; /* virStorageType, internal only */ - int mode; /* virStorageSourcePoolMode */ + virStorageVolType voltype; /* internal only */ + int pooltype; /* virStoragePoolType from storage_conf.h, internal only */ + virStorageType actualtype; /* internal only */ + virStorageSourcePoolMode mode; }; typedef virStorageSourcePoolDef *virStorageSourcePoolDefPtr; @@ -208,15 +208,15 @@ typedef virStorageSource *virStorageSourcePtr; * backing chains, multiple source disks join to form a single guest * view. */ struct _virStorageSource { - int type; /* virStorageType */ + virStorageType type; char *path; - int protocol; /* virStorageNetProtocol */ + virStorageNetProtocol protocol; size_t nhosts; virStorageNetHostDefPtr hosts; virStorageSourcePoolDefPtr srcpool; struct { char *username; - int secretType; /* virStorageSecretType */ + virStorageSecretType secretType; union { unsigned char uuid[VIR_UUID_BUFLEN]; char *usage; @@ -225,7 +225,8 @@ struct _virStorageSource { virStorageEncryptionPtr encryption; char *driverName; - int format; /* virStorageFileFormat */ + int format; /* virStorageFileFormat in domain backing chains, but + * pool-specific enum for storage volumes */ virBitmapPtr features; char *compat; -- 1.9.0
On 05/14/2014 04:45 PM, Eric Blake wrote:
For internal structs, we might as well be type-safe and let the compiler help us with less typing required on our part (getting rid of casts is always nice). In trying to use enums directly, I noticed two problems in virstoragefile.h that can't be fixed without more invasive refactoring: virStorageSource.format is used as more of a union of multiple enums in storage volume code (so it has to remain an int), and virStorageSourcePoolDef refers to pooltype whose enum is declared in src/conf, but where src/util can't pull in headers from src/conf.
I'm probably going to have to revert this patch :( There is a subtle bug here, where the set of machines where the bug surfaces depends on the whims of the compiler. CC conf/libvirt_conf_la-domain_conf.lo conf/domain_conf.c: In function 'virDomainDiskSourceParse': conf/domain_conf.c:4992:9: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] Basically, compilers are allowed to choose whether an enum value with no negative constants behaves as signed or unsigned; if the compiler chooses unsigned, but we then do 'if ((value = virBlahFromString(str)) < 0)', we cause the compiler warning; if the compiler chooses signed, then everything just works. Worse, if we do 'if ((value = virBlahFromString(str)) <= 0)', the compiler silently does the wrong thing (-1 returned from virBlahFromString is promoted to unsigned, so the condition is false, and we are stuck using an out-of-range value without warning). On IRC, I talked about several workarounds that could keep us with an enum type; each looks worse than what this change would cure, which is why I think reverting is the only sane option. 1. Force the comparison in int (yucky because it adds a cast, but my stated intention of this commit was to avoid casts, and requires a code audit): if ((int)(value = virBlahFromString(str)) <= 0) 2. Use temporaries (no ugly casts, but requires a code audit): int tmp; if ((tmp = virBlahFromString(str)) < 0) ... value = tmp; 3. Force the enum to be signed (yucky because we have to inject an otherwise unused -1 element in each enum): typedef enum { VIR_BLAH__FORCE_SIGNED = -1, VIR_BLAH_A, VIR_BLAH_B, VIR_BLAH_LAST } virBlah; 4. Enhance VIR_ENUM_DECL/VIR_ENUM_IMPL to provide yet another conversion function. The existing virBlahFromString(str) returns -1 on failure or enum values on success; the new virBlahEnumFromString(str) returns -1 on failure or 0 on success with the correct value stored into the address. (yucky because you now have to know which of the two FromString variants to call based on what type you are assigning into, because 'int*' and 'enum*' are not compatible types): enum value; int other; if (virBlahEnumFromString(str, &value) < 0) ... if ((other = virBlahFromString(str)) < 0) ,,, 5. Alter VIR_ENUM_DECL/VIR_ENUM_IMPL to make the FromString variant have new semantics - the _LAST value is now hard-coded as the failure value rather than -1, and the return signature is an enum (ugly because it requires a full tree audit, and because checking for ==_LAST is not as nice as checking for < 0; and it gets worse when also excluding 0) if ((value = virBlahFromString(str)) == VIR_BLAH_LAST) ...error about unknown value value = virBlahFromString(str); if (!value || value == VIR_BLAH_LAST) ...error about bad value Any other opinions, or should I just go ahead with the revert? This is breaking the build for some people, depending on their gcc version. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/14/2014 04:45 PM, Eric Blake wrote:
For internal structs, we might as well be type-safe and let the compiler help us with less typing required on our part (getting rid of casts is always nice). In trying to use enums directly, I noticed two problems in virstoragefile.h that can't be fixed without more invasive refactoring: virStorageSource.format is used as more of a union of multiple enums in storage volume code (so it has to remain an int), and virStorageSourcePoolDef refers to pooltype whose enum is declared in src/conf, but where src/util can't pull in headers from src/conf.
I'm probably going to have to revert this patch :(
There is a subtle bug here, where the set of machines where the bug surfaces depends on the whims of the compiler.
CC conf/libvirt_conf_la-domain_conf.lo conf/domain_conf.c: In function 'virDomainDiskSourceParse': conf/domain_conf.c:4992:9: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits]
Basically, compilers are allowed to choose whether an enum value with no negative constants behaves as signed or unsigned; if the compiler chooses unsigned, but we then do 'if ((value = virBlahFromString(str)) < 0)', we cause the compiler warning; if the compiler chooses signed, then everything just works. Worse, if we do 'if ((value = virBlahFromString(str)) <= 0)', the compiler silently does the wrong thing (-1 returned from virBlahFromString is promoted to unsigned, so the condition is false, and we are stuck using an out-of-range value without warning).
On IRC, I talked about several workarounds that could keep us with an enum type; each looks worse than what this change would cure, which is why I think reverting is the only sane option.
1. Force the comparison in int (yucky because it adds a cast, but my stated intention of this commit was to avoid casts, and requires a code audit):
if ((int)(value = virBlahFromString(str)) <= 0)
2. Use temporaries (no ugly casts, but requires a code audit):
int tmp; if ((tmp = virBlahFromString(str)) < 0) ... value = tmp;
I have a patch along these lines to fix the build failures, but agree that it does not sit well with your intentions of this patch.
3. Force the enum to be signed (yucky because we have to inject an otherwise unused -1 element in each enum):
typedef enum { VIR_BLAH__FORCE_SIGNED = -1, VIR_BLAH_A, VIR_BLAH_B, VIR_BLAH_LAST } virBlah;
4. Enhance VIR_ENUM_DECL/VIR_ENUM_IMPL to provide yet another conversion function. The existing virBlahFromString(str) returns -1 on failure or enum values on success; the new virBlahEnumFromString(str) returns -1 on failure or 0 on success with the correct value stored into the address. (yucky because you now have to know which of the two FromString variants to call based on what type you are assigning into, because 'int*' and 'enum*' are not compatible types):
enum value; int other; if (virBlahEnumFromString(str, &value) < 0) ... if ((other = virBlahFromString(str)) < 0) ,,,
5. Alter VIR_ENUM_DECL/VIR_ENUM_IMPL to make the FromString variant have new semantics - the _LAST value is now hard-coded as the failure value rather than -1, and the return signature is an enum (ugly because it requires a full tree audit, and because checking for ==_LAST is not as nice as checking for < 0; and it gets worse when also excluding 0)
if ((value = virBlahFromString(str)) == VIR_BLAH_LAST) ...error about unknown value
value = virBlahFromString(str); if (!value || value == VIR_BLAH_LAST) ...error about bad value
Heh, hard to argue that any of these options _improve_ the code base.
Any other opinions, or should I just go ahead with the revert? This is breaking the build for some people, depending on their gcc version.
I'm torn. This intent of the patch is a nice improvement, but with diminishing returns when adjusting it to work with some versions of gcc and clang. Regards, Jim
On 15.05.2014 00:45, Eric Blake wrote:
Inspired by the cleanups contributed by Julio Faracco, I did some more cleanups of my own. My end goal is to turn on a syntax-check rule that forbids 'enum vir' (since we should always be declaring 'typedef enum { ... } vir...;'), but there's still a lot of violations in the code base, even after this series.
Eric Blake (4): vbox: fix stale comment about vdi storage type maint: use enum typedef for virstorageencryption.h maint: shorten 'TypeType' function names maint: prefer enum over int for virstoragefile structs
src/conf/domain_conf.c | 14 +++++++------- src/conf/domain_conf.h | 2 +- src/conf/secret_conf.c | 8 ++++---- src/conf/secret_conf.h | 4 ++-- src/conf/storage_conf.c | 14 +++++++------- src/conf/storage_conf.h | 6 +++--- src/libvirt_private.syms | 10 +++++----- src/lxc/lxc_controller.c | 2 +- src/qemu/qemu_command.c | 8 ++++++++ src/qemu/qemu_driver.c | 8 ++++---- src/storage/storage_backend_disk.c | 2 +- src/util/virstorageencryption.c | 6 +++--- src/util/virstorageencryption.h | 10 +++++----- src/util/virstoragefile.h | 19 ++++++++++--------- src/vbox/vbox_tmpl.c | 15 ++++----------- tools/virsh-secret.c | 4 ++-- 16 files changed, 67 insertions(+), 65 deletions(-)
ACK to all. Michal
On 05/15/2014 10:37 PM, Michal Privoznik wrote:
On 15.05.2014 00:45, Eric Blake wrote:
Inspired by the cleanups contributed by Julio Faracco, I did some more cleanups of my own. My end goal is to turn on a syntax-check rule that forbids 'enum vir' (since we should always be declaring 'typedef enum { ... } vir...;'), but there's still a lot of violations in the code base, even after this series.
ACK to all.
Thanks; pushed -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Jim Fehlig -
Michal Privoznik