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

2 Jul 2008

      Hi Dan,

Nice work!

"Daniel P. Berrange" <berrange@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.

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

Jim Meyering