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

Re: [libvirt] [PATCH] read network config in OpenVZ driver



On Wed, Sep 17, 2008 at 08:28:56PM +0400, Evgeniy Sokolov wrote:
> 
> I attached patch without hunk which was commited by Daniel.
> Please commit if you are agree with it.

A few comments inline..

> Index: openvz_conf.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/openvz_conf.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 openvz_conf.c
> --- openvz_conf.c	5 Sep 2008 15:00:14 -0000	1.38
> +++ openvz_conf.c	8 Sep 2008 11:38:48 -0000
> @@ -151,6 +151,148 @@ char *openvzMacToString(const unsigned c
>      return strdup(str);
>  }
>  
> +/*parse MAC from view: 00:18:51:8F:D9:F3
> +  return -1 - error
> +          0 - OK
> +*/
> +int openvzParseMac(const char *macaddr, unsigned char *mac)
> +{
> +    int ret;
> +    ret = sscanf((const char *)macaddr, "%02X:%02X:%02X:%02X:%02X:%02X",
> +               (unsigned int*)&mac[0],
> +               (unsigned int*)&mac[1],
> +               (unsigned int*)&mac[2],
> +               (unsigned int*)&mac[3],
> +               (unsigned int*)&mac[4],
> +               (unsigned int*)&mac[5]) ;
> +    if (ret == 6)
> +        return 0;
> +
> +    return -1;
> +}
> +
> +virDomainNetDefPtr
> +openvzReadNetworkConf(virConnectPtr conn, int veid) {
> +    int ret;
> +    virDomainNetDefPtr net = NULL;
> +    virDomainNetDefPtr new_net;
> +    char temp[4096];
> +    char *token, *saveptr = NULL;
> +
> +    /*parse routing network configuration*
> +     * Sample from config:
> +     *   IP_ADDRESS="1.1.1.1 1.1.1.2"
> +     *   splited IPs by space
> +     */
> +    ret = openvzReadConfigParam(veid, "IP_ADDRESS", temp, sizeof(temp));
> +    if (ret < 0) {
> +        openvzError(conn, VIR_ERR_INTERNAL_ERROR,
> +                 _("Cound not read 'IP_ADDRESS' from config for container %d"),
> +                  veid);
> +        goto error;
> +    } else if (ret > 0) {
> +        token = strtok_r(temp, " ", &saveptr);
> +        while (token != NULL) {
> +            new_net = NULL;
> +            if (VIR_ALLOC(new_net) < 0) {
> +                openvzError(conn, VIR_ERR_NO_MEMORY, _("virDomainNetDefPtr"));
> +                goto error;
> +            }
> +            new_net->next = net;
> +            net = new_net;
> +
> +            net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
> +            net->data.ethernet.ipaddr = strdup(token);
> +
> +            token = strtok_r(NULL, " ", &saveptr);
> +        }
> +    }
> +
> +    /*parse bridge devices*/
> +    /*Sample from config:
> +     *NETIF="ifname=eth10,mac=00:18:51:C1:05:EE,host_ifname=veth105.10,host_mac=00:18:51:8F:D9:F3"
> +     *devices splited by ';'
> +     */
> +    ret = openvzReadConfigParam(veid, "NETIF", temp, sizeof(temp));
> +    if (ret < 0) {
> +        openvzError(conn, VIR_ERR_INTERNAL_ERROR,
> +                     _("Cound not read 'NETIF' from config for container %d"),
> +                     veid);
> +        goto error;
> +    } else if (ret > 0) {
> +        token = strtok_r(temp, ";", &saveptr);
> +        while (token != NULL) {
> +            /*add new device to list*/
> +            new_net = NULL;
> +            if (VIR_ALLOC(new_net) < 0) {
> +                openvzError(conn, VIR_ERR_NO_MEMORY, _("virDomainNetDefPtr"));
> +                goto error;
> +            }
> +            new_net->next = net;
> +            net = new_net;
> +
> +            net->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
> +
> +            char *p = token, *next = token;
> +            char cpy_temp[32];
> +            int len;
> +
> +            /*parse string*/
> +            do {
> +                while (*next != '\0' && *next != ',') next++;
> +                if (STREQLEN("ifname=", p, 7)) {

You can optionally use STRPREFIX(p, "ifname=")  for this kind of scenario

> +                    p += 7;
> +                    len = next - p;
> +                    if (len > 16) {
> +                        openvzError(conn, VIR_ERR_INTERNAL_ERROR,
> +                                _("Too long network device name"));
> +                        goto error;
> +                    }
> +                    strncpy(cpy_temp, p, len);
> +                    cpy_temp[len] = '\0';
> +                    net->data.bridge.brname = strdup(cpy_temp);
> +
> +                    if (net->data.bridge.brname == NULL) {
> +                        openvzError(conn, VIR_ERR_NO_MEMORY, _("Can't strdup"));
> +                        goto error;
> +                    }

Why do you need to use the intermediate buffer for the value ? It 
would be a little simpler todo something like

                len = next - p;
                if (VIR_ALLOC_N(net->data.bridge.brname, len+1) < 0)
                     goto no_memory;
                strncpy(net->data.bridge.brname, p, len);
                net->data.bridge.brname[len] = '\0';

So you're not artifically restricting the max length of the
network name.

> +
> +                } else if (STREQLEN("host_ifname=", p, 12)) {
> +                    p += 12;
> +                    //skip in libvirt
> +                } else if (STREQLEN("mac=", p, 4)) {
> +                    p += 4;
> +                    len = next - p;
> +                    if (len != 17) { //should be 17
> +                        openvzError(conn, VIR_ERR_INTERNAL_ERROR,
> +                              _("Wrong length MAC address"));
> +                        goto error;
> +                    }
> +                    strncpy(cpy_temp, p, len);
> +                    cpy_temp[len] = '\0';
> +                    if (openvzParseMac(cpy_temp, net->mac)<0) {
> +                        openvzError(conn, VIR_ERR_INTERNAL_ERROR,
> +                              _("Wrong MAC address"));
> +                        goto error;
> +                    }
> +                } else if (STREQLEN("host_mac=", p, 9)) {
> +                    p += 9;
> +                    //skip in libvirt
> +                }
> +                p = ++next;
> +            } while (p < token + strlen(token));
> +
> +            token = strtok_r(NULL, ";", &saveptr);
> +        }
> +    }
> +
> +    return net;

no_memory:
      openvzError(conn, VIR_ERR_NO_MEMORY, NULL);
     /* fallthrough to error: */

> +error:
> +    virDomainNetDefFree(net);
> +    return NULL;
> +}
> +
> +
>  /* Free all memory associated with a openvz_driver structure */
>  void
>  openvzFreeDriver(struct openvz_driver *driver)
> @@ -243,6 +385,8 @@ int openvzLoadDomains(struct openvz_driv
>  
>          /* XXX load rest of VM config data .... */
>  
> +        dom->def->nets = openvzReadNetworkConf(NULL, veid);
> +
>          if (prev) {
>              prev->next = dom;
>          } else {
> Index: openvz_conf.h
> ===================================================================
> RCS file: /data/cvs/libvirt/src/openvz_conf.h,v
> retrieving revision 1.12
> diff -u -p -r1.12 openvz_conf.h
> --- openvz_conf.h	5 Sep 2008 14:10:58 -0000	1.12
> +++ openvz_conf.h	8 Sep 2008 11:38:48 -0000
> @@ -60,6 +60,9 @@ void openvzFreeDriver(struct openvz_driv
>  int strtoI(const char *str);
>  int openvzCheckEmptyMac(const unsigned char *mac);
>  char *openvzMacToString(const unsigned char *mac);
> +int openvzParseMac(const char *macaddr, unsigned char *mac);
>  int openvzSetDefinedUUID(int vpsid, unsigned char *uuid);
> +virDomainNetDefPtr openvzReadNetworkConf(virConnectPtr conn, int veid);

Since this API is only ever called from within openvz_conf.c, there's
no need to have it in the header. Just make it a static function in
openvz_conf.c


Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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