[libvirt] [PATCH v3 2/2] network: Add code for setting network bandwidth for ethernet interfaces

[Anirban Chakraborty]

On 10/8/14, 2:39 PM, "Eric Blake" <eblake at redhat.com> wrote:

>On 10/08/2014 01:59 PM, Anirban Chakraborty wrote:
>> 
>> Signed-off-by: Anirban Chakraborty <abchak at 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