10:19 a.m.
On 06/24/2016 09:48 AM, John Ferlan wrote:
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 ?
I don't think we're losing any functionality by putting it here (other
than that if someone edits the config files directly on disk and then
restart libvirtd they won't see an error; but then they'll be getting
what they deserve). And I think its function fits better with the
Validate function than with the PostParse callback (which admittedly
does have some validating done in it, but that's just because the
Validate callbacks are a fairly recent addition.
In general, I think anything that is just validating the data in a
domain as a whole should be done in the Validate callbacks, and things
that modify the domain or devices should be in the postparse callbacks.
Eventually pure validation should migrate, and for now at least new
stuff should be added in that way. (Note that I still think that basic
validation that doesn't require knowledge of any other part of the XML
should still be done directly in the parse (numeric ranges, etc;
basically anything that is described in the RNG).
> +
> +
> +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...
The further along I got with this, the more I came to think the same
thing, but I was already up to patch 25 and too deep into this one to
want to back it out and re-do in steps...
> 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....
The reason that extra if() was needed is that virBufferEscapeString()
will emit nothing if the argument is NULL, but in our case we only want
to increment sourceLines if something was printed. (This was caught by a
unit test, btw).
> + 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...
See above. I think you're just resistent to change :-P
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...
Yes. The unit tests caught problems with it initially, so they are there.