[libvirt] [PATCH 4/9] network: save bridge name in ActualNetDef when actualType==network too
John Ferlan
jferlan at redhat.com
Mon Nov 24 22:50:13 UTC 2014
On 11/24/2014 12:48 PM, Laine Stump wrote:
> When the actualType of a virDomainNetDef is "network", it means that
> we are connecting to a libvirt-managed network (routed, natted, or
> isolated) which does use a bridge device (created by libvirt). In the
> past we have required drivers such as qemu to call the public API to
> retrieve the bridge name in this case (even though it is available in
> the NetDef's ActualNetDef if the actualType is "bridge" (i.e., an
> externally-created bridge that isn't managed by libvirt). There is no
> real reason for this difference, and as a matter of fact it
> complicates things for qemu. Also, there is another bridge-related
> attribute (promiscLinks) that will need to be available in both cases,
> so this makes things consistent.
>
> Along with making the bridge name available in the internal object, it
> is also now reported in the <source> element of the <interface> state
> XML (or the <actual> subelement in the internally-stored format).
>
> The one oddity about this change is that usually there is a separate
> union for every different "type" in a higher level object (e.g. in the
> case of a virDomainNetDef there are separate "network" and "bridge"
> members of the union that pivots on the type), but in this case
> network and bridge types both have exactly the same attributes, so the
> "bridge" member is used for both type==network and type==bridge.
> ---
> src/conf/domain_conf.c | 102 +++++++++++++++++++++++---------------------
> src/network/bridge_driver.c | 9 ++++
> 2 files changed, 62 insertions(+), 49 deletions(-)
>
I'm happy someone understands the comings and goings of actual!
Seems reasonable... Is there any concern vis-a-vis migration (or similar
guest state saving activities) with respect to having a <source> element
in the output for actual that wouldn't have been there before? (if I'm
reading the tea leaves correctly - that's what's happening here).
ACK in general - nothing jumps out at me other than the display/saving
of the <source>
John
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 68eef54..932bb1c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1352,6 +1352,7 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def)
>
> switch (def->type) {
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> + case VIR_DOMAIN_NET_TYPE_NETWORK:
> VIR_FREE(def->data.bridge.brname);
> break;
> case VIR_DOMAIN_NET_TYPE_DIRECT:
> @@ -7074,9 +7075,12 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
> goto error;
> }
> VIR_FREE(class_id);
> - } else if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> + }
> + if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> + actual->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> char *brname = virXPathString("string(./source/@bridge)", ctxt);
> - if (!brname) {
> +
> + if (!brname && actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Missing <source> element with bridge name in "
> "interface's <actual> element"));
> @@ -17015,60 +17019,59 @@ virDomainHostdevDefFormatCaps(virBufferPtr buf,
> static int
> virDomainActualNetDefContentsFormat(virBufferPtr buf,
> virDomainNetDefPtr def,
> - const char *typeStr,
> bool inSubelement,
> unsigned int flags)
> {
> - const char *mode;
> -
> - switch (virDomainNetGetActualType(def)) {
> - case VIR_DOMAIN_NET_TYPE_BRIDGE:
> - virBufferEscapeString(buf, "<source bridge='%s'/>\n",
> - virDomainNetGetActualBridgeName(def));
> - break;
> -
> - case VIR_DOMAIN_NET_TYPE_DIRECT:
> - virBufferAddLit(buf, "<source");
> - virBufferEscapeString(buf, " dev='%s'",
> - virDomainNetGetActualDirectDev(def));
> + int actualType = virDomainNetGetActualType(def);
>
> - mode = virNetDevMacVLanModeTypeToString(virDomainNetGetActualDirectMode(def));
> - if (!mode) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("unexpected source mode %d"),
> - virDomainNetGetActualDirectMode(def));
> - return -1;
> - }
> - virBufferAsprintf(buf, " mode='%s'/>\n", mode);
> - break;
> -
> - case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> if (virDomainHostdevDefFormatSubsys(buf, virDomainNetGetActualHostdev(def),
> flags, true) < 0) {
> return -1;
> }
> - break;
> -
> - case VIR_DOMAIN_NET_TYPE_NETWORK:
> - if (!inSubelement) {
> - /* the <source> element isn't included in <actual>
> - * (i.e. when we're putting our output into a subelement
> - * rather than inline) because the main element has the
> - * same info already. If we're outputting inline, though,
> - * we *do* need to output <source>, because the caller
> - * won't have done it.
> + } else {
> + virBufferAddLit(buf, "<source");
> + if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK && !inSubelement) {
> + /* When we're putting our output into the <actual>
> + * subelement rather than the main <interface>, the
> + * network name isn't included in the <source> because the
> + * main interface element's <source> has the same info
> + * already. If we've been called to output directly into
> + * the main element's <source> though (the case here -
> + * "!inSubElement"), we *do* need to output the network
> + * name, because the caller won't have done it).
> */
> - virBufferEscapeString(buf, "<source network='%s'/>\n",
> - def->data.network.name);
> + virBufferEscapeString(buf, " network='%s'", def->data.network.name);
> }
> - if (def->data.network.actual && def->data.network.actual->class_id)
> - virBufferAsprintf(buf, "<class id='%u'/>\n",
> - def->data.network.actual->class_id);
> - break;
> - default:
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("unexpected actual net type %s"), typeStr);
> - return -1;
> + if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> + actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
> + /* actualType == NETWORK includes the name of the bridge
> + * that is used by the network, whether we are
> + * "inSubElement" or not.
> + */
> + virBufferEscapeString(buf, " bridge='%s'",
> + virDomainNetGetActualBridgeName(def));
> + } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
> + const char *mode;
> +
> + virBufferEscapeString(buf, " dev='%s'",
> + virDomainNetGetActualDirectDev(def));
> + mode = virNetDevMacVLanModeTypeToString(virDomainNetGetActualDirectMode(def));
> + if (!mode) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unexpected source mode %d"),
> + virDomainNetGetActualDirectMode(def));
> + return -1;
> + }
> + virBufferAsprintf(buf, " mode='%s'", mode);
> + }
> +
> + virBufferAddLit(buf, "/>\n");
> + }
> + if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT &&
> + def->data.network.actual && def->data.network.actual->class_id) {
> + virBufferAsprintf(buf, "<class id='%u'/>\n",
> + def->data.network.actual->class_id);
> }
>
> if (virNetDevVlanFormat(virDomainNetGetActualVlan(def), buf) < 0)
> @@ -17116,7 +17119,7 @@ virDomainActualNetDefFormat(virBufferPtr buf,
> virBufferAddLit(buf, ">\n");
>
> virBufferAdjustIndent(buf, 2);
> - if (virDomainActualNetDefContentsFormat(buf, def, typeStr, true, flags) < 0)
> + if (virDomainActualNetDefContentsFormat(buf, def, true, flags) < 0)
> return -1;
> virBufferAdjustIndent(buf, -2);
> virBufferAddLit(buf, "</actual>\n");
> @@ -17287,7 +17290,7 @@ virDomainNetDefFormat(virBufferPtr buf,
> * the standard place... (this is for public reporting of
> * interface status)
> */
> - if (virDomainActualNetDefContentsFormat(buf, def, typeStr, false, flags) < 0)
> + if (virDomainActualNetDefContentsFormat(buf, def, false, flags) < 0)
> return -1;
> } else {
> /* ...but if we've asked for the inactive XML (rather than
> @@ -20638,7 +20641,8 @@ virDomainNetGetActualBridgeName(virDomainNetDefPtr iface)
> return iface->data.bridge.brname;
> if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> iface->data.network.actual &&
> - iface->data.network.actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE)
> + (iface->data.network.actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> + iface->data.network.actual->type == VIR_DOMAIN_NET_TYPE_NETWORK))
> return iface->data.network.actual->data.bridge.brname;
> return NULL;
> }
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 6cb421c..92efd7e 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -3771,6 +3771,15 @@ networkAllocateActualDevice(virDomainDefPtr dom,
> */
> iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_NETWORK;
>
> + /* we also store the bridge device
> + * in iface->data.network.actual->data.bridge for later use
> + * after the domain's tap device is created (to attach to the
> + * bridge and set flood/learning mode on the tap device)
> + */
> + if (VIR_STRDUP(iface->data.network.actual->data.bridge.brname,
> + netdef->bridge) < 0)
> + goto error;
> +
> if (networkPlugBandwidth(network, iface) < 0)
> goto error;
>
>
More information about the libvir-list
mailing list