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

Re: [libvirt] [PATCH 1/3] lxc: refactoring LXC network definition with a sparse array.



On 12/28/18 8:01 PM, Julio Faracco wrote:
> LXC was using a single data structure to define all LXC NICs. In terms
> of optimization it is very interesting, but it is not useful when you
> use a index to define networks. After major release 3.0, LXC adopted
> indexes for network definitions. So, this commit adds a sparse vector to
> handle network indexes. Now, networks can be defined in sequence using
> the legacy config. Or using a sparse array, using version 3 with indexes.
> 
> Signed-off-by: Julio Faracco <jcfaracco gmail com>
> ---
>  src/lxc/lxc_native.c | 191 ++++++++++++++++++++++++++++---------------
>  1 file changed, 124 insertions(+), 67 deletions(-)
> 
> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
> index 1eee3fc2bb..860cf87227 100644
> --- a/src/lxc/lxc_native.c
> +++ b/src/lxc/lxc_native.c
> @@ -22,6 +22,7 @@
>  #include <config.h>
>  
>  #include "internal.h"
> +#include "c-ctype.h"
>  #include "lxc_container.h"
>  #include "lxc_native.h"
>  #include "util/viralloc.h"
> @@ -409,8 +410,9 @@ lxcCreateHostdevDef(int mode, int type, const char *data)
>      return hostdev;
>  }
>  
> -typedef struct {
> -    virDomainDefPtr def;
> +typedef struct _lxcNetworkParseData lxcNetworkParseData;
> +typedef lxcNetworkParseData *lxcNetworkParseDataPtr;
> +struct _lxcNetworkParseData {
>      char *type;
>      char *link;
>      char *mac;
> @@ -422,9 +424,13 @@ typedef struct {
>      size_t nips;
>      char *gateway_ipv4;
>      char *gateway_ipv6;
> -    bool privnet;
> -    size_t networks;
> -} lxcNetworkParseData;
> +    size_t index;
> +};
> +
> +typedef struct {
> +    lxcNetworkParseDataPtr *data;
> +    int networks;

This is in fact an array and its size. For that we tend to use X name
for array and 'nX' for the size. So how about 'networks' and
'nnetworks'? Also, s/int/size_t/.

> +} lxcNetworkParseArray;
>  
>  static int
>  lxcAddNetworkRouteDefinition(const char *address,
> @@ -464,7 +470,7 @@ lxcAddNetworkRouteDefinition(const char *address,
>  }
>  
>  static int
> -lxcAddNetworkDefinition(lxcNetworkParseData *data)
> +lxcAddNetworkDefinition(virDomainDefPtr def, lxcNetworkParseData *data)
>  {
>      virDomainNetDefPtr net = NULL;
>      virDomainHostdevDefPtr hostdev = NULL;
> @@ -512,9 +518,9 @@ lxcAddNetworkDefinition(lxcNetworkParseData *data)
>                                           &hostdev->source.caps.u.net.ip.nroutes) < 0)
>                  goto error;
>  
> -        if (VIR_EXPAND_N(data->def->hostdevs, data->def->nhostdevs, 1) < 0)
> +        if (VIR_EXPAND_N(def->hostdevs, def->nhostdevs, 1) < 0)
>              goto error;
> -        data->def->hostdevs[data->def->nhostdevs - 1] = hostdev;
> +        def->hostdevs[def->nhostdevs - 1] = hostdev;
>      } else {
>          if (!(net = lxcCreateNetDef(data->type, data->link, data->mac,
>                                      data->flag, data->macvlanmode,
> @@ -536,9 +542,9 @@ lxcAddNetworkDefinition(lxcNetworkParseData *data)
>                                           &net->guestIP.nroutes) < 0)
>                  goto error;
>  
> -        if (VIR_EXPAND_N(data->def->nets, data->def->nnets, 1) < 0)
> +        if (VIR_EXPAND_N(def->nets, def->nnets, 1) < 0)
>              goto error;
> -        data->def->nets[data->def->nnets - 1] = net;
> +        def->nets[def->nnets - 1] = net;
>      }
>  
>      return 1;
> @@ -552,51 +558,89 @@ lxcAddNetworkDefinition(lxcNetworkParseData *data)
>      return -1;
>  }
>  
> +#define lxcNetworkHasIndex(entry) \
> +    (STRPREFIX(entry, "lxc.net.") && c_isdigit(entry[8]))
> +
> +#define lxcNetworkIndexHasType(entry, type) \
> +    (lxcNetworkHasIndex(entry) && virFileHasSuffix(entry, type))

I rather see these as inline functions.

> +
>  static int
>  lxcNetworkWalkCallback(const char *name, virConfValuePtr value, void *data)
>  {
> -    lxcNetworkParseData *parseData = data;
> -    int status;
> +    lxcNetworkParseArray *parseData = data;
> +    size_t index;
> +
> +    /* Managing memory and indexes for version 3.0 */
> +    if (lxcNetworkHasIndex(name)) {
> +        sscanf(name, "lxc.net.%zu", &index);
> +
> +        if (index >= parseData->networks) {
> +            if (!parseData->data) {
> +                if (VIR_ALLOC_N(parseData->data, index + 1) < 0)
> +                    return -1;
> +            } else {
> +                if (VIR_REALLOC_N(parseData->data, index + 1) < 0)
> +                    return -1;

This is allocated but released only on 'error'.

> +            }
>  
> -    if (STREQ(name, "lxc.network.type")) {
> -        virDomainDefPtr def = parseData->def;
> -        size_t networks = parseData->networks;
> -        bool privnet = parseData->privnet;
> +            parseData->networks = index + 1;
> +        }
>  
> -        /* Store the previous NIC */
> -        status = lxcAddNetworkDefinition(parseData);
> +        if (!parseData->data[index]) {
> +            if (VIR_ALLOC(parseData->data[index]) < 0)
> +                return -1;
> +        }
> +    } else {
> +        /* Indexes can be 0 when a network is defined */
> +        index = parseData->networks - 1;
> +    }
>  
> -        if (status < 0)
> -            return -1;
> -        else if (status > 0)
> -            networks++;
> -        else if (parseData->type != NULL && STREQ(parseData->type, "none"))
> -            privnet = false;
> +    if (STREQ(name, "lxc.network.type")) {
> +        /* A new NIC will be added */
> +        index = parseData->networks;
>  
> -        /* clean NIC to store a new one */
> -        memset(parseData, 0, sizeof(*parseData));
> +        if (!parseData->data) {
> +            if (VIR_ALLOC_N(parseData->data, index + 1) < 0)
> +                return -1;
> +        } else {
> +            if (VIR_REALLOC_N(parseData->data, index + 1) < 0)
> +                return -1;
> +        }
>  
> -        parseData->def = def;
> -        parseData->networks = networks;
> -        parseData->privnet = privnet;
> +        if (VIR_ALLOC(parseData->data[index]) < 0)
> +            return -1;
>  
>          /* Keep the new value */
> -        parseData->type = value->str;
> +        parseData->data[index]->type = value->str;
> +        parseData->data[index]->index = index;
> +
> +        /* Network interface added */
> +        parseData->networks++;
>      }
> -    else if (STREQ(name, "lxc.network.link"))
> -        parseData->link = value->str;
> -    else if (STREQ(name, "lxc.network.hwaddr"))
> -        parseData->mac = value->str;
> -    else if (STREQ(name, "lxc.network.flags"))
> -        parseData->flag = value->str;
> -    else if (STREQ(name, "lxc.network.macvlan.mode"))
> -        parseData->macvlanmode = value->str;
> -    else if (STREQ(name, "lxc.network.vlan.id"))
> -        parseData->vlanid = value->str;
> -    else if (STREQ(name, "lxc.network.name"))
> -        parseData->name = value->str;
> -    else if (STREQ(name, "lxc.network.ipv4") ||
> -             STREQ(name, "lxc.network.ipv6")) {
> +    else if (lxcNetworkIndexHasType(name, ".type"))
> +        parseData->data[index]->type = value->str;
> +    else if (STREQ(name, "lxc.network.link") ||
> +             lxcNetworkIndexHasType(name, ".link"))
> +        parseData->data[index]->link = value->str;
> +    else if (STREQ(name, "lxc.network.hwaddr") ||
> +             lxcNetworkIndexHasType(name, ".hwaddr"))
> +        parseData->data[index]->mac = value->str;
> +    else if (STREQ(name, "lxc.network.flags") ||
> +             lxcNetworkIndexHasType(name, ".flags"))
> +        parseData->data[index]->flag = value->str;
> +    else if (STREQ(name, "lxc.network.macvlan.mode") ||
> +             lxcNetworkIndexHasType(name, ".macvlan.mode"))
> +        parseData->data[index]->macvlanmode = value->str;
> +    else if (STREQ(name, "lxc.network.vlan.id") ||
> +             lxcNetworkIndexHasType(name, ".vlan.id"))
> +        parseData->data[index]->vlanid = value->str;
> +    else if (STREQ(name, "lxc.network.name") ||
> +             lxcNetworkIndexHasType(name, ".name"))
> +        parseData->data[index]->name = value->str;
> +    else if ((STREQ(name, "lxc.network.ipv4") ||
> +              STREQ(name, "lxc.network.ipv6")) ||
> +             (lxcNetworkIndexHasType(name, ".ipv4") ||
> +              lxcNetworkIndexHasType(name, ".ipv6"))) {
>          int family = AF_INET;
>          char **ipparts = NULL;
>          virNetDevIPAddrPtr ip = NULL;
> @@ -604,7 +648,8 @@ lxcNetworkWalkCallback(const char *name, virConfValuePtr value, void *data)
>          if (VIR_ALLOC(ip) < 0)
>              return -1;
>  
> -        if (STREQ(name, "lxc.network.ipv6"))
> +        if (STREQ(name, "lxc.network.ipv6") ||
> +            lxcNetworkIndexHasType(name, ".ipv6"))
>              family = AF_INET6;
>  
>          ipparts = virStringSplit(value->str, "/", 2);
> @@ -622,15 +667,19 @@ lxcNetworkWalkCallback(const char *name, virConfValuePtr value, void *data)
>  
>          virStringListFree(ipparts);
>  
> -        if (VIR_APPEND_ELEMENT(parseData->ips, parseData->nips, ip) < 0) {
> +        if (VIR_APPEND_ELEMENT(parseData->data[index]->ips,
> +                               parseData->data[index]->nips, ip) < 0) {
>              VIR_FREE(ip);
>              return -1;
>          }
> -    } else if (STREQ(name, "lxc.network.ipv4.gateway")) {
> -        parseData->gateway_ipv4 = value->str;
> -    } else if (STREQ(name, "lxc.network.ipv6.gateway")) {
> -        parseData->gateway_ipv6 = value->str;
> -    } else if (STRPREFIX(name, "lxc.network")) {
> +    } else if (STREQ(name, "lxc.network.ipv4.gateway") ||
> +               lxcNetworkIndexHasType(name, ".ipv4.gateway")) {
> +        parseData->data[index]->gateway_ipv4 = value->str;
> +    } else if (STREQ(name, "lxc.network.ipv6.gateway") ||
> +               lxcNetworkIndexHasType(name, ".ipv6.gateway")) {
> +        parseData->data[index]->gateway_ipv6 = value->str;
> +    } else if (STRPREFIX(name, "lxc.network") ||
> +               lxcNetworkHasIndex(name)) {
>          VIR_WARN("Unhandled network property: %s = %s",
>                   name,
>                   value->str);
> @@ -645,25 +694,29 @@ lxcConvertNetworkSettings(virDomainDefPtr def, virConfPtr properties)
>      int status;
>      int result = -1;
>      size_t i;
> -    lxcNetworkParseData data = {def, NULL, NULL, NULL, NULL,
> -                                NULL, NULL, NULL, NULL, 0,
> -                                NULL, NULL, true, 0};
> +    bool privnet = true;
> +    lxcNetworkParseArray nets = {NULL, 0};
>  
> -    if (virConfWalk(properties, lxcNetworkWalkCallback, &data) < 0)
> +    if (virConfWalk(properties, lxcNetworkWalkCallback, &nets) < 0)
>          goto error;
>  
> +    for (i = 0; i < nets.networks; i++) {
> +        lxcNetworkParseDataPtr data = nets.data[i];
>  
> -    /* Add the last network definition found */
> -    status = lxcAddNetworkDefinition(&data);
> +        /* It needs to guarantee that index exists. */
> +        /* Since there is a sparse array. */
> +        if (data) {
> +            /* Add network definitions */
> +            status = lxcAddNetworkDefinition(def, data);
>  
> -    if (status < 0)
> -        goto error;
> -    else if (status > 0)
> -        data.networks++;
> -    else if (data.type != NULL && STREQ(data.type, "none"))
> -        data.privnet = false;
> +            if (status < 0)
> +                goto error;
> +            else if (data->type != NULL && STREQ(data->type, "none"))
> +                privnet = false;
> +        }
> +    }
>  
> -    if (data.networks == 0 && data.privnet) {
> +    if (nets.networks == 0 && privnet) {
>          /* When no network type is provided LXC only adds loopback */
>          def->features[VIR_DOMAIN_FEATURE_PRIVNET] = VIR_TRISTATE_SWITCH_ON;
>      }
> @@ -672,9 +725,13 @@ lxcConvertNetworkSettings(virDomainDefPtr def, virConfPtr properties)
>      return result;
>  
>   error:
> -    for (i = 0; i < data.nips; i++)
> -        VIR_FREE(data.ips[i]);
> -    VIR_FREE(data.ips);
> +    for (i = 0; i <= nets.networks; i++) {

There is no reason to go to including nets.networks. In fact, there
should be nothing at nets.networks index as it is past allocated array
possibly leading to a crash.

> +        for (i = 0; i < nets.data[i]->nips; i++)
> +            VIR_FREE(nets.data[i]->ips[i]);
> +        VIR_FREE(nets.data[i]->ips);
> +    }
> +    for (i = 0; i <= nets.networks; i++)
> +        VIR_FREE(nets.data[i]);
>      return -1;
>  }
>  
> 

The rest of the patches look okay.

Michal


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