"Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> This patch ports the Test driver over to the domain XML apis.
...
> docs/testdomfc4.xml | 2
> src/test.c | 1020 ++++++++++++++++++------------------------------
> tests/read-non-seekable | 3
> tests/testutils.c | 3
> tests/virshtest.c | 4
> 5 files changed, 391 insertions(+), 641 deletions(-)
...
Hi Dan,
Nice work. This will make testing a lot easier.
I've made a few suggestions, spotted some new bugs
and a couple in moved code.
> +static virCapsPtr
> +testBuildCapabilities(virConnectPtr conn) {
> + virCapsPtr caps;
> + virCapsGuestPtr guest;
> + const char *guest_types[] = { "hvm", "xen" };
you can sneak one more "const" in there:
const char *const guest_types[] = { "hvm", "xen" };
> + int i;
> + GET_CONNECTION(conn);
> +
> + if ((caps = virCapabilitiesNew(TEST_MODEL, 0, 0)) == NULL)
> + goto no_memory;
> +
> + if (virCapabilitiesAddHostFeature(caps, "pae") < 0)
> + goto no_memory;
> + if (virCapabilitiesAddHostFeature(caps ,"nonpae") < 0)
> + goto no_memory;
> +
> + for (i = 0; i < privconn->numCells; ++i) {
> + if (virCapabilitiesAddHostNUMACell(caps, i, privconn->cells[i].numCpus,
> + privconn->cells[i].cpus) < 0)
> + goto no_memory;
> + }
> +
> + for (i = 0; i < (sizeof(guest_types)/sizeof(guest_types[0])); ++i) {
With ARRAY_CARDINALITY it's more readable:
for (i = 0; i < ARRAY_CARDINALITY(guest_types); ++i) {
> + if ((guest = virCapabilitiesAddGuest(caps,
> + guest_types[i],
> + TEST_MODEL,
> + TEST_MODEL_WORDSIZE,
> + TEST_EMULATOR,
> + NULL,
> + 0,
> + NULL)) == NULL)
> + goto no_memory;
...
> @@ -538,11 +372,17 @@
> xmlNodePtr *domains, *networks = NULL;
> xmlXPathContextPtr ctxt = NULL;
> virNodeInfoPtr nodeInfo;
> + virDomainObjPtr dom;
> + virNetworkObjPtr net;
> testConnPtr privconn;
> if (VIR_ALLOC(privconn) < 0) {
> testError(NULL, NULL, NULL, VIR_ERR_NO_MEMORY, "testConn");
> return VIR_DRV_OPEN_ERROR;
> }
> + conn->privateData = privconn;
> +
> + if (!(privconn->caps = testBuildCapabilities(conn)))
> + goto error;
>
> if ((fd = open(file, O_RDONLY)) < 0) {
> testError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("loading host definition file"));
> @@ -570,10 +410,7 @@
> goto error;
> }
>
> -
> - conn->privateData = privconn;
> privconn->nextDomID = 1;
> - privconn->numDomains = 0;
> privconn->numCells = 0;
> strncpy(privconn->path, file, PATH_MAX-1);
> privconn->path[PATH_MAX-1] = '\0';
> @@ -645,39 +482,35 @@
> goto error;
> }
>
> +
> ret = virXPathNodeSet("/node/domain", ctxt, &domains);
> - if (ret < 0) {
> - testError(NULL, NULL, NULL, VIR_ERR_XML_ERROR, _("node domain list"));
> - goto error;
> + if (ret > 0) {
> + for (i = 0 ; i < ret ; i++) {
> + xmlChar *netFile = xmlGetProp(domains[i], BAD_CAST "file");
This needs to handle xmlGetProp failure.
[the bug was present before this change: just indented]
> + char *absFile = testBuildFilename(file, (const char *)netFile);
> + VIR_FREE(netFile);
> + if (!absFile) {
> + testError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("resolving domain filename"));
> + goto error;
...
> - for (i = 0 ; i < ret ; i++) {
> - xmlChar *domFile = xmlGetProp(domains[i], BAD_CAST "file");
> - char *absFile = testBuildFilename(file, (const char *)domFile);
> - int domid = privconn->nextDomID++, handle;
> - VIR_FREE(domFile);
> - if (!absFile) {
> - testError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("resolving domain filename"));
> - goto error;
...
> static virDomainPtr
> -testDomainCreateLinux(virConnectPtr conn, const char *xmlDesc,
> +testDomainCreateLinux(virConnectPtr conn, const char *xml,
> unsigned int flags ATTRIBUTE_UNUSED)
> {
...
> - dom = virGetDomain(conn, privconn->domains[handle].name, privconn->domains[handle].uuid);
> - if (dom == NULL) return NULL;
> - privconn->numDomains++;
> - return (dom);
> + ret = virGetDomain(conn, def->name, def->uuid);
Handle virGetDomain failure.
> + ret->id = def->id;
> + return ret;
> }
>
> static virDomainPtr testLookupDomainByID(virConnectPtr conn,
> int id)
> {
...
> + ret = virGetDomain(conn, dom->def->name, dom->def->uuid);
Likewise. Handle virGetDomain failure.
> + ret->id = dom->def->id;
> + return ret;
> }
>
> static virDomainPtr testLookupDomainByUUID(virConnectPtr conn,
> const unsigned char *uuid)
> {
...
> + ret = virGetDomain(conn, dom->def->name, dom->def->uuid);
Likewise.
> + ret->id = dom->def->id;
> + return ret;
> }
>
> static virDomainPtr testLookupDomainByName(virConnectPtr conn,
> const char *name)
> {
...
> + ret = virGetDomain(conn, dom->def->name, dom->def->uuid);
Likewise.
> + ret->id = dom->def->id;
> + return ret;
> }
...
> @@ -1215,8 +970,10 @@
> {
> char *xml;
> char magic[15];
> - int fd, len, ret, domid;
> - GET_CONNECTION(conn, -1);
> + int fd, len;
> + virDomainDefPtr def;
> + virDomainObjPtr dom;
> + GET_CONNECTION(conn);
>
> if ((fd = open(path, O_RDONLY)) < 0) {
> testError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> @@ -1260,10 +1017,19 @@
> }
> xml[len] = '\0';
> close(fd);
> - domid = privconn->nextDomID++;
> - ret = testLoadDomainFromDoc(conn, domid, xml);
> +
> + if ((def = virDomainDefParse(conn, privconn->caps, xml, NULL)) == NULL) {
> + VIR_FREE(xml);
> + return -1;
> + }
> VIR_FREE(xml);
You can save a VIR_FREE:
def = virDomainDefParse(conn, privconn->caps, xml, NULL);
VIR_FREE(xml);
if (def == NULL)
return -1;
...
> static int testListDefinedDomains(virConnectPtr conn,
> char **const names,
> int maxnames) {
> - int n = 0, i;
> - GET_CONNECTION(conn, -1);
> + int n = 0;
> + virDomainObjPtr dom;
> + GET_CONNECTION(conn);
>
> - for (i = 0, n = 0 ; i < MAX_DOMAINS && n < maxnames ; i++) {
> - if (privconn->domains[i].active &&
> - privconn->domains[i].info.state == VIR_DOMAIN_SHUTOFF) {
> - names[n++] = strdup(privconn->domains[i].name);
> - }
> + dom = privconn->domains;
> + while (dom && n < maxnames) {
> + if (virDomainIsActive(dom))
> + names[n++] = strdup(dom->def->name);
This could be the same problem I pointed out in testListDefinedNetworks:
when strdup fails, some caller may dereference the NULL pointer
we record here. Either skip any NULL pointers as I suggested
before, or return -1 to indicate the error (and update all callers).
> + dom = dom->next;
> }
> - return (n);
> + return n;
> }
>
> static virDomainPtr testDomainDefineXML(virConnectPtr conn,
...
> - if (handle < 0)
> - return (NULL);
> -
> - return virGetDomain(conn, privconn->domains[handle].name, privconn->domains[handle].uuid);
> + ret = virGetDomain(conn, def->name, def->uuid);
Possible NULL deref.
> + ret->id = -1;
> + return ret;
> }
...
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list