Re: [libvirt] [PATCH] network: Add bandwidth support to ethernet interface

On 30.10.2014 00:56, Anirban Chakraborty wrote: ><snip> > > +static inline 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: > + break; > + } > + return false; > +} > + I've got a feeling that this should go to src/util/virnetdevbandwidth* among with the rest of virNetDevBandwitdh functions.

> #endif /* __DOMAIN_CONF_H */ > diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c > index 979382b..8a21af4 100644 > --- a/src/lxc/lxc_driver.c > +++ b/src/lxc/lxc_driver.c > @@ -72,6 +72,7 @@ > #include "viraccessapicheck.h" > #include "viraccessapichecklxc.h" > #include "virhostdev.h" > +#include "qemu/qemu_command.h" This is not allowed. In case somebody is building with LXC but without QEMU ..

> > #define VIR_FROM_THIS VIR_FROM_LXC > > @@ -4634,6 +4635,8 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm, > > detach = vm->def->nets[detachidx]; > > + qemuDomainClearNetBandwidth(vm); > + .. this is going to be an undefined reference. > switch (virDomainNetGetActualType(detach)) { > case VIR_DOMAIN_NET_TYPE_BRIDGE: > case VIR_DOMAIN_NET_TYPE_NETWORK: > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c > index ed30c37..3192011 100644 > --- a/src/lxc/lxc_process.c > +++ b/src/lxc/lxc_process.c > @@ -274,11 +274,6 @@ char >*virLXCProcessSetupInterfaceBridged(virConnectPtr conn, > if (virNetDevSetOnline(parentVeth, true) < 0) > goto cleanup; > > - if (virNetDevBandwidthSet(net->ifname, > - virDomainNetGetActualBandwidth(net), > - false) < 0) > - goto cleanup; > - No, this function is called from two places: 1) when domain is starting up 2) on NIC hotplug you are covering 1) but removing already working 2). We can't lose that functionality.

> if (net->filter && > virDomainConfNWFilterInstantiate(conn, vm->uuid, net) < 0) > goto cleanup; > @@ -300,6 +295,7 @@ char >*virLXCProcessSetupInterfaceDirect(virConnectPtr conn, > virNetDevBandwidthPtr bw; > virNetDevVPortProfilePtr prof; > virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); > + const char *linkdev = virDomainNetGetActualDirectDev(net); > > /* XXX how todo bandwidth controls ? > * Since the 'net-ifname' is about to be moved to a different > @@ -329,14 +325,13 @@ char >*virLXCProcessSetupInterfaceDirect(virConnectPtr conn, > > if (virNetDevMacVLanCreateWithVPortProfile( > net->ifname, &net->mac, > - virDomainNetGetActualDirectDev(net), > + linkdev, > virDomainNetGetActualDirectMode(net), > false, def->uuid, > - virDomainNetGetActualVirtPortProfile(net), > + prof, > &res_ifname, > VIR_NETDEV_VPORT_PROFILE_OP_CREATE, > - cfg->stateDir, > - virDomainNetGetActualBandwidth(net), 0) < 0) > + cfg->stateDir, 0) < 0) > goto cleanup; > Same comment applies here.

> ret = res_ifname; > @@ -450,6 +445,11 @@ static int >virLXCProcessSetupInterfaces(virConnectPtr conn, > goto cleanup; > } > > + /* set network bandwidth */ > + if (virNetDevBandwidthSet(def->nets[i]->ifname, > + virDomainNetGetActualBandwidth(def->nets[i]), false) < >0) > + goto cleanup; > + Shouldn't this be guarded with virNetDevSupportBandwidth()? The problem is, there's a switch() just before this where all the unsupported NIC types are rejected (ETHERNET is rejected too btw). What will come through is DIRECT type. I'm not saying that we should not set bandwidth there, but this patch aims at ethernet.

> (*veths)[(*nveths)-1] = veth; > > /* Make sure all net definitions will have a name in the >container */ > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 2e5af4f..7922672 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -191,7 +191,6 @@ qemuPhysIfaceConnect(virDomainDefPtr def, > virDomainNetGetActualVirtPortProfile(net), > &res_ifname, > vmop, cfg->stateDir, > - virDomainNetGetActualBandwidth(net), > macvlan_create_flags); > if (rc >= 0) { > virDomainAuditNetDevice(def, net, res_ifname, true); > @@ -371,11 +370,6 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, > &net->mac) < 0) > goto cleanup; > > - if (virNetDevBandwidthSet(net->ifname, > - virDomainNetGetActualBandwidth(net), > - false) < 0) > - goto cleanup; > - > if (net->filter && > virDomainConfNWFilterInstantiate(conn, def->uuid, net) < 0) { > goto cleanup; > @@ -7427,6 +7421,13 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, > goto cleanup; > } > > + /* Set Bandwidth */ > + if (virNetDevSupportBandwidth(actualType) && > + virNetDevBandwidthSet(net->ifname, > + virDomainNetGetActualBandwidth(net), > + false) < 0) > + goto cleanup; There's no guarantee that net->ifname is filled in here: virsh # start migt10 error: Failed to start domain migt10 error: internal error: Child process (/sbin/tc qdisc add dev) unexpected exit status 255: Command line is not complete. Try option "help"

And I have to mention weird code formatting.

> + > if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK || > actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || > actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || > @@ -12463,3 +12464,15 @@ virDomainDefPtr >qemuParseCommandLinePid(virCapsPtr qemuCaps, > virStringFreeList(progenv); > return def; > } > + > +void qemuDomainClearNetBandwidth(virDomainObjPtr vm) > +{ > + size_t i; > + virDomainNetType type; > + > + for (i = 0; i < vm->def->nnets; i++) { > + type = virDomainNetGetActualType(vm->def->nets[i]); > + if (virNetDevSupportBandwidth(type)) > + virNetDevBandwidthClear(vm->def->nets[i]->ifname); > + } > +} Having this in qemu specific code feels strange, esp. when the code is to be called from other drivers too. How about moving it under src/util/virnetdevbandwidth*?

> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h > index aa40c9e..7963a91 100644 > --- a/src/qemu/qemu_command.h > +++ b/src/qemu/qemu_command.h > @@ -277,4 +277,6 @@ int qemuCheckDiskConfig(virDomainDiskDefPtr disk); > > bool > qemuCheckFips(void); > + > +void qemuDomainClearNetBandwidth(virDomainObjPtr vm); > #endif /* __QEMU_COMMAND_H__*/ > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 2eaf77d..6ef1132 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -2196,6 +2196,9 @@ qemuDomainDestroyFlags(virDomainPtr dom, > if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0) > goto cleanup; > > + /* Clear network bandwidth */ > + qemuDomainClearNetBandwidth(vm); > + This is not needed. qemuProcessStop() will take care of that.

> qemuDomainSetFakeReboot(driver, vm, false); > > ><snip> BTW: it would be nice if you can version you patches. I mean, this is what, 4th or 5th version? Say that in subject explicitly please. You know, in the prefix: [PATCH v5] network: ...