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@xxxxxxxxx>
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
