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

[Julio Faracco]

Em qua., 29 de jan. de 2020 às 08:38, Daniel P. Berrangé
<berrange at redhat.com> escreveu:
>
> 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 at gmail.com>
> > ---
> >  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.

I think the main problem here with success code path is loosing IPs
when we free networks.parseData. The failure code path is not
relevant. We can free everything.

>
> 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 :|
>