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@xxxxxxxxx>
> ---
> 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 networks.parseData 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 :|
