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

Re: [libvirt] [PATCHv2 9/9] network: implement backend of virNetworkUpdate(IP_DHCP_HOST)



On Tue, Sep 18, 2012 at 03:39:05AM -0400, Laine Stump wrote:
> This patch fills in the first implementation for one of the
> virNetworkUpdate sections. With this code, you can now add/delete/edit
> <host> entries in a network's <ip> address <dhcp> element (by
> specifying a section of VIR_NETWORK_SECTION_IP_DHCP_HOST).
> 
> If you pass in a parentIndex of -1, the code will automatically find
> the one ip element that has a <dhcp> section and make the updates
> there. Otherwise, you can specify an index >= 0, and libvirt will look
> for that particular instance of <ip> in the network, and modify its
> <dhcp> element. (This currently isn't very useful, because libvirt
> only supports having dhcp information on a single IP address, but that
> could change in the future).
> 
> When adding a new host entry
> (VIR_NETWORK_UPDATE_COMMAND_ADD_(FIRST|LAST)), the existing entries
> will be compared to the new entry, and if any non-empty attribute
> matches, the add will fail. When updating an existing entry
> (VIR_NETWORK_UPDATE_COMMAND_MODIFY), the mac address or name will be
> used to find the existing entry, and other fields will only be updated
> (note there is some potential for ambiguity here if you specify the
> mac address from one entry and the name from another).  When deleting
> an existing entry (VIR_NETWORK_UPDATE_COMMAND_DELETE), all non-empty
> attributes in the supplied xml arg will be compared - all of them must
> match before libvirt will delete the host.
> 
> The xml should be a fully formed <host> element as it would appear in
> a network definition, e.g. "<host mac=00:11:22:33:44:55 ip=10.1.23.22
> name='testbox'/>" (when adding/updating, ip and one of mac|name is
> required; when deleting, you can specify any one, two, or all
> attributes, but they all must match the target element).
> 
> As with the update of any other section, you can choose to affect the
> live config (with flag VIR_NETWORK_UPDATE_AFFECT_LIVE), the persistent
> config (VIR_NETWORK_UPDATE_AFFECT_CONFIG), or both. If you've chosen
> to affect the live config, those changes will take effect immediately,
> with no need to destroy/restart the network.
> 
> An example of adding a host entry:
> 
>    virNetworkUpdate(net, VIR_NETWORK_UPDATE_COMMAND_ADD_LAST,
>                      VIR_NETWORK_SECTION_IP_DHCP_HOST, -1,
>                     "<host mac='00:11:22:33:44:55' ip='192.168.122.5'/>",
>                     VIR_NETWORK_UPDATE_AFFECT_LIVE
>                     | VIR_NETWORK_UPDATE_AFFECT_CONFIG);
> 
> To delete that same entry:
> 
>    virNetworkUpdate(net, VIR_NETWORK_UPDATE_COMMAND_DELETE,
>                     VIR_NETWORK_SECTION_IP_DHCP_HOST, -1,
>                     "<host mac='00:11:22:33:44:55'/>",
>                     VIR_NETWORK_UPDATE_AFFECT_LIVE
>                     | VIR_NETWORK_UPDATE_AFFECT_CONFIG);
> 
> (you could also delete it by replacing "mac='00:11:22:33:44:55'" with
> "ip='192.168.122.5'".)

  We need to move that documentation outside of the git commit log,
I mean keep it here but this need to go on the web site !

>  src/conf/network_conf.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 174 insertions(+), 7 deletions(-)
> 
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 2a65f1d..4f40c10 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -2261,7 +2261,6 @@ virNetworkDefUpdateNoSupport(virNetworkDefPtr def, const char *section)
>                     section, def->name);
>  }
>  
> -#if 0
>  static int
>  virNetworkDefUpdateCheckElementName(virNetworkDefPtr def,
>                                      xmlNodePtr node,
> @@ -2276,7 +2275,6 @@ virNetworkDefUpdateCheckElementName(virNetworkDefPtr def,
>      }
>      return 0;
>  }
> -#endif
>  
>  static int
>  virNetworkDefUpdateBridge(virNetworkDefPtr def,
> @@ -2314,16 +2312,185 @@ virNetworkDefUpdateIP(virNetworkDefPtr def,
>      return -1;
>  }
>  
> +static virNetworkIpDefPtr
> +virNetworkIpDefByIndex(virNetworkDefPtr def, int parentIndex)
> +{
> +    virNetworkIpDefPtr ipdef = NULL;
> +
> +    /* first find which ip element's dhcp host list to work on */
> +    if (parentIndex >= 0) {
> +        ipdef = virNetworkDefGetIpByIndex(def, AF_UNSPEC, parentIndex);
> +        if (!(ipdef &&
> +              VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET))) {
> +            virReportError(VIR_ERR_OPERATION_INVALID,
> +                           _("couldn't update dhcp host entry - "
> +                             "no <ip family='ipv4'> "
> +                             "element found at index %d in network '%s'"),
> +                           parentIndex, def->name);
> +        }
> +        return ipdef;
> +    }
> +
> +    /* -1 means "find the most appropriate", which in this case
> +     * means the one and only <ip> that has <dhcp> element
> +     */
> +    int ii;

  <grin/> can you move that up to be block start ?

> +
> +    for (ii = 0;
> +         (ipdef = virNetworkDefGetIpByIndex(def, AF_UNSPEC, ii));
> +         ii++) {
> +        if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET) &&
> +            (ipdef->nranges || ipdef->nhosts)) {
> +            break;
> +        }
> +    }
> +    if (!ipdef)
> +        ipdef = virNetworkDefGetIpByIndex(def, AF_INET, 0);
> +    if (!ipdef) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       _("couldn't update dhcp host entry - "
> +                         "no <ip family='ipv4'> "
> +                         "element found in network '%s'"), def->name);
> +    }
> +    return ipdef;
> +}
> +
>  static int
>  virNetworkDefUpdateIPDHCPHost(virNetworkDefPtr def,
> -                              unsigned int command ATTRIBUTE_UNUSED,
> -                              int parentIndex ATTRIBUTE_UNUSED,
> -                              xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED,
> +                              unsigned int command,
> +                              int parentIndex,
> +                              xmlXPathContextPtr ctxt,
>                                /* virNetworkUpdateFlags */
>                                unsigned int fflags ATTRIBUTE_UNUSED)
>  {
> -    virNetworkDefUpdateNoSupport(def, "ip dhcp host");
> -    return -1;
> +    int ii, ret = -1;
> +    virNetworkIpDefPtr ipdef = virNetworkIpDefByIndex(def, parentIndex);
> +    virNetworkDHCPHostDef host;
> +
> +    memset(&host, 0, sizeof(host));
> +
> +    if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "host") < 0)
> +        goto cleanup;
> +
> +    /* ipdef is the ip element that needs its host array updated */
> +    if (!ipdef)
> +        goto cleanup;
> +
> +    /* parse the xml into a virNetworkDHCPHostDef */
> +    if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {
> +
> +        if (virNetworkDHCPHostDefParse(def->name, ctxt->node, &host, false) < 0)
> +            goto cleanup;
> +
> +        /* search for the entry with this (mac|name),
> +         * and update the IP+(mac|name) */
> +        for (ii = 0; ii < ipdef->nhosts; ii++) {
> +            if ((host.mac &&
> +                 !virMacAddrCompare(host.mac, ipdef->hosts[ii].mac)) ||
> +                (host.name &&
> +                 STREQ_NULLABLE(host.name, ipdef->hosts[ii].name))) {
> +                break;
> +            }
> +        }
> +
> +        if (ii == ipdef->nhosts) {
> +            virReportError(VIR_ERR_OPERATION_INVALID,
> +                           _("couldn't locate an existing dhcp host entry with "
> +                             "\"mac='%s'\" in network '%s'"),
> +                           host.mac, def->name);
> +            goto cleanup;
> +        }
> +
> +        /* eliminate the old */
> +        virNetworkDHCPHostDefClear(&ipdef->hosts[ii]);
> +        /* replace with new */
> +        ipdef->hosts[ii] = host;
> +        /* eliminate the extra copy of the new */
> +        memset(&host, 0, sizeof(host));
> +        /* Success! */

  Hum, those comments are fine in development mode, but maybe a bit of
cleanup or more synthetic comment would be better :-)

> +    } else if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) ||
> +               (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST)) {
> +
> +        if (virNetworkDHCPHostDefParse(def->name, ctxt->node, &host, true) < 0)
> +            goto cleanup;
> +
> +        /* log error if an entry with same name/address/ip already exists */
> +        for (ii = 0; ii < ipdef->nhosts; ii++) {
> +            if ((host.mac &&
> +                 !virMacAddrCompare(host.mac, ipdef->hosts[ii].mac)) ||
> +                (host.name &&
> +                 STREQ_NULLABLE(host.name, ipdef->hosts[ii].name)) ||
> +                (VIR_SOCKET_ADDR_VALID(&host.ip) &&
> +                 virSocketAddrEqual(&host.ip, &ipdef->hosts[ii].ip))) {
> +                char *ip = virSocketAddrFormat(&host.ip);
> +
> +                virReportError(VIR_ERR_OPERATION_INVALID,
> +                               _("there is an existing dhcp host entry in "
> +                                 "network '%s' that matches "
> +                                 "\"<host mac='%s' name='%s' ip='%s'/>\""),
> +                               def->name, host.mac, host.name,
> +                               ip ? ip : "unknown");
> +                VIR_FREE(ip);
> +                goto cleanup;
> +            }
> +        }
> +        /* add to beginning/end of list */
> +        if (VIR_REALLOC_N(ipdef->hosts, ipdef->nhosts +1) < 0) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +
> +        if (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) {
> +
> +            ipdef->hosts[ipdef->nhosts] = host;
> +            ipdef->nhosts++;
> +            memset(&host, 0, sizeof(host));
> +
> +        } else { /* implied (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) */
> +
> +            memmove(ipdef->hosts + 1, ipdef->hosts,
> +                    sizeof(ipdef->hosts) * ipdef->nhosts);
> +            ipdef->hosts[0] = host;
> +            ipdef->nhosts++;
> +            memset(&host, 0, sizeof(host));
> +        }
> +
> +    } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) {
> +
> +        if (virNetworkDHCPHostDefParse(def->name, ctxt->node, &host, false) < 0)
> +            goto cleanup;
> +
> +        /* find matching entry - all specified attributes must match */
> +        for (ii = 0; ii < ipdef->nhosts; ii++) {
> +            if ((!host.mac ||
> +                 !virMacAddrCompare(host.mac, ipdef->hosts[ii].mac)) &&
> +                (!host.name ||
> +                 STREQ_NULLABLE(host.name, ipdef->hosts[ii].name)) &&
> +                (!VIR_SOCKET_ADDR_VALID(&host.ip) ||
> +                 virSocketAddrEqual(&host.ip, &ipdef->hosts[ii].ip))) {
> +                break;
> +            }
> +        }
> +        if (ii == ipdef->nhosts) {
> +            virReportError(VIR_ERR_OPERATION_INVALID,
> +                           _("couldn't locate a matching dhcp host entry "
> +                             "in network '%s'"), def->name);
> +            goto cleanup;
> +        }
> +
> +        /* remove it */
> +        virNetworkDHCPHostDefClear(&ipdef->hosts[ii]);
> +        memmove(ipdef->hosts + ii, ipdef->hosts + ii + 1,
> +                sizeof(ipdef->hosts) * ipdef->nhosts - ii - 1);
> +        ipdef->nhosts--;
> +        ignore_value(VIR_REALLOC_N(ipdef->hosts, ipdef->nhosts));
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    virNetworkDHCPHostDefClear(&host);
> +    return ret;
>  }
>  
>  static int

  Okay, i think we need to add that patch to be able to actually test
the new API, ACK but if possible clean the 2 things above

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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