Re: [libvirt] [PATCH v3 2/2] network: Add code for setting network bandwidth for ethernet interfaces
On 10/10/14, 5:27 AM, "Eric Blake" <eblake(a)redhat.com> wrote:
On 10/09/2014 12:21 AM, Anirban Chakraborty wrote:
>> Alas, using a default case means the compiler can no longer tell you
>> about missed cases. I wrote my example to intentionally spell out ALL
>> cases without a default, in order to ensure the compiler will loudly
>> warn if we add another enum in the future.
>
> I understood the intention. However, I was aiming for a compact code. If
> we add additional enums for network type, we should be adding those
>types
> in the above switch provided the new interface type supports bandwidth.
>I
> do not know if we should prevent future coding error by elaborating the
> current code. Forgive me, as I do not know the coding convention for
> libvirt. If this is the standard practice, I¹ll change the above switch
>as
> per your wish.
Hmm - we really ought to add a segment to HACKING that mentions the trick.
The thing to remember is that gcc is able to warn for any enum used as a
switch statement where there are only 'case FOO:' labels and no
'default:' label, and where not all the enum values are covered. So
code-maintenance wise, if you have an enum that might grow a value in
the future, and want to make sure any addition of a new value gets
automatically flagged as to all places of code that need to think about
behavior related to that new value, then using a switch with no default
lets the compiler help you find those places.
Thanks, however, I do not see that in practice. I have searched through
the libvirt code base and I do see many instances of switch statements
where ‘default’ was used and not all the types of the enum are covered as
you suggested in your outlined code. Some examples are given below.
In file qemu/qemu_process.c,
static int qemuProcessGetPCIDiskVendorProduct()
{
switch (def->bus) {
case VIR_DOMAIN_DISK_BUS_VIRTIO:
*vendor = QEMU_PCI_VENDOR_REDHAT;
*product = QEMU_PCI_PRODUCT_DISK_VIRTIO;
break;
default:
return -1;
}
There are many more types of VIR_DOMAIN_DISK_BUS_*. All of those are
covered via default.
Similarly in lxc/lxc_driver.c,
static int
lxcConnectSupportsFeature(virConnectPtr conn, int feature)
{
if (virConnectSupportsFeatureEnsureACL(conn) < 0)
return -1;
switch (feature) {
case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
return 1;
default:
return 0;
}
}
There are a whole lot of other ‘feature’ (total 14 defined), all the rest
are covered via default in the above switch.
The list is really very long to put all in here. If we have to do what you
suggested, then we’d be changing all such usage of switch statements,
which is not trivial, I believe.
Thanks.
Anirban