Hi Dan,
Nice work!
"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list