[libvirt] [PATCH 25/28] conf: support host-side IP/route information in <interface>
John Ferlan
jferlan at redhat.com
Fri Jun 24 13:48:26 UTC 2016
On 06/22/2016 01:37 PM, Laine Stump wrote:
> This is non-intuitively placed as a sub-element of <target>, because
> it will be used to configure the interface that is named in <target
> dev='x'/> (which is the interface on the host-side). (The
> already-existing configuration for the guest-side of interfaces is in
> subelements directly under <interface>):
>
> <interface type='ethernet'>
> <mac address='00:16:3e:0f:ef:8a'/>
> <source>
> <ip address='192.168.122.12' family='ipv4'
> prefix='24' peer='192.168.122.1'/>
> <ip address='192.168.122.13' family='ipv4' prefix='24'/>
> <route family='ipv4' address='0.0.0.0'
> gateway='192.168.122.1'/>
> <route family='ipv4' address='192.168.124.0' prefix='24'
> gateway='192.168.124.1'/>
> </source>
> </interface>
>
> In practice, this will likely only be useful for type='ethernet', so
> its presence in any other type of interface is currently forbidden in
> the generic device Validate function (but it's been put into the
> general population of virDomainNetDef rather than the
> ethernet-specific union member so that 1) we can more easily add the
> capability to other types, and 2) we can retain the info when set to
> an invalid interface type all the way through to validation and report
> a proper error, rather than just ignoring it (which is currently what
> happens for many other type-specific settings).
> ---
> docs/formatdomain.html.in | 26 ++++++++
> docs/schemas/domaincommon.rng | 3 +-
> src/conf/domain_conf.c | 97 ++++++++++++++++++++++------
> src/conf/domain_conf.h | 3 +-
> tests/lxcxml2xmldata/lxc-ethernet-hostip.xml | 44 +++++++++++++
> tests/lxcxml2xmltest.c | 1 +
> 6 files changed, 152 insertions(+), 22 deletions(-)
> create mode 100644 tests/lxcxml2xmldata/lxc-ethernet-hostip.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 2466df7..bb1c079 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -5012,6 +5012,32 @@ qemu-kvm -net nic,model=? /dev/null
> definitions</a>. This is used by the LXC driver.
> </p>
>
> +<pre>
> + ...
> + <devices>
> + <interface type='ethernet'>
> + <b><source/></b>
> + <b><ip address='192.168.123.1' prefix='24'/></b>
> + <b><ip address='10.0.0.10' prefix='24' peer='192.168.122.5'/></b>
> + <b><route family='ipv4' address='192.168.42.0' prefix='24' gateway='192.168.123.4'/></b>
> + <b><source/></b>
> + ...
> + </interface>
> + ...
> + </devices>
> + ...
> +</pre>
> +
> + <p>
> + <span class="since">Since 2.0.0</span> network devices of type
> + "ethernet" can optionally be provided one or more IP addresses
> + and one or more routes to set on the <b>host</b> side of the
> + network device. These are configured as subelements of
> + the <code><source></code> element of the interface, and
> + have the same attributes as the similarly named elements used to
> + configure the guest side of the interface (described above).
> + </p>
> +
> <h5><a name="elementVhostuser">vhost-user interface</a></h5>
>
> <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 2d12da9..964ff92 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2142,7 +2142,7 @@
> <interleave>
> <optional>
> <element name="source">
> - <empty/>
> + <ref name="interface-ip-info"/>
> </element>
> </optional>
> <ref name="interface-options"/>
> @@ -2392,7 +2392,6 @@
> <attribute name="dev">
> <ref name="deviceName"/>
> </attribute>
> - <empty/>
> </element>
> </optional>
> <optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ad2d983..c2e6663 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1800,6 +1800,7 @@ virDomainNetDefClear(virDomainNetDefPtr def)
> VIR_FREE(def->ifname_guest_actual);
>
> virNetDevIPInfoClear(&def->guestIP);
> + virNetDevIPInfoClear(&def->hostIP);
> virDomainDeviceInfoClear(&def->info);
>
> VIR_FREE(def->filter);
> @@ -4610,6 +4611,23 @@ virDomainRedirdevDefValidate(const virDomainDef *def,
>
>
> static int
> +virDomainNetDefValidate(const virDomainNetDef *net)
> +{
> + if ((net->hostIP.nroutes || net->hostIP.nips) &&
> + net->type != VIR_DOMAIN_NET_TYPE_ETHERNET) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Invalid attempt to set network interface "
> + "host-side IP route and/or address info on "
> + "interface of type '%s'. This is only supported "
> + "on interfaces of type 'ethernet'"),
> + virDomainNetTypeToString(net->type));
> + return -1;
> + }
> + return 0;
> +}
It seems as though you are *adding* a new element - thus, this could not
be present on a currently running domain, so wouldn't the "more correct"
placement be the PostParse API's ?
> +
> +
> +static int
> virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev,
> const virDomainDef *def)
> {
> @@ -4620,9 +4638,11 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev,
> case VIR_DOMAIN_DEVICE_REDIRDEV:
> return virDomainRedirdevDefValidate(def, dev->data.redirdev);
>
> + case VIR_DOMAIN_DEVICE_NET:
> + return virDomainNetDefValidate(dev->data.net);
> +
> case VIR_DOMAIN_DEVICE_LEASE:
> case VIR_DOMAIN_DEVICE_FS:
> - case VIR_DOMAIN_DEVICE_NET:
> case VIR_DOMAIN_DEVICE_INPUT:
> case VIR_DOMAIN_DEVICE_SOUND:
> case VIR_DOMAIN_DEVICE_VIDEO:
> @@ -8977,6 +8997,15 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> cur = node->children;
> while (cur != NULL) {
> if (cur->type == XML_ELEMENT_NODE) {
> + if (xmlStrEqual(cur->name, BAD_CAST "source")) {
> + xmlNodePtr tmpnode = ctxt->node;
> +
> + ctxt->node = cur;
> + if (virDomainNetIPInfoParseXML(_("interface host IP"),
> + ctxt, &def->hostIP) < 0)
> + goto error;
> + ctxt->node = tmpnode;
> + }
> if (!macaddr && xmlStrEqual(cur->name, BAD_CAST "mac")) {
> macaddr = virXMLPropString(cur, "address");
> } else if (!network &&
> @@ -20682,6 +20711,7 @@ virDomainNetDefFormat(virBufferPtr buf,
> {
> unsigned int actualType = virDomainNetGetActualType(def);
> bool publicActual = false;
> + int sourceLines = 0;
> const char *typeStr;
> virDomainHostdevDefPtr hostdef = NULL;
> char macstr[VIR_MAC_STRING_BUFLEN];
> @@ -20751,15 +20781,7 @@ virDomainNetDefFormat(virBufferPtr buf,
> 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 ((flags & VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET) &&
> - (virDomainActualNetDefFormat(buf, def, flags) < 0))
> - return -1;
> + sourceLines++;
All these sourceLines++ probably could have been their own patch...
Adding the hostIP's separately. Mixing the two is was a bit tough to
read and understand...
> break;
>
> case VIR_DOMAIN_NET_TYPE_ETHERNET:
> @@ -20773,13 +20795,16 @@ virDomainNetDefFormat(virBufferPtr buf,
> virBufferAsprintf(buf, " mode='%s'",
> def->data.vhostuser->data.nix.listen ?
> "server" : "client");
> - virBufferAddLit(buf, "/>\n");
> + sourceLines++;
> }
> break;
>
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> - virBufferEscapeString(buf, "<source bridge='%s'/>\n",
> - def->data.bridge.brname);
> + if (def->data.bridge.brname) {
This alone seems line a separate patch... But then again there's already
sooooo many....
> + virBufferEscapeString(buf, "<source bridge='%s'",
> + def->data.bridge.brname);
> + sourceLines++;
> + }
> break;
>
> case VIR_DOMAIN_NET_TYPE_SERVER:
> @@ -20794,25 +20819,25 @@ virDomainNetDefFormat(virBufferPtr buf,
> virBufferAsprintf(buf, "<source port='%d'",
> def->data.socket.port);
> }
> + sourceLines++;
>
> - if (def->type != VIR_DOMAIN_NET_TYPE_UDP) {
> - virBufferAddLit(buf, "/>\n");
> + if (def->type != VIR_DOMAIN_NET_TYPE_UDP)
> break;
> - }
>
> virBufferAddLit(buf, ">\n");
> + sourceLines++;
> virBufferAdjustIndent(buf, 2);
>
> virBufferAsprintf(buf, "<local address='%s' port='%d'/>\n",
> def->data.socket.localaddr,
> def->data.socket.localport);
> virBufferAdjustIndent(buf, -2);
> - virBufferAddLit(buf, "</source>\n");
> break;
>
> case VIR_DOMAIN_NET_TYPE_INTERNAL:
> - virBufferEscapeString(buf, "<source name='%s'/>\n",
> + virBufferEscapeString(buf, "<source name='%s'",
> def->data.internal.name);
> + sourceLines++;
> break;
>
> case VIR_DOMAIN_NET_TYPE_DIRECT:
> @@ -20820,7 +20845,7 @@ virDomainNetDefFormat(virBufferPtr buf,
> def->data.direct.linkdev);
> virBufferAsprintf(buf, " mode='%s'",
> virNetDevMacVLanModeTypeToString(def->data.direct.mode));
> - virBufferAddLit(buf, "/>\n");
> + sourceLines++;
> break;
>
> case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> @@ -20835,12 +20860,44 @@ virDomainNetDefFormat(virBufferPtr buf,
> break;
> }
>
> + /* if sourceLines == 0 - no <source> info at all so far
> + * sourceLines == 1 - first line writte, no terminating ">"
s/writte/written
I think the Validate should be a PostParse - your thoughts... The
'contents' of the change are ACKable, I just think the placement is a
bit off. Then of course there's the whole doing multiple things here
(there could conceivably be 3 patches out of this).
I "assume" there are other XML2XML tests that ensure all the following
magic is correct since you added one for the new data...
John
> + * sourceLines > 1 - multiple lines, including subelements
> + */
> + if (def->hostIP.nips || def->hostIP.nroutes) {
> + if (sourceLines == 0) {
> + virBufferAddLit(buf, "<source>\n");
> + sourceLines += 2;
> + } else if (sourceLines == 1) {
> + virBufferAddLit(buf, ">\n");
> + sourceLines++;
> + }
> + virBufferAdjustIndent(buf, 2);
> + if (virDomainNetIPInfoFormat(buf, &def->hostIP) < 0)
> + return -1;
> + virBufferAdjustIndent(buf, -2);
> + }
> + if (sourceLines == 1)
> + virBufferAddLit(buf, "/>\n");
> + else if (sourceLines > 1)
> + virBufferAddLit(buf, "</source>\n");
> +
> if (virNetDevVlanFormat(&def->vlan, buf) < 0)
> return -1;
> if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0)
> return -1;
> if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0)
> return -1;
> +
> + /* ONLY for internal status storage - format the ActualNetDef
> + * as a subelement of <interface> so that no persistent config
> + * data is overwritten.
> + */
> + if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> + (flags & VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET) &&
> + (virDomainActualNetDefFormat(buf, def, flags) < 0))
> + return -1;
> +
> }
More information about the libvir-list
mailing list