[libvirt] [PATCH v2 1/3] lxc: refactoring LXC network definition with a sparse array.
John Ferlan
jferlan at redhat.com
Thu Jan 24 22:47:17 UTC 2019
On 1/10/19 10: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 at gmail.com>
> ---
> src/lxc/lxc_native.c | 195 ++++++++++++++++++++++++++++---------------
> 1 file changed, 128 insertions(+), 67 deletions(-)
>
Looks like review "clash" w/ Cole... I had some different ideas, but
similar to Cole's comments about separating things. I did go into more
detail though (as is my norm).
There's way too much going on in one patch here - make quite a few
patches out of this to show the steps to get from point A to point B and
make sure each step along the way compiles and tests properly. I'll give
you a suggestion below.
> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
> index 1eee3fc2bb..8ddcdc8180 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 {
Modifying this would be a separate patch. Make just the struct change
then modify the "lxcNetworkParseData *" usages to be lxcNetworkParseDataPtr.
> 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;
> + size_t nnetworks;
> +} lxcNetworkParseArray;
This too would be a separate, but later patch... I see you followed
Michal's advice about usage of size_t; however, @data should be
@networks. Additionally, the struct should follow convention:
typedef struct _lxcNetworkParseArray lxcNetworkParseArray;
typedef lxcNetworkParseArray *lxcNetworkParseArrayPtr;
struct _lxcNetworkParseArray {
lxcNetworkParseDataPtr *networks;
size_t nnetworks;
};
>
> static int
> lxcAddNetworkRouteDefinition(const char *address,
> @@ -464,7 +470,7 @@ lxcAddNetworkRouteDefinition(const char *address,
> }
>
> static int
> -lxcAddNetworkDefinition(lxcNetworkParseData *data)
> +lxcAddNetworkDefinition(virDomainDefPtr def, lxcNetworkParseData *data)
One argument per line
> {
> 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,parseData 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,93 @@ lxcAddNetworkDefinition(lxcNetworkParseData *data)
> return -1;
> }
>
> +static inline int lxcNetworkHasIndex(const char *entry)
> +{
> + return STRPREFIX(entry, "lxc.net.") && c_isdigit(entry[8]);
> +}
> +
> +static inline int lxcNetworkIndexHasType(const char *entry, const char *type)
> +{
> + return lxcNetworkHasIndex(entry) && virFileHasSuffix(entry, type);
> +}
> +
FWIW: Both of these should be bool methods, but I don't think they're
necessary. Also, my build wasn't happy :
CC lxc/libvirt_driver_lxc_impl_la-lxc_native.lo
lxc/lxc_native.c: In function 'lxcNetworkWalkCallback':
lxc/lxc_native.c:566:19: error: inlining failed in call to
'lxcNetworkIndexHasType': call is unlikely and code size would grow
[-Werror=inline]
static inline int lxcNetworkIndexHasType(const char *entry, const char
*type)
^~~~~~~~~~~~~~~~~~~~~~
lxc/lxc_native.c:636:14: note: called from here
lxcNetworkIndexHasType(name, ".macvlan.mode"))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lxc/lxc_native.c:566:19: error: inlining failed in call to
'lxcNetworkIndexHasType': call is unlikely and code size would grow
[-Werror=inline]
static inline int lxcNetworkIndexHasType(const char *entry, const char
*type)
^~~~~~~~~~~~~~~~~~~~~~
Repeated for 8 or 9 of these calls.
Removing the 'inline' from lxcNetworkIndexHasType made it happy again.
FWIW:
$ gcc --version
gcc (GCC) 8.2.1 20181215 (Red Hat 8.2.1-6)
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
This is one of those welcome to wonderful world of multiple compilers
which decide to squawk and complain about different things.
> 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);
So this can be 0, 1, 2, 3, etc.; however, you're adding 'index + 1'
networks and you're assigning at [index] plus storing index at ->index
for both rather than just for the one that doesn't use index.
So if someone for some strange reason has one entry:
lxc.net.100.type
then you allocate way more than you need... IOW: I think you should
move away from a sparse array idea...
> +
> + if (index >= parseData->nnetworks) {
> + 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;
> + }
I think the code is a bit confusing and would be "better" if it was more
modular. Like I noted earlier generate a few patches to show some steps
to getting to a final result.
So here's my suggestion rework this patch, but take things slower.
Hopefully it makes sense, if not feel free to ask questions...
1. Add a patch to just have the lxcNetworkParseDataPtr get created/used.
2. Extract out the processing of "lxc.network.ipv4" and
"lxc.network.ipv6" into its own method, so that it's not in the middle
of some if () else if () else if () construct. The usage of { } in the
module are totally against our current standard too.
3. Extract out the if then else if ... processing into its own method
*and* only have it manage the suffix part, such as:
suffix = STRSKIP(name, "lxc.network.");
return lxcNetworkParseDataSuffix(suffix, value, parseData);
The method *would* process the "type" as well.
4. Create a method that would only be called if STRPREFIX(name,
"lxc.network."), so that you further isolate things. This should leave
lxcNetworkWalkCallback looking very simple:
static int
lxcNetworkWalkCallback(const char *name,
virConfValuePtr value,
void *data)
{
lxcNetworkParseDataPtr parseData = data;
if (STRPREFIX(name, "lxc.network."))
return lxcNetworkParseDataEntry(name, value, parseData);
return 0;
}
5. Introduce the lxcNetworkParseArrayPtr and all the changes to manage
VIR_ALLOC_N the array and VIR_ALLOC the entry. If a second entry for
"lxc.network.type" is found, use VIR_EXPAND_N and VIR_ALLOC. Use the
nets->networks[0]->index to keep track of which one you're processing.
The method is a little tricky, but it should return the address of
whatever lxcNetworkParseDataPtr is being processed. That way the
lxcNetworkWalkCallback would change to something like:
if (STRPREFIX(name, "lxc.network.")) {
if (!(parseData = lxcNetworkGetParseData(nets, name)))
return -1;
return lxcNetworkParseEntry(name, value, parseData);
}
6. Now introduce the "lxc.net.#" processing. This is also a little
tricky, but not too bad. The helpers that handle the suffix and the IP
processing shouldn't need to change. The callers to those functions just
need to get to the right place. For "lxc.net.", use STRSKIP and
strchr(xxx, '.') + 1 to find the suffix. When allocating, you should
just be able to use VIR_EXPAND_N and VIR_ALLOC directly. Save the sscanf
fetched index in the -> index field so that you can search nnetworks for
the matching index before deciding to expand.
>
> - if (STREQ(name, "lxc.network.type")) {
> - virDomainDefPtr def = parseData->def;
> - size_t networks = parseData->networks;
> - bool privnet = parseData->privnet;
> + parseData->nnetworks = 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->nnetworks - 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->nnetworks;
>
> - /* 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->nnetworks++;
> }
> - 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 +652,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 +671,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 +698,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.nnetworks; 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.nnetworks == 0 && privnet) {
> /* When no network type is provided LXC only adds loopback */
> def->features[VIR_DOMAIN_FEATURE_PRIVNET] = VIR_TRISTATE_SWITCH_ON;
> }
> @@ -672,9 +729,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.nnetworks; i++) {
> + for (i = 0; i < nets.data[i]->nips; i++)
You'll need a 'j' for this inner loop to reference "ips[j]"..
> + VIR_FREE(nets.data[i]->ips[i]);
> + VIR_FREE(nets.data[i]->ips);
> + }
> + for (i = 0; i < nets.nnetworks; i++)
> + VIR_FREE(nets.data[i]);
Why have a second loop on i, just do this in the i loop.
and finally you'll need a VIR_FREE(nets.networks);
John
> return -1;
> }
>
>
More information about the libvir-list
mailing list