[libvirt] [PATCH v2 6/8] bandwidth: Create network bandwidth (un)plug functions

[Laine Stump]

On 12/04/2012 02:19 PM, Michal Privoznik wrote:
> Network should be notified if we plug in or unplug an
> interface, so it can perform some action, e.g. set/unset
> network part of QoS. However, we are doing this in very
> early stage, so iface->ifname isn't filled in yet. So
> whenever we want to report an error, we must use a different
> identifier, e.g. the MAC address.
> ---
>  src/Makefile.am             |    7 +-
>  src/conf/domain_conf.h      |    1 +
>  src/conf/network_conf.c     |   21 ++++
>  src/conf/network_conf.h     |    4 +
>  src/libvirt_network.syms    |   13 +++
>  src/libvirt_private.syms    |    8 --
>  src/network/bridge_driver.c |  223 ++++++++++++++++++++++++++++++++++++++++++-
>  7 files changed, 266 insertions(+), 11 deletions(-)
>  create mode 100644 src/libvirt_network.syms
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 01cb995..04378d1 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1373,6 +1373,10 @@ if WITH_ATOMIC_OPS_PTHREAD
>  USED_SYM_FILES += libvirt_atomic.syms
>  endif
>  
> +if WITH_NETWORK
> +USED_SYM_FILES += libvirt_network.syms
> +endif
> +
>  EXTRA_DIST += \
>    libvirt_public.syms		\
>    libvirt_private.syms		\
> @@ -1386,7 +1390,8 @@ EXTRA_DIST += \
>    libvirt_sasl.syms		\
>    libvirt_vmx.syms		\
>    libvirt_xenxs.syms	\
> -  libvirt_libssh2.syms
> +  libvirt_libssh2.syms	\
> +  libvirt_network.syms
>  
>  GENERATED_SYM_FILES = libvirt.syms libvirt.def libvirt_qemu.def
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 4ab15e9..b4d149b 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -807,6 +807,7 @@ struct _virDomainActualNetDef {
>      virNetDevVPortProfilePtr virtPortProfile;
>      virNetDevBandwidthPtr bandwidth;
>      virNetDevVlan vlan;
> +    unsigned int class_id; /* class ID for bandwidth 'floor' */
>  };

I also just noticed that you add this directly into the actualnetdef
rather than into the virNetDevBandwidth. Did you maybe do this because
you wanted the parser to be able to see the network type before allowing
it? If so, I don't think that's necessary - the actualdef is never
parsed from user config, so if it's there at an inappropriate time, it's
either a program bug, or someone messing with the domain status file. I
figured it would be simplest to pass around if it was part of the
bandwidth object (and logically it fits there).

Also, I think it's appropriate to add the bits for parsing/formatting a
new element in the same patch where it was added to the struct, but
you've delayed it to a separate patch at the end that only does that one
thing. I'm okay with that, since it will still pass make check at each
step, but I think it would make more sense to put that last patch
earlier, and include this data definition with it.