On 11/3/14, 2:58 AM, "Michal Privoznik" <mprivozn@xxxxxxxxxx> wrote:
>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.
I thought about moving this and the qemuDomainClearNetBandwidth() to
src/util/virnetdevbandwidth.h earlier, however, these functions need
reference to virDomainNetType and virDomainObjPtr which are defined in
conf/domain_conf.h and apparently src/util/*.h can not have any reference
to files from conf/.
>
>> #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 ..
Thanks for pointing it out.
>
>>
>> #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.
Got it. Thanks.
>
>> 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.
Thanks.
>
>> 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.
Agreed. Will take care of it next version of the patch.
>
>> (*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"
I will check for empty string here.
>
>And I have to mention weird code formatting.
I am assuming that you are referring to misaligned arguments to function
virNetDevSupportBandwidth() above. They should follow the first char of
the opening parentheses, right?
>
>> +
>> 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*?
As mentioned above, I was not sure if I could put them in the file you
mentioned because virDomaiObjPtr will need reference to conf/domain_conf.h
file and I think that file can not be included in
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.
Ok, thanks.
>
>> 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: ...
I was doing it earlier and then dropped it. I¹ll resin the patch
addressing all your comments and send it out. However, please let me know
if I should move the above functions (virNetDevBandwidthSet etc.) in
src/util/virnetdevbandwidth.* and add #include "conf/domain_conf.h" in
virnetdevbandwidth.h file.
Thanks,
Anirban
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list