[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] PATCH: 2/2: port Test driver to new domain APIs



"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;
>  }
...


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]