[libvirt] PATCH: 1/2: port Test driver to network APIs

[Jim Meyering]

Hi Dan,

Nice work!

"Daniel P. Berrange" <berrange at redhat.com> wrote:
> diff -r 77e5fcbd24b7 src/test.c
...
> +static virNetworkObjPtr
> +testLoadNetworkFromFile(virConnectPtr conn,
...
> +    if ((def = virNetworkDefParse(conn, data, filename)) == NULL) {
> +        VIR_FREE(data);
> +        return NULL;
>      }
> +    VIR_FREE(data);

Here's another case where you can avoid a duplicate free:

    def = virNetworkDefParse(conn, data, filename);
    VIR_FREE(data);
    if (def == NULL)
        return NULL;

...
>  static virNetworkPtr testLookupNetworkByUUID(virConnectPtr conn,
>                                             const unsigned char *uuid)
>  {
> -    int i, idx = -1;
> +    virNetworkObjPtr net = NULL;

s/ = NULL//

...
>  static virNetworkPtr testLookupNetworkByName(virConnectPtr conn,
> -                                           const char *name)
> +                                             const char *name)
>  {
> -    int i, idx = -1;
> +    virNetworkObjPtr net = NULL;

Same here.

...
>  static int testNumNetworks(virConnectPtr conn) {
> -    int numInactive = 0, i;
> +    int numActive = 0;
> +    virNetworkObjPtr net;
>      GET_CONNECTION(conn, -1);
>
> -    for (i = 0 ; i < MAX_NETWORKS ; i++) {
> -        if (!privconn->networks[i].active ||
> -            !privconn->networks[i].running)
> -            continue;
> -        numInactive++;
> +    net = privconn->networks;
> +    while (net) {
> +        if (virNetworkIsActive(net))
> +            numActive++;
> +        net = net->next;
>      }
> -    return (numInactive);
> +    return numActive;
>  }
>
>  static int testListNetworks(virConnectPtr conn, char **const names, int nnames) {
> -    int n = 0, i;
> +    int n = 0;
> +    virNetworkObjPtr net;
>      GET_CONNECTION(conn, -1);
>
> -    for (i = 0, n = 0 ; i < MAX_NETWORKS && n < nnames ; i++) {
> -        if (privconn->networks[i].active &&
> -            privconn->networks[i].running) {
> -            names[n++] = strdup(privconn->networks[i].name);
> -        }
> +    net = privconn->networks;
> +    while (net && n < nnames) {
> +        if (virNetworkIsActive(net))
> +            names[n++] = strdup(net->def->name);

I know this isn't new with your changes, but there seems
to be a potential problem here, when strdup fails.
The resulting NULL pointer looks like it will be dereferenced
at least in virsh.c via cmdNetworkList's use of qsort+namesorter
(it can call this function through virConnectListNetworks).

As a work-around, this could increment "n" only for each
non-NULL pointer:

        if (virNetworkIsActive(net)) {
            char *p = strdup(net->def->name);
            names[n] = p;
            if (p)
                ++n;
        }

...
>  static int testListDefinedNetworks(virConnectPtr conn, char **const names, int nnames) {
> -    int n = 0, i;
> +    int n = 0;
> +    virNetworkObjPtr net;
>      GET_CONNECTION(conn, -1);
>
> -    for (i = 0, n = 0 ; i < MAX_NETWORKS && n < nnames ; i++) {
> -        if (privconn->networks[i].active &&
> -            !privconn->networks[i].running) {
> -            names[n++] = strdup(privconn->networks[i].name);

Same here.

> -        }
> +    net = privconn->networks;
> +    while (net && n < nnames) {
> +        if (!virNetworkIsActive(net))
> +            names[n++] = strdup(net->def->name);

and here.

> +        net = net->next;
>      }
> -    return (n);
> +    return n;
>  }
>
>  static virNetworkPtr testNetworkCreate(virConnectPtr conn, const char *xml) {
> -    int handle = -1;
> -    virNetworkPtr net;
> +    virNetworkDefPtr def;
> +    virNetworkObjPtr net;
>      GET_CONNECTION(conn, NULL);
>
> -    if (xml == NULL) {
> -        testError(conn, NULL, NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
> -        return (NULL);
> -    }
> +    if ((def = virNetworkDefParse(conn, xml, NULL)) == NULL)
> +        return NULL;
>
> -    if (privconn->numNetworks == MAX_NETWORKS) {
> -        testError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("too many networks"));
> -        return (NULL);
> -    }
> +    if ((net = virNetworkAssignDef(conn, &privconn->networks,
> +                                   def)) == NULL)

Don't we need to call virNetworkDefFree(def) before returning?

> +        return NULL;
> +    net->active = 1;
>
> -    if ((handle = testLoadNetworkFromDoc(conn, xml)) < 0)
> -        return (NULL);
> -    privconn->networks[handle].config = 0;
> -
> -    net = virGetNetwork(conn, privconn->networks[handle].name, privconn->networks[handle].uuid);
> -    if (net == NULL) return NULL;
> -    privconn->numNetworks++;
> -    return (net);
> +    return virGetNetwork(conn, def->name, def->uuid);

And before this one, too?  i.e.,

      virNetworkPtr result_net = virGetNetwork(conn, def->name, def->uuid);
      virNetworkDefFree(def);
      return result_net;

>  }
>
>  static virNetworkPtr testNetworkDefine(virConnectPtr conn, const char *xml) {
> -    int handle = -1;
> -    virNetworkPtr net;
> +    virNetworkDefPtr def;
> +    virNetworkObjPtr net;
>      GET_CONNECTION(conn, NULL);
>
> -    if (xml == NULL) {
> -        testError(conn, NULL, NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
> -        return (NULL);
> -    }
> +    if ((def = virNetworkDefParse(conn, xml, NULL)) == NULL)
> +        return NULL;
>
> -    if (privconn->numNetworks == MAX_NETWORKS) {
> -        testError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("too many networks"));
> -        return (NULL);
> -    }
> +    if ((net = virNetworkAssignDef(conn, &privconn->networks,
> +                                   def)) == NULL)

If needed above, then it's needed here, too.

> +        return NULL;
> +    net->persistent = 1;
>
> -    if ((handle = testLoadNetworkFromDoc(conn, xml)) < 0)
> -        return (NULL);
> -
> -    net = virGetNetwork(conn, privconn->networks[handle].name, privconn->networks[handle].uuid);
> -    privconn->networks[handle].config = 1;
> -    if (net == NULL) return NULL;
> -    privconn->numNetworks++;
> -    return (net);
> +    return virGetNetwork(conn, def->name, def->uuid);

And here.

>  }
>
>  static int testNetworkUndefine(virNetworkPtr network) {
...

I'll finish tomorrow.