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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list