[libvirt] [PATCH] bandwidth: Integrate bandwidth into portgroups
Laine Stump
laine at laine.org
Tue Jul 26 18:04:40 UTC 2011
On 07/26/2011 10:35 AM, Michal Privoznik wrote:
> Every DomainNetDef has a bandwidth, as does every portgroup.
> Whenever a DomainNetDef of type NETWORK is about to be used, a call is
> made to networkAllocateActualDevice(). This function chooses the "best"
> bandwidth object and places it in the DomainActualNetDef.
> > From that point on, whenever some code needs to use the bandwidth data
> for the interface, it's retrieved with virDomainNetGetActualBandwidth(),
> which will always return the "best" info as determined in the
> previous step.
> ---
> docs/formatnetwork.html.in | 2 +
> src/conf/domain_conf.c | 25 ++++++++++++++++++-
> src/conf/domain_conf.h | 3 ++
> src/conf/network_conf.c | 10 +++++++
> src/conf/network_conf.h | 1 +
> src/libvirt_private.syms | 1 +
> src/network/bridge_driver.c | 19 +++++++++++++-
> src/util/network.c | 56 +++++++++++++++++++++++++++++++++++++++++++
> src/util/network.h | 2 +
> 9 files changed, 116 insertions(+), 3 deletions(-)
You missed the two most important changes :-) The bandwidth pointer sent
to virBandwidthEnable needs to use virDomainNetGetActualBandwidth()
rather than referencing the def->bandwidth directly.
> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
> index f0ff703..ddfa77c 100644
> --- a/docs/formatnetwork.html.in
> +++ b/docs/formatnetwork.html.in
> @@ -129,6 +129,8 @@
> numbers, The units for<code>average</code> and<code>peak</code> attributes
> are kilobytes per second, and for the<code>burst</code> just kilobytes.
> The rate is shared equally within domains connected to the network.
> + Moreover,<code>bandwidth</code> element can be included in
> +<code>portgroup</code> element.
> <span class="since">Since 0.9.4</span>
> </p>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 072c970..031862a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -753,6 +753,8 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def)
> break;
> }
>
> + virBandwidthDefFree(def->bandwidth);
> +
> VIR_FREE(def);
> }
>
> @@ -2621,6 +2623,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
> virDomainActualNetDefPtr actual = NULL;
> int ret = -1;
> xmlNodePtr save_ctxt = ctxt->node;
> + xmlNodePtr bandwidth_node = NULL;
> char *type = NULL;
> char *mode = NULL;
>
> @@ -2677,6 +2680,11 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
> }
> }
>
> + bandwidth_node = virXPathNode("./bandwidth", ctxt);
> + if (bandwidth_node&&
> + !(actual->bandwidth = virBandwidthDefParseNode(bandwidth_node)))
> + goto error;
> +
> *def = actual;
> actual = NULL;
> ret = 0;
> @@ -8713,6 +8721,10 @@ virDomainActualNetDefFormat(virBufferPtr buf,
> default:
> break;
> }
> +
> + if (virBandwidthDefFormat(buf, def->bandwidth, " ")< 0)
> + goto error;
> +
> virBufferAddLit(buf, "</actual>\n");
>
> ret = 0;
> @@ -8855,7 +8867,8 @@ virDomainNetDefFormat(virBufferPtr buf,
> virBufferAddLit(buf, "</tune>\n");
> }
>
> - if (virBandwidthDefFormat(buf, def->bandwidth, " ")< 0)
> + if (virBandwidthDefFormat(buf, virDomainNetGetActualBandwidth(def),
> + " ")< 0)
This is the one case where we want to specifically use def->bandwidth
rather than virDomainNetGetActualBandwidth. Otherwise you end up writing
the portgroup's bandwidth settings into the interface directly; then the
next time you change the settings in the portgroup, they don't take
effect in the interface because it now has its own private bandwidth
settings, which take precedence over those in the portgroup.
> return -1;
>
> if (virDomainDeviceInfoFormat(buf,&def->info, flags)< 0)
> @@ -11383,3 +11396,13 @@ virDomainNetGetActualDirectVirtPortProfile(virDomainNetDefPtr iface)
> return NULL;
> return iface->data.network.actual->data.direct.virtPortProfile;
> }
> +
> +virBandwidthPtr
> +virDomainNetGetActualBandwidth(virDomainNetDefPtr iface)
> +{
> + if ((iface->type == VIR_DOMAIN_NET_TYPE_NETWORK)&&
> + iface->data.network.actual&& iface->data.network.actual->bandwidth) {
> + return iface->data.network.actual->bandwidth;
> + }
> + return iface->bandwidth;
> +}
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 212f0c5..715d995 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -362,6 +362,7 @@ struct _virDomainActualNetDef {
> virVirtualPortProfileParamsPtr virtPortProfile;
> } direct;
> } data;
> + virBandwidthPtr bandwidth;
> };
>
> /* Stores the virtual network interface configuration */
> @@ -1490,6 +1491,8 @@ char *virDomainNetGetActualDirectDev(virDomainNetDefPtr iface);
> int virDomainNetGetActualDirectMode(virDomainNetDefPtr iface);
> virVirtualPortProfileParamsPtr
> virDomainNetGetActualDirectVirtPortProfile(virDomainNetDefPtr iface);
> +virBandwidthPtr
> +virDomainNetGetActualBandwidth(virDomainNetDefPtr iface);
>
> int virDomainControllerInsert(virDomainDefPtr def,
> virDomainControllerDefPtr controller);
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 1ef80dc..6714c20 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -92,6 +92,8 @@ virPortGroupDefClear(virPortGroupDefPtr def)
> {
> VIR_FREE(def->name);
> VIR_FREE(def->virtPortProfile);
> + virBandwidthDefFree(def->bandwidth);
> + def->bandwidth = NULL;
> }
>
> static void
> @@ -771,6 +773,7 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def,
>
> xmlNodePtr save;
> xmlNodePtr virtPortNode;
> + xmlNodePtr bandwidth_node;
> char *isDefault = NULL;
>
> int result = -1;
> @@ -790,6 +793,12 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def,
> goto error;
> }
>
> + bandwidth_node = virXPathNode("./bandwidth", ctxt);
> + if (bandwidth_node&&
> + !(def->bandwidth = virBandwidthDefParseNode(bandwidth_node))) {
> + goto error;
> + }
> +
> result = 0;
> error:
> if (result< 0) {
> @@ -1257,6 +1266,7 @@ virPortGroupDefFormat(virBufferPtr buf,
> }
> virBufferAddLit(buf, ">\n");
> virVirtualPortProfileFormat(buf, def->virtPortProfile, " ");
> + virBandwidthDefFormat(buf, def->bandwidth, " ");
> virBufferAddLit(buf, "</portgroup>\n");
> }
>
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 6d0845e..869085e 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -123,6 +123,7 @@ struct _virPortGroupDef {
> char *name;
> bool isDefault;
> virVirtualPortProfileParamsPtr virtPortProfile;
> + virBandwidthPtr bandwidth;
> };
>
> typedef struct _virNetworkDef virNetworkDef;
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 853ee62..0fa26dd 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -710,6 +710,7 @@ nlComm;
>
>
> # network.h
> +virBandwidthCopy;
> virBandwidthDefFormat;
> virBandwidthDefFree;
> virBandwidthDefParseNode;
You also need to add virDomainNetGetActualBandwidth (since it will be
called from qemu_command.c - see below).
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index b1c6b12..b8c6c97 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2811,6 +2811,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> (netdef->forwardType == VIR_NETWORK_FORWARD_VEPA) ||
> (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH)) {
> virVirtualPortProfileParamsPtr virtport = NULL;
> + virPortGroupDefPtr portgroup = NULL;
>
> /*<forward type='bridge|private|vepa|passthrough'> are all
> * VIR_DOMAIN_NET_TYPE_DIRECT.
> @@ -2839,11 +2840,10 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> }
>
> /* Find the most specific virtportprofile and copy it */
> + portgroup = virPortGroupFindByName(netdef, iface->data.network.portgroup);
> if (iface->data.network.virtPortProfile) {
> virtport = iface->data.network.virtPortProfile;
> } else {
> - virPortGroupDefPtr portgroup
> - = virPortGroupFindByName(netdef, iface->data.network.portgroup);
> if (portgroup)
> virtport = portgroup->virtPortProfile;
> else
> @@ -2859,6 +2859,21 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
> */
> *iface->data.network.actual->data.direct.virtPortProfile = *virtport;
> }
> +
> + /* Find the most specific bandwidth config and copy it */
> + if (iface->bandwidth) {
> + if (virBandwidthCopy(&iface->data.network.actual->bandwidth,
> + iface->bandwidth)< 0) {
> + goto cleanup;
> + }
> + } else {
> + if (portgroup&&
> + virBandwidthCopy(&iface->data.network.actual->bandwidth,
> + portgroup->bandwidth)< 0) {
> + goto cleanup;
> + }
> + }
> +
> /* If there is only a single device, just return it (caller will detect
> * any error if exclusive use is required but could not be acquired).
> */
> diff --git a/src/util/network.c b/src/util/network.c
> index 5561012..314cabe 100644
> --- a/src/util/network.c
> +++ b/src/util/network.c
> @@ -1263,3 +1263,59 @@ cleanup:
> virCommandFree(cmd);
> return ret;
> }
> +
> +/*
> + * virBandwidthCopy:
> + * @dest: destination
> + * @src: source
> + *
> + * Returns -1 on OOM error (which gets reported),
> + * 0 otherwise.
> + */
> +int
> +virBandwidthCopy(virBandwidthPtr *dest,
> + const virBandwidthPtr src)
> +{
> + int ret = -1;
> +
> + if (!dest) {
> + virSocketError(VIR_ERR_INVALID_ARG, "%s",
> + _("invalid argument supplied"));
> + return -1;
> + }
> +
> + if (!src) {
> + /* nothing to be copied */
Just in case someone calls virBandwidthCopy with a pointer to an
uninitialized dest, we should set *dest = NULL here.
> + return 0;
> + }
> +
> + if (VIR_ALLOC(*dest)< 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if (src->in) {
> + if (VIR_ALLOC((*dest)->in)< 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + memcpy((*dest)->in, src->in, sizeof(*src->in));
> + }
> +
> + if (src->out) {
> + if (VIR_ALLOC((*dest)->out)< 0) {
In the case that the alloc of dest->in is successful and dest->out
fails, you would leak the dest->in.
> + virReportOOMError();
> + goto cleanup;
> + }
> + memcpy((*dest)->out, src->out, sizeof(*src->out));
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + if (ret< 0) {
> + virBandwidthDefFree(*dest);
> + *dest = NULL;
> + }
> + return ret;
> +}
> diff --git a/src/util/network.h b/src/util/network.h
> index 139f6cc..6ceaa6d 100644
> --- a/src/util/network.h
> +++ b/src/util/network.h
> @@ -158,4 +158,6 @@ int virBandwidthDefFormat(virBufferPtr buf,
>
> int virBandwidthEnable(virBandwidthPtr bandwidth, const char *iface);
> int virBandwidthDisable(const char *iface, bool may_fail);
> +int virBandwidthCopy(virBandwidthPtr *dest, const virBandwidthPtr src);
> +
> #endif /* __VIR_NETWORK_H__ */
ACK with the following patch (which addresses the 4 issues I noted
above) squashed in:
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: bandwidth-portgroup-squash.patch
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110726/7043f7d2/attachment-0001.ksh>
More information about the libvir-list
mailing list