[libvirt] [PATCH] virsh-volume: Unify strigification of volume type
There were two separate places with that were stringifying type of a volume. One of the places was out of sync with types implemented upstream. To avoid such problems in the future, this patch adds a common function to convert the type to string and reuses it across the two said places. --- tools/virsh-volume.c | 59 ++++++++++++++++++++++++---------------------------- 1 file changed, 27 insertions(+), 32 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index d85ae92..604ada5 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -942,6 +942,29 @@ out: return ret; } + +static const char * +vshVolumeTypeToString(int type) +{ + switch (type) { + case VIR_STORAGE_VOL_FILE: + return N_("file"); + + case VIR_STORAGE_VOL_BLOCK: + return N_("block"); + + case VIR_STORAGE_VOL_DIR: + return N_("dir"); + + case VIR_STORAGE_VOL_NETWORK: + return N_("network"); + + default: + return N_("unknown"); + } +} + + /* * "vol-info" command */ @@ -983,26 +1006,9 @@ cmdVolInfo(vshControl *ctl, const vshCmd *cmd) if (virStorageVolGetInfo(vol, &info) == 0) { double val; const char *unit; - switch (info.type) { - case VIR_STORAGE_VOL_FILE: - vshPrint(ctl, "%-15s %s\n", _("Type:"), _("file")); - break; - - case VIR_STORAGE_VOL_BLOCK: - vshPrint(ctl, "%-15s %s\n", _("Type:"), _("block")); - break; - case VIR_STORAGE_VOL_DIR: - vshPrint(ctl, "%-15s %s\n", _("Type:"), _("dir")); - break; - - case VIR_STORAGE_VOL_NETWORK: - vshPrint(ctl, "%-15s %s\n", _("Type:"), _("network")); - break; - - default: - vshPrint(ctl, "%-15s %s\n", _("Type:"), _("unknown")); - } + vshPrint(ctl, "%-15s %s\n", _("Type:"), + _(vshVolumeTypeToString(info.type))); val = vshPrettyCapacity(info.capacity, &unit); vshPrint(ctl, "%-15s %2.2lf %s\n", _("Capacity:"), val, unit); @@ -1377,19 +1383,8 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) /* Convert the returned volume info into output strings */ /* Volume type */ - switch (volumeInfo.type) { - case VIR_STORAGE_VOL_FILE: - volInfoTexts[i].type = vshStrdup(ctl, _("file")); - break; - case VIR_STORAGE_VOL_BLOCK: - volInfoTexts[i].type = vshStrdup(ctl, _("block")); - break; - case VIR_STORAGE_VOL_DIR: - volInfoTexts[i].type = vshStrdup(ctl, _("dir")); - break; - default: - volInfoTexts[i].type = vshStrdup(ctl, _("unknown")); - } + volInfoTexts[i].type = vshStrdup(ctl, + _(vshVolumeTypeToString(volumeInfo.type))); /* Create the capacity output string */ val = vshPrettyCapacity(volumeInfo.capacity, &unit); -- 1.8.4.3
On 11/12/2013 08:16 AM, Peter Krempa wrote:
There were two separate places with that were stringifying type of a volume. One of the places was out of sync with types implemented upstream.
To avoid such problems in the future, this patch adds a common function to convert the type to string and reuses it across the two said places. --- tools/virsh-volume.c | 59 ++++++++++++++++++++++++---------------------------- 1 file changed, 27 insertions(+), 32 deletions(-)
ACK with one nit:
+static const char * +vshVolumeTypeToString(int type) +{ + switch (type) {
Please make this "switch ((virStorageVolType) type)"
+ case VIR_STORAGE_VOL_FILE: + return N_("file"); + + case VIR_STORAGE_VOL_BLOCK: + return N_("block"); + + case VIR_STORAGE_VOL_DIR: + return N_("dir"); + + case VIR_STORAGE_VOL_NETWORK: + return N_("network"); + + default: + return N_("unknown"); + }
drop the default: case, and instead use: VIR_STORAGE_VOL_LAST: break; } return N_("unknown"); which will then let the compiler enforce us to expand the list if we ever add another type. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/12/13 17:13, Eric Blake wrote:
On 11/12/2013 08:16 AM, Peter Krempa wrote:
There were two separate places with that were stringifying type of a volume. One of the places was out of sync with types implemented upstream.
To avoid such problems in the future, this patch adds a common function to convert the type to string and reuses it across the two said places. --- tools/virsh-volume.c | 59 ++++++++++++++++++++++++---------------------------- 1 file changed, 27 insertions(+), 32 deletions(-)
ACK with one nit:
+static const char * +vshVolumeTypeToString(int type) +{ + switch (type) {
Please make this "switch ((virStorageVolType) type)"
I was considering this, but in the VIR_STORAGE_VOL_LAST value defined virStorageVolType enum is protected by an ifdef: typedef enum { VIR_STORAGE_VOL_FILE = 0, /* Regular file based volumes */ VIR_STORAGE_VOL_BLOCK = 1, /* Block based volumes */ VIR_STORAGE_VOL_DIR = 2, /* Directory-passthrough based volume */ VIR_STORAGE_VOL_NETWORK = 3,/* Network volumes like RBD (RADOS Block Device) */ #ifdef VIR_ENUM_SENTINELS VIR_STORAGE_VOL_LAST <- here #endif } virStorageVolType; As I don't know what are the conditions that make VIR_ENUM_SENTINELS defined I didn't want to risk a broken build.
+ case VIR_STORAGE_VOL_FILE: + return N_("file"); + + case VIR_STORAGE_VOL_BLOCK: + return N_("block"); + + case VIR_STORAGE_VOL_DIR: + return N_("dir"); + + case VIR_STORAGE_VOL_NETWORK: + return N_("network"); + + default: + return N_("unknown"); + }
drop the default: case, and instead use:
VIR_STORAGE_VOL_LAST: break; } return N_("unknown");
which will then let the compiler enforce us to expand the list if we ever add another type.
Peter
On Tue, Nov 12, 2013 at 06:33:04PM +0100, Peter Krempa wrote:
On 11/12/13 17:13, Eric Blake wrote:
On 11/12/2013 08:16 AM, Peter Krempa wrote:
There were two separate places with that were stringifying type of a volume. One of the places was out of sync with types implemented upstream.
To avoid such problems in the future, this patch adds a common function to convert the type to string and reuses it across the two said places. --- tools/virsh-volume.c | 59 ++++++++++++++++++++++++---------------------------- 1 file changed, 27 insertions(+), 32 deletions(-)
ACK with one nit:
+static const char * +vshVolumeTypeToString(int type) +{ + switch (type) {
Please make this "switch ((virStorageVolType) type)"
I was considering this, but in the VIR_STORAGE_VOL_LAST value defined virStorageVolType enum is protected by an ifdef:
typedef enum { VIR_STORAGE_VOL_FILE = 0, /* Regular file based volumes */ VIR_STORAGE_VOL_BLOCK = 1, /* Block based volumes */ VIR_STORAGE_VOL_DIR = 2, /* Directory-passthrough based volume */ VIR_STORAGE_VOL_NETWORK = 3,/* Network volumes like RBD (RADOS Block Device) */
#ifdef VIR_ENUM_SENTINELS VIR_STORAGE_VOL_LAST <- here #endif } virStorageVolType;
As I don't know what are the conditions that make VIR_ENUM_SENTINELS defined I didn't want to risk a broken build.
Applications are expected to #define VIR_ENUM_SENTINELS themselves before including libvirt.h, once they accept that the _LAST values are subject to change between releases. /* General note - throughout this file, any linear enumeration which * might be expanded in the future has an optional *_LAST value that * gives the size of the enum at the time of compilation, if the user * defines VIR_ENUM_SENTINELS. Enumerations for bit values do not * have a *_LAST value, but additional bits may be defined. */ 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 :|
On 11/12/2013 10:33 AM, Peter Krempa wrote:
Please make this "switch ((virStorageVolType) type)"
I was considering this, but in the VIR_STORAGE_VOL_LAST value defined virStorageVolType enum is protected by an ifdef:
As I don't know what are the conditions that make VIR_ENUM_SENTINELS defined I didn't want to risk a broken build.
We define it internally for all our code except for the python bindings, precisely because it is useful for our internal code. A quick 'git grep _LAST tools' will show that you can safely rely on the *_LAST sentinels always being present from within virsh code (that is, we'd have to change a lot of code if we ever decided not to expose it during internal compilation). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/12/13 19:16, Eric Blake wrote:
On 11/12/2013 10:33 AM, Peter Krempa wrote:
Please make this "switch ((virStorageVolType) type)"
I was considering this, but in the VIR_STORAGE_VOL_LAST value defined virStorageVolType enum is protected by an ifdef:
As I don't know what are the conditions that make VIR_ENUM_SENTINELS defined I didn't want to risk a broken build.
We define it internally for all our code except for the python bindings, precisely because it is useful for our internal code. A quick 'git grep _LAST tools' will show that you can safely rely on the *_LAST sentinels always being present from within virsh code (that is, we'd have to change a lot of code if we ever decided not to expose it during internal compilation).
Okay, I've changed the patch according to your suggestion and pushed. Thanks for the insight. Peter
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa