[libvirt] [PATCH v3 2/2] network: Add code for setting network bandwidth for ethernet interfaces
Signed-off-by: Anirban Chakraborty <abchak@juniper.net> --- src/conf/domain_conf.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f03599e..91da1ec 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2852,7 +2852,8 @@ static inline bool virNetDevSupportBandwidth(int type) { return ((type == VIR_DOMAIN_NET_TYPE_BRIDGE || type == VIR_DOMAIN_NET_TYPE_NETWORK || - type == VIR_DOMAIN_NET_TYPE_DIRECT) ? true : false); + type == VIR_DOMAIN_NET_TYPE_DIRECT || + type == VIR_DOMAIN_NET_TYPE_ETHERNET) ? true : false); }; #endif /* __DOMAIN_CONF_H */ -- 1.9.1
On 10/08/2014 01:59 PM, Anirban Chakraborty wrote:
Signed-off-by: Anirban Chakraborty <abchak@juniper.net> --- src/conf/domain_conf.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f03599e..91da1ec 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2852,7 +2852,8 @@ static inline bool virNetDevSupportBandwidth(int type) { return ((type == VIR_DOMAIN_NET_TYPE_BRIDGE || type == VIR_DOMAIN_NET_TYPE_NETWORK || - type == VIR_DOMAIN_NET_TYPE_DIRECT) ? true : false); + type == VIR_DOMAIN_NET_TYPE_DIRECT || + type == VIR_DOMAIN_NET_TYPE_ETHERNET) ? true : false);
Needs rework after my comments on 1/2. I also wonder if this should just be folded in to that patch, and/or made into a switch statement where the compiler forces us to think about any future VIR_DOMAIN_NET_TYPE_* additions on whether they should return true or false. As in: bool virNetDevSupportBandwidth(virDomainNetType type) { switch (type) { case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_ETHERNET: return true; case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_VHOSTUSER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: case VIR_DOMAIN_NET_TYPE_MCAST: case VIR_DOMAIN_NET_TYPE_INTERNAL: case VIR_DOMAIN_NET_TYPE_HOSTDEV: case VIR_DOMAIN_NET_TYPE_LAST: /* cover all enums to appease the compiler */ ; } return false; } -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/8/14, 2:39 PM, "Eric Blake" <eblake@redhat.com> wrote:
On 10/08/2014 01:59 PM, Anirban Chakraborty wrote:
Signed-off-by: Anirban Chakraborty <abchak@juniper.net> --- src/conf/domain_conf.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f03599e..91da1ec 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2852,7 +2852,8 @@ static inline bool virNetDevSupportBandwidth(int type) { return ((type == VIR_DOMAIN_NET_TYPE_BRIDGE || type == VIR_DOMAIN_NET_TYPE_NETWORK || - type == VIR_DOMAIN_NET_TYPE_DIRECT) ? true : false); + type == VIR_DOMAIN_NET_TYPE_DIRECT || + type == VIR_DOMAIN_NET_TYPE_ETHERNET) ? true : false);
Needs rework after my comments on 1/2. I also wonder if this should just be folded in to that patch, and/or made into a switch statement where the compiler forces us to think about any future VIR_DOMAIN_NET_TYPE_* additions on whether they should return true or false.
I will rework the 1/2 patch to fix syntax etc. Initially, I sent a single patch to keep everything together, however, there was a suggestion to split it. I decided to split it into two as clearly there are two parts to this series, one to prepare the code for addition of ethernet interfaces and the other to add the ethernet type. True, the second patch is very minimal and I can fold it back into a single patch if everyone agrees to it.
As in:
bool virNetDevSupportBandwidth(virDomainNetType type) { switch (type) { case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_ETHERNET: return true; case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_VHOSTUSER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: case VIR_DOMAIN_NET_TYPE_MCAST: case VIR_DOMAIN_NET_TYPE_INTERNAL: case VIR_DOMAIN_NET_TYPE_HOSTDEV: case VIR_DOMAIN_NET_TYPE_LAST: /* cover all enums to appease the compiler */ ; } return false; }
We could have the function defined with a switch, however, instead of listing all the types, I could return false from the default case. Something like: switch (type) { case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_ETHERNET: return true; default; return false; } Will this work for you? Thanks. -Anirban
On 10/08/2014 04:10 PM, Anirban Chakraborty wrote:
Needs rework after my comments on 1/2. I also wonder if this should just be folded in to that patch, and/or made into a switch statement where the compiler forces us to think about any future VIR_DOMAIN_NET_TYPE_* additions on whether they should return true or false.
bool virNetDevSupportBandwidth(virDomainNetType type) { switch (type) { case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_ETHERNET: return true; case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_VHOSTUSER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: case VIR_DOMAIN_NET_TYPE_MCAST: case VIR_DOMAIN_NET_TYPE_INTERNAL: case VIR_DOMAIN_NET_TYPE_HOSTDEV: case VIR_DOMAIN_NET_TYPE_LAST: /* cover all enums to appease the compiler */ ; } return false; }
We could have the function defined with a switch, however, instead of listing all the types, I could return false from the default case.
Something like: switch (type) { case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_ETHERNET: return true;
default; return false;
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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/8/14, 8:39 PM, "Eric Blake" <eblake@redhat.com> wrote:
On 10/08/2014 04:10 PM, Anirban Chakraborty wrote:
Needs rework after my comments on 1/2. I also wonder if this should just be folded in to that patch, and/or made into a switch statement where the compiler forces us to think about any future VIR_DOMAIN_NET_TYPE_* additions on whether they should return true or false.
bool virNetDevSupportBandwidth(virDomainNetType type) { switch (type) { case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_ETHERNET: return true; case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_VHOSTUSER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: case VIR_DOMAIN_NET_TYPE_MCAST: case VIR_DOMAIN_NET_TYPE_INTERNAL: case VIR_DOMAIN_NET_TYPE_HOSTDEV: case VIR_DOMAIN_NET_TYPE_LAST: /* cover all enums to appease the compiler */ ; } return false; }
We could have the function defined with a switch, however, instead of listing all the types, I could return false from the default case.
Something like: switch (type) { case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_DIRECT: case VIR_DOMAIN_NET_TYPE_ETHERNET: return true;
default; return false;
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. Anirban
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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/10/14, 5:27 AM, "Eric Blake" <eblake@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
On 10/10/2014 10:59 AM, Anirban Chakraborty wrote:
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.
The point of adding it to HACKING is to encourage new code to abide by the standard, and not necessarily to retrofit existing code. And you are correct that existing code doesn't always use enum type-safety compiler guarantees - which makes the enum that much harder to modify later if we ever add enum values. But in some cases, it is fairly obvious that we don't plan to add any enum values, in which case a simple if statement or use of a default label is fine. It's a case-by-case judgment call of what makes the code easier to maintain. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/10/14, 11:13 AM, "Eric Blake" <eblake@redhat.com> wrote:
On 10/10/2014 10:59 AM, Anirban Chakraborty wrote:
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.
The point of adding it to HACKING is to encourage new code to abide by the standard, and not necessarily to retrofit existing code. And you are correct that existing code doesn't always use enum type-safety compiler guarantees - which makes the enum that much harder to modify later if we ever add enum values. But in some cases, it is fairly obvious that we don't plan to add any enum values, in which case a simple if statement or use of a default label is fine. It's a case-by-case judgment call of what makes the code easier to maintain.
So, you are proposing not to touch the existing code but do it for all new code addition. I do not have any problem with that, except for the fact that this would bloat new code, which is equally important, IMHO. Should we bloat code to future proof coding error? If other folks think yes we should (in the context of this code), then I¹d be more than happy to swap the Œdefault¹ case with rest of the types of virDomainNetType. Anirban
On 10/10/14, 11:42 AM, "Anirban Chakraborty" <abchak@juniper.net> wrote:
On 10/10/14, 11:13 AM, "Eric Blake" <eblake@redhat.com> wrote:
On 10/10/2014 10:59 AM, Anirban Chakraborty wrote:
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.
The point of adding it to HACKING is to encourage new code to abide by the standard, and not necessarily to retrofit existing code. And you are correct that existing code doesn't always use enum type-safety compiler guarantees - which makes the enum that much harder to modify later if we ever add enum values. But in some cases, it is fairly obvious that we don't plan to add any enum values, in which case a simple if statement or use of a default label is fine. It's a case-by-case judgment call of what makes the code easier to maintain.
So, you are proposing not to touch the existing code but do it for all new code addition. I do not have any problem with that, except for the fact that this would bloat new code, which is equally important, IMHO. Should we bloat code to future proof coding error? If other folks think yes we should (in the context of this code), then I¹d be more than happy to swap the Œdefault¹ case with rest of the types of virDomainNetType.
Anirban
Never mind, I’ll change the patch to include what you suggested. It has come to a point that there is little technical value on either side of the argument. Anirban
participants (2)
-
Anirban Chakraborty -
Eric Blake