9:16 a.m.
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
10:13 a.m.
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
11:33 a.m.
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
11:34 a.m.
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 :|
12:16 p.m.
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
12:27 p.m.
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
4063
days inactive
4063
days old
5 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa