On 01/10/2019 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(a)gmail.com>
Hi Julio, I tried to build this patch for review but I'm seeing a lot of
errors like this:
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"))
Probably just drop the inline annotation
More broadly though this patch is really hard to review, seems like it
is doing a lot of different cleanups and improvements mixed together. I
didn't dig into it too deeply but:
---
src/lxc/lxc_native.c | 195 ++++++++++++++++++++++++++++---------------
1 file changed, 128 insertions(+), 67 deletions(-)
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 {
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;
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;
}
Converting this function to take an explicit 'def' could be one prep
patch, even if it's redundant because 'data' contains a reference. It
will reduce diffs of later patches and be easier to review in isolation
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);
+}
+
Addition and usage of these look like they could be their own patch and
easy to review.
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->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;
+ }
- 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;
All these lines that are essentially s/parseData/parseData->data[index]/
you could eliminate the churn by renaming parseData to something else in
this context, and set parseData = FOO->data[index]
Stuff like that will help reduce the size of the diff, make the no-op
cleanups easy to review, and isolate the actual behavior change to their
own patches
Thanks,
Cole