Re: [PATCH v2 2/6] lxc: Rebase lxcNetworkParseData pointers to use new structures.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 3/4/19 8:54 PM, Julio Faracco wrote:
> This commit refactor the code logic to introduce new array structures
> instead of single lxcNetworkParseData struct.
> 
> Signed-off-by: Julio Faracco <jcfaracco@xxxxxxxxx>
> ---
>  src/lxc/lxc_native.c | 94 +++++++++++++++-----------------------------
>  1 file changed, 32 insertions(+), 62 deletions(-)
> 

lxc/lxc_native.c:679:27: error: unused variable 'networks'
      [-Werror,-Wunused-variable]
    lxcNetworkParseArray *networks = data;
                          ^
1 error generated.
make[5]: *** [Makefile:10902:
lxc/libvirt_driver_lxc_impl_la-lxc_native.lo] Error 1

> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
> index bf82cd1e98..a6afbbe865 100644
> --- a/src/lxc/lxc_native.c
> +++ b/src/lxc/lxc_native.c
> @@ -484,7 +484,7 @@ lxcAddNetworkRouteDefinition(const char *address,
>  }
>  
>  static int
> -lxcAddNetworkDefinition(lxcNetworkParseData *data)
> +lxcAddNetworkDefinition(virDomainDefPtr def, lxcNetworkParseDataPtr data)

One argument per line...

Still I think too much going on at a time...  Removal of the @def from
lxcNetworkParseData should be separated.

>  {
>      virDomainNetDefPtr net = NULL;
>      virDomainHostdevDefPtr hostdev = NULL;
> @@ -532,9 +532,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,
> @@ -556,9 +556,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;
>      }
>  
>      return 1;
> @@ -572,44 +572,10 @@ lxcAddNetworkDefinition(lxcNetworkParseData *data)
>      return -1;
>  }
>  
> -
> -static int
> -lxcNetworkParseDataType(virConfValuePtr value,
> -                        lxcNetworkParseData *parseData)
> -{

Another example of too much happening at one time...  Strange to see
this "go away"

> -    virDomainDefPtr def = parseData->def;
> -    size_t networks = parseData->networks;
> -    bool privnet = parseData->privnet;
> -    int status;
> -
> -    /* Store the previous NIC */
> -    status = lxcAddNetworkDefinition(parseData);
> -
> -    if (status < 0)
> -        return -1;
> -    else if (status > 0)
> -        networks++;
> -    else if (parseData->type != NULL && STREQ(parseData->type, "none"))
> -        privnet = false;
> -
> -    /* clean NIC to store a new one */
> -    memset(parseData, 0, sizeof(*parseData));
> -
> -    parseData->def = def;
> -    parseData->networks = networks;
> -    parseData->privnet = privnet;
> -
> -    /* Keep the new value */
> -    parseData->type = value->str;
> -
> -    return 0;
> -}
> -
> -
>  static int
>  lxcNetworkParseDataIPs(const char *name,
>                         virConfValuePtr value,
> -                       lxcNetworkParseData *parseData)
> +                       lxcNetworkParseDataPtr parseData)

This hunk would be in previous patch

>  {
>      int family = AF_INET;
>      char **ipparts = NULL;
> @@ -648,14 +614,13 @@ lxcNetworkParseDataIPs(const char *name,
>  static int
>  lxcNetworkParseDataSuffix(const char *entry,
>                            virConfValuePtr value,
> -                          lxcNetworkParseData *parseData)
> +                          lxcNetworkParseDataPtr parseData)

Same - previous patch

>  {
>      int elem = virLXCNetworkConfigEntryTypeFromString(entry);
>  
>      switch (elem) {
>      case VIR_LXC_NETWORK_CONFIG_TYPE:
> -        if (lxcNetworkParseDataType(value, parseData) < 0)
> -            return -1;
> +        parseData->type = value->str;

It's confusing why lxcNetworkParseDataType is no longer necessary

>          break;
>      case VIR_LXC_NETWORK_CONFIG_LINK:
>          parseData->link = value->str;
> @@ -700,7 +665,7 @@ lxcNetworkParseDataSuffix(const char *entry,
>  static int
>  lxcNetworkParseDataEntry(const char *name,
>                           virConfValuePtr value,
> -                         lxcNetworkParseData *parseData)
> +                         lxcNetworkParseDataPtr parseData)

Again, previous patch

>  {
>      const char *suffix = STRSKIP(name, "lxc.network.");
>  
> @@ -711,7 +676,8 @@ lxcNetworkParseDataEntry(const char *name,
>  static int
>  lxcNetworkWalkCallback(const char *name, virConfValuePtr value, void *data)
>  {
> -    lxcNetworkParseData *parseData = data;
> +    lxcNetworkParseArray *networks = data;

As the compiler tells you @networks is not used

> +    lxcNetworkParseDataPtr parseData = NULL;

If you took this one patch at a time, then @data "used" to be a pointer
to a structure of mostly empty data with a filled in @def and this
changes it to a NULL structure, which doesn't feel quite right.  That
means lxcNetworkParseDataEntry gets called w/ NULL parameter in 3rd
parameter.

Going to stop here and wait for the next series... I think removing the
@networks and @privnet from _lxcNetworkParseData should be one step and
then the removal of @def another step.

John

>  
>      if (STRPREFIX(name, "lxc.network."))
>          return lxcNetworkParseDataEntry(name, value, parseData);
> @@ -724,26 +690,26 @@ lxcConvertNetworkSettings(virDomainDefPtr def, virConfPtr properties)
>  {
>      int status;
>      int result = -1;
> -    size_t i;
> -    lxcNetworkParseData data = {def, NULL, NULL, NULL, NULL,
> -                                NULL, NULL, NULL, NULL, 0,
> -                                NULL, NULL, true, 0};
> +    size_t i, j;
> +    bool privnet = true;
> +    lxcNetworkParseArray nets = {NULL, 0};
>  
> -    if (virConfWalk(properties, lxcNetworkWalkCallback, &data) < 0)
> +    if (virConfWalk(properties, lxcNetworkWalkCallback, &nets) < 0)
>          goto error;
>  
> +    for (i = 0; i < nets.nnetworks; i++) {
> +        lxcNetworkParseDataPtr data = nets.data[i];
>  
> -    /* Add the last network definition found */
> -    status = lxcAddNetworkDefinition(&data);
> +        /* Add network definitions */
> +        status = lxcAddNetworkDefinition(def, 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 (nets.nnetworks == 0 && privnet) {
>          /* When no network type is provided LXC only adds loopback */
>          def->features[VIR_DOMAIN_FEATURE_PRIVNET] = VIR_TRISTATE_SWITCH_ON;
>      }
> @@ -752,9 +718,13 @@ lxcConvertNetworkSettings(virDomainDefPtr def, virConfPtr properties)
>      return result;
>  
>   error:
> -    for (i = 0; i < data.nips; i++)
> -        VIR_FREE(data.ips[i]);
> -    VIR_FREE(data.ips);
> +    for (i = 0; i < nets.nnetworks; i++) {
> +        for (j = 0; j < nets.data[i]->nips; j++)
> +            VIR_FREE(nets.data[i]->ips[j]);
> +        VIR_FREE(nets.data[i]->ips);
> +    }
> +    for (i = 0; i < nets.nnetworks; i++)
> +        VIR_FREE(nets.data[i]);
>      return -1;
>  }
>  
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux