"Daniel P. Berrange" <berrange@redhat.com> 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; } ...