Re: [PATCH v3 1/4] lxc: refactor lxcNetworkParseData pointers to use new structures

On Tue, Jan 28, 2020 at 10:54:08PM -0300, Julio Faracco wrote: > Struct lxcNetworkParseData is being used as a single pointer which > iterates through LXC config lines. It means that it will be applied as a > network each time that a new type appears. After, the same struct is > used to populate a new network interface. This commit changes this logic > to multiple lxcNetworkParseData to move this strcuture to an array. It > makes more sense if we are using indexes to fill interface settings. > This is better to improve code clarity. > > This commit still introduces *Legacy() functions to keep support of > network old style definitions. > > Signed-off-by: Julio Faracco <jcfaracco(a)gmail.com&gt; > --- > src/lxc/lxc_native.c | 129 +++++++++++++++++++++++-------------------- > 1 file changed, 68 insertions(+), 61 deletions(-) > > diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c > index dd2345c324..b101848c09 100644 > --- a/src/lxc/lxc_native.c > +++ b/src/lxc/lxc_native.c > @@ -702,35 +703,41 @@ static int > lxcConvertNetworkSettings(virDomainDefPtr def, virConfPtr properties) > { > int status; > - size_t i; > - lxcNetworkParseData data = {def, NULL, NULL, NULL, NULL, > - NULL, NULL, NULL, NULL, 0, > - NULL, NULL, true, 0}; > + bool privnet = true; > + size_t i, j; > + lxcNetworkParseDataArray networks = {0, NULL}; > + > + networks.parseData = g_new0(lxcNetworkParseDataPtr, 1); > > - if (virConfWalk(properties, lxcNetworkWalkCallback, &data) < 0) > + if (virConfWalk(properties, lxcNetworkWalkCallback, &networks) < 0) > goto error; > > + for (i = 0; i < networks.ndata; i++) { > + lxcNetworkParseDataPtr data = networks.parseData[i]; > + data->def = def; > > - /* Add the last network definition found */ > - status = lxcAddNetworkDefinition(&data); > + status = lxcAddNetworkDefinition(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 (networks.ndata == 0 && privnet) { > /* When no network type is provided LXC only adds loopback */ > def->features[VIR_DOMAIN_FEATURE_PRIVNET] = VIR_TRISTATE_SWITCH_ON; > } > return 0; > > error: > - for (i = 0; i < data.nips; i++) > - VIR_FREE(data.ips[i]); > - VIR_FREE(data.ips); > + for (i = 0; i < networks.ndata; i++) { > + lxcNetworkParseDataPtr data = networks.parseData[i]; > + for (j = 0; j < data->nips; j++) > + VIR_FREE(data->ips[j]); > + VIR_FREE(data->ips); > + } > + VIR_FREE(networks.parseData); > return -1; > } Unfortunately I noticed some memory leaks in this method reported by valgrind Check it with this: ./run valgrind --leak-check=full --show-possibly-lost=no \ ./tests/lxcconf2xmltest Essentially we're not free'ing c in the success code path, only the failure code path. The failure codepath also fails to free the 'lxcNetworkParseDataPtr' instance.

Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|