Re: [PATCH 25/28] conf: support host-side IP/route information in <interface>

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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>
> +  ...
> +  &lt;devices&gt;
> +    &lt;interface type='ethernet'&gt;
> +      <b>&lt;source/&gt;</b>
> +        <b>&lt;ip address='192.168.123.1' prefix='24'/&gt;</b>
> +        <b>&lt;ip address='10.0.0.10' prefix='24' peer='192.168.122.5'/&gt;</b>
> +        <b>&lt;route family='ipv4' address='192.168.42.0' prefix='24' gateway='192.168.123.4'/&gt;</b>
> +      <b>&lt;source/&gt;</b>
> +      ...
> +    &lt;/interface&gt;
> +    ...
> +  &lt;/devices&gt;
> +  ...
> +</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>&lt;source&gt;</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;
> +
>      }

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]