[libvirt] [PATCHv2 6/7] conf: output actual netdev status in <interface> XML
Michal Privoznik
mprivozn at redhat.com
Mon Feb 24 16:45:54 UTC 2014
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
More information about the libvir-list
mailing list