3:13 p.m.
Hi Dan,
Nice work!
"Daniel P. Berrange" <berrange(a)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.