4:03 p.m.
"Daniel P. Berrange" <berrange(a)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;
}
...