On 21.02.2014 14:58, Laine Stump wrote:

In practice, if a virDomainNetDef has a virDomainActualNetDef allocated, the ActualNetDef will *always* contain the bandwidth and vlan data from the NetDef (unless there was also a portgroup involved - see networkAllocateActualDevice()).

However, virDomainNetGetActual(Bandwidth|Vlan)() were coded to make it appear as if it might be possible to have a valid bandwidth/vlan in the NetDef, but a NULL in the ActualNetDef. Believing this un-truth could lead to writing unnecessarily defensive code when dealing with the virDomainGetActual*() functions, so this patch makes it more obvious:

If there is an ActualNetDef, it will always have a copy of the various appropriate bits from its parent NetDef, and the virDomainGetActual* function will *always* return the data from the ActualNetDef, not from the NetDef.

The reason for this effective-NOP patch is that a subsequent patch to change virDomainNetDefFormat will rely on the above rule. --- src/conf/domain_conf.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 064a40e..755066c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18638,8 +18638,11 @@ virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface) virNetDevBandwidthPtr virDomainNetGetActualBandwidth(virDomainNetDefPtr iface) { + /* if there is an ActualNetDef, *always* return + * its bandwidth rather than the NetDef's bandwidth. + */ if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK && - iface->data.network.actual && iface->data.network.actual->bandwidth) { + iface->data.network.actual) { return iface->data.network.actual->bandwidth; } return iface->bandwidth; @@ -18648,12 +18651,17 @@ virDomainNetGetActualBandwidth(virDomainNetDefPtr iface) virNetDevVlanPtr virDomainNetGetActualVlan(virDomainNetDefPtr iface) { + virNetDevVlanPtr vlan = &iface->vlan; + + /* if there is an ActualNetDef, *always* return + * its vlan rather than the NetDef's vlan. + */ if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK && - iface->data.network.actual && - iface->data.network.actual->vlan.nTags > 0) - return &iface->data.network.actual->vlan; - if (iface->vlan.nTags > 0) - return &iface->vlan; + iface->data.network.actual) + vlan = &iface->data.network.actual->vlan; + + if (vlan->nTags > 0) + return vlan; return 0;

When you're at this, can you s/0/NULL/? We are returning a pointer after all.

}

ACK Michal

On 21.02.2014 14:58, Laine Stump wrote:

Other *Format() functions (e.g. virNetDevBandwidthFormat()) return with no action when called with a NULL *Def pointer. This makes virNetDevVlanFormat() consistent with that behavior. --- src/conf/netdev_vlan_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c index f58b4b8..dbe203e 100644 --- a/src/conf/netdev_vlan_conf.c +++ b/src/conf/netdev_vlan_conf.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2013 Red Hat, Inc. + * Copyright (C) 2009-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -144,7 +144,7 @@ virNetDevVlanFormat(const virNetDevVlan *def, virBufferPtr buf) { size_t i;

- if (def->nTags == 0) + if (!(def && def->nTags)) return 0;

if (!def->tag) {

ACK Michal

On 21.02.2014 14:58, Laine Stump wrote:

We will need to call virDomainNetDefFormat() from the network hook (in the network driver). --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 755066c..b5b70cd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15504,7 +15504,7 @@ virDomainActualNetDefFormat(virBufferPtr buf, return 0; }

-static int +int virDomainNetDefFormat(virBufferPtr buf, virDomainNetDefPtr def, unsigned int flags) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 97d6337..2467f65 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2402,6 +2402,10 @@ int virDomainDiskSourceDefFormatInternal(virBufferPtr buf, virDomainDiskSourcePoolDefPtr srcpool, unsigned int flags);

+int virDomainNetDefFormat(virBufferPtr buf, + virDomainNetDefPtr def, + unsigned int flags); + int virDomainDefCompatibleDevice(virDomainDefPtr def, virDomainDeviceDefPtr dev);

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0896287..bb7b6da 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -300,6 +300,7 @@ virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; virDomainMemDumpTypeFromString; virDomainMemDumpTypeToString; +virDomainNetDefFormat; virDomainNetDefFree; virDomainNetFind; virDomainNetFindIdx;

ACK Michal

On 21.02.2014 14:58, Laine Stump wrote:

This moves the call to virNetDevBandwidthFormat() in virDomainNetDefFormat() to be called right after the call to virNetDevVPortProfileFormat(), so that a single chunk of that function can be placed inside an if that conditionally calls virDomainActualNetDefContentsFormat() instead (next patch). The re-ordering necessitates modifying a couple of test data files. --- src/conf/domain_conf.c | 6 +++--- .../qemuhotplug-console-compat-2+console-virtio.xml | 4 ++-- tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml | 4 ++-- tests/qemuxml2argvdata/qemuxml2argv-net-bandwidth.xml | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b5b70cd..67fc372 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15599,6 +15599,9 @@ virDomainNetDefFormat(virBufferPtr buf, return -1; if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0) return -1; + if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0) + return -1; + virBufferEscapeString(buf, "<script path='%s'/>\n", def->script); if (def->ifname && @@ -15651,9 +15654,6 @@ virDomainNetDefFormat(virBufferPtr buf, virDomainNetInterfaceLinkStateTypeToString(def->linkstate)); }

- if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0) - return -1; - virBufferAdjustIndent(buf, -6);

if (virDomainDeviceInfoFormat(buf, &def->info, diff --git a/tests/qemuhotplugtestdata/qemuhotplug-console-compat-2+console-virtio.xml b/tests/qemuhotplugtestdata/qemuhotplug-console-compat-2+console-virtio.xml index a484e82..25fc120 100644 --- a/tests/qemuhotplugtestdata/qemuhotplug-console-compat-2+console-virtio.xml +++ b/tests/qemuhotplugtestdata/qemuhotplug-console-compat-2+console-virtio.xml @@ -59,12 +59,12 @@ <interface type='network'> <mac address='52:54:00:ea:35:6f'/> <source network='default'/> - <target dev='vnet0'/> - <model type='virtio'/> <bandwidth> <inbound average='4000' peak='8000' floor='200' burst='1024'/> <outbound average='4000' peak='8000' burst='1024'/> </bandwidth> + <target dev='vnet0'/> + <model type='virtio'/> <alias name='net0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </interface> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml b/tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml index 065ef2d..e5c45a3 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml @@ -59,12 +59,12 @@ <interface type='network'> <mac address='52:54:00:ea:35:6f'/> <source network='default'/> - <target dev='vnet0'/> - <model type='virtio'/> <bandwidth> <inbound average='4000' peak='8000' floor='200' burst='1024'/> <outbound average='4000' peak='8000' burst='1024'/> </bandwidth> + <target dev='vnet0'/> + <model type='virtio'/> <alias name='net0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </interface> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-bandwidth.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-bandwidth.xml index 064a05f..f70e20a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-bandwidth.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-bandwidth.xml @@ -45,11 +45,11 @@ <interface type='network'> <mac address='52:54:00:24:a5:9f'/> <source network='default'/> - <model type='rtl8139'/> <bandwidth> <inbound average='1000' peak='4000' burst='1024'/> <outbound average='128' peak='256' burst='32768'/> </bandwidth> + <model type='rtl8139'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> </interface> <serial type='pty'>

Yup. since there can be only one <bandwidth/> sub-element right now, the ordering should not matter. ACK Michal

This function is currently only called from one place, but in a subsequent patch will be called from a 2nd place. The new function exactly replicates the original behavior of the part of virDomainActualNetDefFormat() that it replaces, but takes a virDomainNetDefPtr instead of virDomainActualNetDefPtr, and uses the virDomainNetGetActual*() functions whenever possible, rather than reaching into def->data.network.actual - this is to be sure that we are reporting exactly what is being used internally, just in case there are any discrepancies (there shouldn't be). --- Change from V1: I noticed during testing that bandwidth from a portgroup wasn't properly displayed if the network was forward mode='nat|route'. Fixing this required changing the conditions for using virDomainActualNetDefContentsFormat() - it must be used for those types of network as well, and when that is the case, we need to be sure to output the <source> element, as it otherwise would be skipped. src/conf/domain_conf.c | 113 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 76 insertions(+), 37 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 67fc372..8291b73 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15427,78 +15427,112 @@ virDomainHostdevDefFormatCaps(virBufferPtr buf, return 0; } +/* virDomainActualNetDefContentsFormat() - format just the subelements + * of <interface> that may be overridden by what is in the + * virDomainActualNetDef, but inside the current element, rather + * than enclosed in an <actual> subelement. + */ static int -virDomainActualNetDefFormat(virBufferPtr buf, - virDomainActualNetDefPtr def, - unsigned int flags) +virDomainActualNetDefContentsFormat(virBufferPtr buf, + virDomainNetDefPtr def, + const char *typeStr, + bool inSubelement, + unsigned int flags) { - const char *type; const char *mode; - if (!def) - return 0; - - type = virDomainNetTypeToString(def->type); - if (!type) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected net type %d"), def->type); - return -1; - } - - virBufferAsprintf(buf, "<actual type='%s'", type); - if (def->type == VIR_DOMAIN_NET_TYPE_HOSTDEV && - def->data.hostdev.def.managed) { - virBufferAddLit(buf, " managed='yes'"); - } - virBufferAddLit(buf, ">\n"); - - virBufferAdjustIndent(buf, 2); - switch (def->type) { + switch (virDomainNetGetActualType(def)) { case VIR_DOMAIN_NET_TYPE_BRIDGE: virBufferEscapeString(buf, "<source bridge='%s'/>\n", - def->data.bridge.brname); + virDomainNetGetActualBridgeName(def)); break; case VIR_DOMAIN_NET_TYPE_DIRECT: virBufferAddLit(buf, "<source"); - if (def->data.direct.linkdev) - virBufferEscapeString(buf, " dev='%s'", - def->data.direct.linkdev); + virBufferEscapeString(buf, " dev='%s'", + virDomainNetGetActualDirectDev(def)); - mode = virNetDevMacVLanModeTypeToString(def->data.direct.mode); + mode = virNetDevMacVLanModeTypeToString(virDomainNetGetActualDirectMode(def)); if (!mode) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected source mode %d"), - def->data.direct.mode); + virDomainNetGetActualDirectMode(def)); return -1; } virBufferAsprintf(buf, " mode='%s'/>\n", mode); break; case VIR_DOMAIN_NET_TYPE_HOSTDEV: - if (virDomainHostdevDefFormatSubsys(buf, &def->data.hostdev.def, + if (virDomainHostdevDefFormatSubsys(buf, virDomainNetGetActualHostdev(def), flags, true) < 0) { return -1; } break; case VIR_DOMAIN_NET_TYPE_NETWORK: - if (def->class_id) - virBufferAsprintf(buf, "<class id='%u'/>", def->class_id); + 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. + */ + virBufferEscapeString(buf, "<source network='%s'/>\n", + 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 net type %s"), type); + _("unexpected actual net type %s"), typeStr); return -1; } - if (virNetDevVlanFormat(&def->vlan, buf) < 0) + if (virNetDevVlanFormat(virDomainNetGetActualVlan(def), buf) < 0) return -1; - if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0) + if (virNetDevVPortProfileFormat(virDomainNetGetActualVirtPortProfile(def), buf) < 0) return -1; - if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0) + if (virNetDevBandwidthFormat(virDomainNetGetActualBandwidth(def), buf) < 0) + return -1; + return 0; +} + + +/* virDomainActualNetDefFormat() - format the ActualNetDef + * info inside an <actual> element, as required for internal storage + * of domain status + */ +static int +virDomainActualNetDefFormat(virBufferPtr buf, + virDomainNetDefPtr def, + unsigned int flags) +{ + unsigned int type = virDomainNetGetActualType(def); + const char *typeStr = virDomainNetTypeToString(type); + + if (!def) + return 0; + + if (!typeStr) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected net type %d"), def->type); return -1; + } + virBufferAsprintf(buf, "<actual type='%s'", typeStr); + if (type == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + virDomainHostdevDefPtr hostdef = virDomainNetGetActualHostdev(def); + if (hostdef && hostdef->managed) + virBufferAddLit(buf, " managed='yes'"); + } + virBufferAddLit(buf, ">\n"); + + virBufferAdjustIndent(buf, 2); + if (virDomainActualNetDefContentsFormat(buf, def, typeStr, true, flags) < 0) + return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</actual>\n"); return 0; @@ -15536,8 +15570,13 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferEscapeString(buf, " portgroup='%s'", def->data.network.portgroup); virBufferAddLit(buf, "/>\n"); + + /* ONLY for internal status storage - format the ActualNetDef + * as a subelement of <interface> so that no persistent config + * data is overwritten. + */ if ((flags & VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET) && - (virDomainActualNetDefFormat(buf, def->data.network.actual, flags) < 0)) + (virDomainActualNetDefFormat(buf, def, flags) < 0)) return -1; break; -- 1.8.5.3

On 21.02.2014 17:41, Laine Stump wrote:

Until now, the "live" XML status of an <interface type='network'> device would always show the network information, rather than the exact hardware device that was used. It would also show the name any portgroup the interface belonged to, rather than providing the configuration that was derived from the portgroup. As an example, given the following network definition:

[A] <network> <name>testnet</name> <forward type='bridge' dev='p4p1_0'> <interface dev='p4p1_0'/> <interface dev='p4p1_1'/> <interface dev='p4p1_2'/> <interface dev='p4p1_3'/> </forward> <portgroup name='admin'> <bandwidth> <inbound average='1000' peak='5000' burst='1024'/> <outbound average='128' peak='256' burst='256'/> </bandwidth> </portgroup> </network>

and the following domain <interface>:

[B] <interface type='network'> <source network='testnet' portgroup='admin'/> </interface>

the output of "virsh dumpxml $domain" while the domain was running would yield something like this:

[C] <interface type='network'> <source network='testnet' portgroup='admin'/> <target dev='macvtap0'/> <alias name='net0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </interface>

In order to learn the exact bandwidth information of the interface, a management application would need to retrieve the XML for testnet, then search for the portgroup named "admin". Even worse, there was no simple and standard way to learn which host physdev the macvtap0 device is attached to.

Internally, libvirt has always kept this information in the virDomainDef that's held in memory, as well as storing it in the (libvirt-internal-only) domain status XML (in /etc/libvirt/qemu/$domain.xml). In order to not confuse the runtime

I believe the status XMLs are kept under /var/run/libvirt/qemu/$domain.xml

"actual state" with the config of the device, it's internally stored like this:

[D] <interface type='network'> <source network='testnet' portgroup='admin'/> <actual type='direct'> <source dev='p4p1_0' mode='bridge'/> <bandwidth> <inbound average='1000' peak='5000' burst='1024'/> <outbound average='128' peak='256' burst='256'/> </bandwidth> </actual> <target dev='macvtap0'/> <alias name='net0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </interface>

This was never exposed outside of libvirt though, because I thought it would be too awkward for a management application to need to look in two places for the same information, but I also wasn't sure that it would be okay to overwrite the config info (in this case "<source network='testnet' portgroup='admin'/>") with the actual runtime info (everything inside <actual> above).

Now the time has come that this information must be made available to management applications (in particular, so that a network "plugged" hook will have full information about the device that is being plugged in), so it's time to take the leap and decide that it is acceptable for the config info to be replaced with actual runtime state (but *only* when reporting domain live status, *not* when saving state in /etc/libvirt/qemu/$domain.xml - that remains the same so that there is no loss of information). That is what this patch does. With this patch applied, the output of "virsh dumpxml $domain" when the domain is running will contain something like this:

[E] <interface type='direct'> <source dev='p4p1_0' mode='bridge'/> <bandwidth> <inbound average='1000' peak='5000' burst='1024'/> <outbound average='128' peak='256' burst='256'/> </bandwidth> <target dev='macvtap0'/> <alias name='net0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </interface>

In effect, everything that is internally stored within <actual> is moved up a level to where a management application will expect it. This means that the management application will only look in a single place to learn - the type of interface in use, the name of the physdev (if relevant), the <bandwidth>, <vlan>, and <virtualport> settings in use.

The potential downside is that a management app looking at this output will not see that the physdev 'p4p1_0' was actually allocated from the network named 'testnet', or that the bandwidth numbers were taken from the portgroup 'admin'. However, if they are interested in that info, they can always get the "inactive" XML for the domain.

An example of where this could cause problems is in virt-manager's network device display, which shows the status of the device, but allows you to edit that status info and save it as the new config. Previously virt-manager would always display the information in example [C] above, and allow editing that. With this patch, it will instead display what is in [E] and allow editing it directly, which could lead to some confusion. I would suggest that virt-manager have an "edit" button which would change the display from the "live" xml to the "inactive" xml, so that editing would be done on that; such a change would both handle the new situation, and also be compatible with older releases. --- Change from V1:

Updated arglist for virDomainActualNetDefContentsFormat() to match change in Patch 5/7.

src/conf/domain_conf.c | 189 +++++++++++++++++++++++++++++-------------------- 1 file changed, 114 insertions(+), 75 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8291b73..695a8e7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15543,104 +15543,143 @@ virDomainNetDefFormat(virBufferPtr buf, virDomainNetDefPtr def, unsigned int flags) { - const char *type = virDomainNetTypeToString(def->type); + /* publicActual is true if we should report the current state in + * def->data.network.actual *instead of* the config (*not* in + * addition to) + */ + unsigned int actualType = virDomainNetGetActualType(def); + bool publicActual + = (def->type == VIR_DOMAIN_NET_TYPE_NETWORK && def->data.network.actual && + !(flags & (VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET))); + const char *typeStr; + virDomainHostdevDefPtr hostdef = NULL; char macstr[VIR_MAC_STRING_BUFLEN];

- if (!type) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected net type %d"), def->type); - return -1; + + if (publicActual) { + if (!(typeStr = virDomainNetTypeToString(actualType))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected actual net type %d"), actualType); + return -1; + } + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) + hostdef = virDomainNetGetActualHostdev(def); + } else { + if (!(typeStr = virDomainNetTypeToString(def->type))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected net type %d"), def->type); + return -1; + } + if (def->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) + hostdef = &def->data.hostdev.def; }

- virBufferAsprintf(buf, " <interface type='%s'", type); - if (def->type == VIR_DOMAIN_NET_TYPE_HOSTDEV && - def->data.hostdev.def.managed) { + virBufferAsprintf(buf, " <interface type='%s'", typeStr); + if (hostdef && hostdef->managed) virBufferAddLit(buf, " managed='yes'"); - } virBufferAddLit(buf, ">\n");

virBufferAdjustIndent(buf, 6); virBufferAsprintf(buf, "<mac address='%s'/>\n", virMacAddrFormat(&def->mac, macstr));

- switch (def->type) { - case VIR_DOMAIN_NET_TYPE_NETWORK: - virBufferEscapeString(buf, "<source network='%s'", - def->data.network.name); - virBufferEscapeString(buf, " portgroup='%s'", - def->data.network.portgroup); - virBufferAddLit(buf, "/>\n"); - - /* ONLY for internal status storage - format the ActualNetDef - * as a subelement of <interface> so that no persistent config - * data is overwritten. + if (publicActual) { + /* when there is a virDomainActualNetDef, and we haven't been + * asked to 1) report the domain's inactive XML, or 2) give + * the internal version of the ActualNetDef separately in an + * <actual> subelement, we can just put the ActualDef data in + * the standard place... (this is for public reporting of + * interface status) */ - if ((flags & VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET) && - (virDomainActualNetDefFormat(buf, def, flags) < 0)) + if (virDomainActualNetDefContentsFormat(buf, def, typeStr, false, flags) < 0) return -1; - break; + } else { + /* ...but if we've asked for the inactive XML (rather than + * status), or to report the ActualDef as a separate <actual> + * subelement (this is how we privately store interface + * status), or there simply *isn't* any ActualNetDef, then + * format the NetDef's data here, and optionally format the + * ActualNetDef as an <actual> subelement of this element. + */ + switch (def->type) { + case VIR_DOMAIN_NET_TYPE_NETWORK: + virBufferEscapeString(buf, "<source network='%s'", + def->data.network.name); + virBufferEscapeString(buf, " portgroup='%s'", + def->data.network.portgroup); + virBufferAddLit(buf, "/>\n");

- case VIR_DOMAIN_NET_TYPE_ETHERNET: - virBufferEscapeString(buf, "<source dev='%s'/>\n", - def->data.ethernet.dev); - if (def->data.ethernet.ipaddr) - virBufferAsprintf(buf, "<ip address='%s'/>\n", - def->data.ethernet.ipaddr); - break; + /* ONLY for internal status storage - format the ActualNetDef + * as a subelement of <interface> so that no persistent config + * data is overwritten. + */ + if ((flags & VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET) && + (virDomainActualNetDefFormat(buf, def, flags) < 0)) + return -1; + break;

- case VIR_DOMAIN_NET_TYPE_BRIDGE: - virBufferEscapeString(buf, "<source bridge='%s'/>\n", - def->data.bridge.brname); - if (def->data.bridge.ipaddr) { - virBufferAsprintf(buf, "<ip address='%s'/>\n", - def->data.bridge.ipaddr); - } - break; + case VIR_DOMAIN_NET_TYPE_ETHERNET: + virBufferEscapeString(buf, "<source dev='%s'/>\n", + def->data.ethernet.dev); + if (def->data.ethernet.ipaddr) + virBufferAsprintf(buf, "<ip address='%s'/>\n", + def->data.ethernet.ipaddr); + break;

- case VIR_DOMAIN_NET_TYPE_SERVER: - case VIR_DOMAIN_NET_TYPE_CLIENT: - case VIR_DOMAIN_NET_TYPE_MCAST: - if (def->data.socket.address) { - virBufferAsprintf(buf, "<source address='%s' port='%d'/>\n", - def->data.socket.address, def->data.socket.port); - } else { - virBufferAsprintf(buf, "<source port='%d'/>\n", - def->data.socket.port); - } - break; + case VIR_DOMAIN_NET_TYPE_BRIDGE: + virBufferEscapeString(buf, "<source bridge='%s'/>\n", + def->data.bridge.brname); + if (def->data.bridge.ipaddr) { + virBufferAsprintf(buf, "<ip address='%s'/>\n", + def->data.bridge.ipaddr); + } + break;

- case VIR_DOMAIN_NET_TYPE_INTERNAL: - virBufferEscapeString(buf, "<source name='%s'/>\n", - def->data.internal.name); - break; + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + if (def->data.socket.address) { + virBufferAsprintf(buf, "<source address='%s' port='%d'/>\n", + def->data.socket.address, def->data.socket.port); + } else { + virBufferAsprintf(buf, "<source port='%d'/>\n", + def->data.socket.port); + } + break;

- case VIR_DOMAIN_NET_TYPE_DIRECT: - virBufferEscapeString(buf, "<source dev='%s'", - def->data.direct.linkdev); - virBufferAsprintf(buf, " mode='%s'", - virNetDevMacVLanModeTypeToString(def->data.direct.mode)); - virBufferAddLit(buf, "/>\n"); - break; + case VIR_DOMAIN_NET_TYPE_INTERNAL: + virBufferEscapeString(buf, "<source name='%s'/>\n", + def->data.internal.name); + break;

- case VIR_DOMAIN_NET_TYPE_HOSTDEV: - if (virDomainHostdevDefFormatSubsys(buf, &def->data.hostdev.def, - flags, true) < 0) { - return -1; + case VIR_DOMAIN_NET_TYPE_DIRECT: + virBufferEscapeString(buf, "<source dev='%s'", + def->data.direct.linkdev); + virBufferAsprintf(buf, " mode='%s'", + virNetDevMacVLanModeTypeToString(def->data.direct.mode)); + virBufferAddLit(buf, "/>\n"); + break; + + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + if (virDomainHostdevDefFormatSubsys(buf, &def->data.hostdev.def, + flags, true) < 0) { + return -1; + } + break; + + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_LAST: + break; } - break;

- case VIR_DOMAIN_NET_TYPE_USER: - case VIR_DOMAIN_NET_TYPE_LAST: - break; + if (virNetDevVlanFormat(&def->vlan, buf) < 0) + return -1; + if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0) + return -1; + if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0) + return -1; }

- if (virNetDevVlanFormat(&def->vlan, buf) < 0) - return -1; - if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0) - return -1; - if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0) - return -1; - virBufferEscapeString(buf, "<script path='%s'/>\n", def->script); if (def->ifname &&

ACK Michal

On 02/24/2014 06:45 PM, Michal Privoznik wrote:

On 21.02.2014 14:58, Laine Stump wrote:

...

The network hook script gets called whenever an interface is plugged into or unplugged from a network, but even though the full XML of both the network and the domain is included, there is no reasonable way to determine what exact resources the plugged interface is using:

1) Prior to a recent patch which modified the status XML of interfaces to include the information about actual hardware resources used, it would be possible to scan through the domain XML output sent to the hook, and from there find the correct interface, but that interface definition would not include any runtime info (e.g. bandwidth or vlan taken from a portgroup, or which physdev was used in case of a macvtap network).

2) After the patch modifying the status XML of interfaces, the network name would no longer be included in the domain XML, so it would be completely impossible to determine which interface was the one being plugged.

To solve that problem, this patch includes a single <interface> element at the beginning of the XML sent to the network hook for "plugged" and "unplugged" (just inside <hookData>) that is the status XML of the interface being plugged. This XML will include all info gathered from the chosen network and portgroup.

NB: due to hardcoded spaces in all of the device *Format() functions, the <interface> element inside the <hookData> will be indented by 6 spaces rather than 2. I had intended to fix this, but it turns out that to make virDomainNetDefFormat() indentation relative, I would have to do the same to virDomainDeviceInfoFormat(), and that function is called from 19 places - making that a prerequisite of this patch would cause too many merge difficulties if we needed to backport network hooks, so I chose to ignore the problem here and fix the problem for *all* devices in a followup later. --- src/network/bridge_driver.c | 48 ++++++++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a6c719d..152bd06 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -141,6 +141,7 @@ networkObjFromNetwork(virNetworkPtr net) static int networkRunHook(virNetworkObjPtr network, virDomainDefPtr dom, + virDomainNetDefPtr iface, int op, int sub_op) { @@ -158,6 +159,8 @@ networkRunHook(virNetworkObjPtr network,

virBufferAddLit(&buf, "<hookData>\n"); virBufferAdjustIndent(&buf, 2); + if (iface && virDomainNetDefFormat(&buf, iface, 0) < 0) + goto cleanup; if (virNetworkDefFormatBuf(&buf, network->def, 0) < 0) goto cleanup; if (dom && virDomainDefFormatInternal(dom, 0, &buf) < 0) @@ -2067,7 +2070,7 @@ networkStartNetwork(virNetworkDriverStatePtr driver,

/* Run an early hook to set-up missing devices. * If the script raised an error abort the launch. */ - if (networkRunHook(network, NULL, + if (networkRunHook(network, NULL, NULL, VIR_HOOK_NETWORK_OP_START, VIR_HOOK_SUBOP_BEGIN) < 0) goto cleanup; @@ -2092,7 +2095,7 @@ networkStartNetwork(virNetworkDriverStatePtr driver, }

/* finally we can call the 'started' hook script if any */ - if (networkRunHook(network, NULL, + if (networkRunHook(network, NULL, NULL, VIR_HOOK_NETWORK_OP_STARTED, VIR_HOOK_SUBOP_BEGIN) < 0) goto cleanup; @@ -2158,7 +2161,7 @@ static int networkShutdownNetwork(virNetworkDriverStatePtr driver, }

/* now that we know it's stopped call the hook if present */ - networkRunHook(network, NULL, VIR_HOOK_NETWORK_OP_STOPPED, + networkRunHook(network, NULL, NULL, VIR_HOOK_NETWORK_OP_STOPPED, VIR_HOOK_SUBOP_END);

network->active = 0; @@ -3659,14 +3662,8 @@ validate: } }

- /* finally we can call the 'plugged' hook script if any */ - if (networkRunHook(network, dom, - VIR_HOOK_NETWORK_OP_IFACE_PLUGGED, - VIR_HOOK_SUBOP_BEGIN) < 0) - goto error; - if (dev) { - /* we are now assured of success, so mark the allocation */ + /* mark the allocation */ dev->connections++; if (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV) { VIR_DEBUG("Using physical device %s, %d connections", @@ -3684,6 +3681,19 @@ validate: VIR_DEBUG("Using network %s, %d connections", netdef->name, netdef->connections); } + + /* finally we can call the 'plugged' hook script if any */ + if (networkRunHook(network, dom, iface, + VIR_HOOK_NETWORK_OP_IFACE_PLUGGED, + VIR_HOOK_SUBOP_BEGIN) < 0) { + /* adjust for failure */ + if (dev) + dev->connections--; + if (netdef) + netdef->connections--; + goto error; + } + ret = 0;

cleanup: @@ -3865,14 +3875,20 @@ networkNotifyActualDevice(virDomainDefPtr dom, }

success: - /* finally we can call the 'plugged' hook script if any */ - if (networkRunHook(network, dom, VIR_HOOK_NETWORK_OP_IFACE_PLUGGED, - VIR_HOOK_SUBOP_BEGIN) < 0) - goto error; - netdef->connections++; VIR_DEBUG("Using network %s, %d connections", netdef->name, netdef->connections); + + /* finally we can call the 'plugged' hook script if any */ + if (networkRunHook(network, dom, iface, VIR_HOOK_NETWORK_OP_IFACE_PLUGGED, + VIR_HOOK_SUBOP_BEGIN) < 0) { + /* adjust for failure */ + if (dev) + dev->connections--; + netdef->connections--; + goto error; + } + ret = 0; cleanup: if (network) @@ -4018,7 +4034,7 @@ success: netdef->connections--;

/* finally we can call the 'unplugged' hook script if any */ - networkRunHook(network, dom, VIR_HOOK_NETWORK_OP_IFACE_UNPLUGGED, + networkRunHook(network, dom, iface, VIR_HOOK_NETWORK_OP_IFACE_UNPLUGGED, VIR_HOOK_SUBOP_BEGIN);

VIR_DEBUG("Releasing network %s, %d connections",

ACK although the indentation of XML we're passing to the hook script seems off:

Right. That's what the last paragraph of the commit log message is about - fixing that indentation would require a much more invasive change that would touch all the other device parsing functions, which could turn any potential backport into a real headache, so I chose to not fix it in this series.

<hookData> <interface type='network'> ... </interface> <network> ... </network> <domain type='kvm'> </domain> </hookData>

This is not a show stopper for me. I wonder if we should push these patches now, even during the freeze as this is very closely related to the network hooks - one of the main features in this release.

In a way it is a bugfix for that feature (since the functionality of the "plugged" hook is severely limited without it). I would feel more comfortable about pushing it, though, if danpb took a look at the commit log message for patch 5/7 and gave his okay on the change in the XML. My opinion is that I should have done it this way to begin with, but Dan has much better insight than I do on what is and isn't good for management applications.