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

Re: [Libvir] PATCH: Support serial & parallel devices in QEMU driver



"Daniel P. Berrange" <berrange redhat com> wrote:
> A long time ago I proposed a syntax for serial / parallel port handling
> in libvirt XML.

Nice.
And it looks correct, too, but that's not too
surprising, especially with all of those tests.
I confirmed that it passes a valgrind-enabled "make check"
with no errors or leaks.

So ACK.

The only suggestions I can make are for maintainability/readability,
and one place where it'd be good to add an additional check.

> Index: src/qemu_conf.c
> ===================================================================
...
> +    case QEMUD_CHR_SRC_TYPE_DEV:
> +    case QEMUD_CHR_SRC_TYPE_FILE:
> +    case QEMUD_CHR_SRC_TYPE_PIPE:
> +        if (path == NULL) {
> +            qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                             "%s", _("Missing source path attribute for char device"));
> +            goto cleanup;
> +        }
> +
> +        strncpy(chr->srcData.file.path, (const char *)path,
> +                sizeof(chr->srcData.file.path));
> +        chr->srcData.file.path[sizeof(chr->srcData.file.path)-1] = '\0';

Since there are so many lines like the one above, how about using a
macro instead?  Otherwise, it's too hard/risky (as reviewer/maintainer)
to ensure that the long index expression always matches the array name.  E.g.,

  #define NUL_TERMINATE(buf) do { (buf)[sizeof(buf)-1] = '\0'; } while (0)

Then you can do this, here and in the 9 other cases below:
(this also shortens several >80 lines)

           NUL_TERMINATE(chr->srcData.file.path);

> +        break;
> +
> +    case QEMUD_CHR_SRC_TYPE_STDIO:
> +        /* Nada */
> +        break;
> +
> +    case QEMUD_CHR_SRC_TYPE_TCP:
> +        if (mode == NULL ||
> +            STREQ((const char *)mode, "connect")) {
> +            if (connectHost == NULL) {
> +                qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                                 "%s", _("Missing source host attribute for char device"));
> +                goto cleanup;
> +            }
> +            if (connectService == NULL) {
> +                qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                                 "%s", _("Missing source service attribute for char device"));
> +                goto cleanup;
> +            }
> +
> +            strncpy(chr->srcData.tcp.host, (const char *)connectHost,
> +                    sizeof(chr->srcData.tcp.host));
> +            chr->srcData.tcp.host[sizeof(chr->srcData.tcp.host)-1] = '\0';
> +            strncpy(chr->srcData.tcp.service, (const char *)connectService,
> +                    sizeof(chr->srcData.tcp.service));
> +            chr->srcData.tcp.service[sizeof(chr->srcData.tcp.service)-1] = '\0';
> +
> +            chr->srcData.tcp.listen = 0;
> +        } else {
> +            if (bindHost == NULL) {
> +                qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                                 "%s", _("Missing source host attribute for char device"));
> +                goto cleanup;
> +            }
> +            if (bindService == NULL) {
> +                qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                                 "%s", _("Missing source service attribute for char device"));
> +                goto cleanup;
> +            }
> +
> +            strncpy(chr->srcData.tcp.host, (const char *)bindHost,
> +                    sizeof(chr->srcData.tcp.host));
> +            chr->srcData.tcp.host[sizeof(chr->srcData.tcp.host)-1] = '\0';
> +            strncpy(chr->srcData.tcp.service, (const char *)bindService,
> +                    sizeof(chr->srcData.tcp.service));
> +            chr->srcData.tcp.service[sizeof(chr->srcData.tcp.service)-1] = '\0';

...
> +static int qemudParseCharXMLDevices(virConnectPtr conn,
> +				    xmlXPathContextPtr ctxt,
> +                                    const char *xpath,
> +                                    int *ndevs,

Maybe better to make this "unsigned int".  See below.

> +                                    struct qemud_vm_chr_def **devs)
> +{
> +    xmlXPathObjectPtr obj;
> +    int i;
> +
> +    obj = xmlXPathEval(BAD_CAST xpath, ctxt);
> +    if ((obj != NULL) && (obj->type == XPATH_NODESET) &&
> +        (obj->nodesetval != NULL) && (obj->nodesetval->nodeNr >= 0)) {
> +        struct qemud_vm_chr_def *prev = *devs;
> +        if (ndevs == NULL &&
> +            obj->nodesetval->nodeNr > 1) {
> +            qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                             "%s", _("too many character devices"));
> +            goto error;
> +        }
> +
> +        for (i = 0; i < obj->nodesetval->nodeNr; i++) {
> +            struct qemud_vm_chr_def *chr = calloc(1, sizeof(*chr));
> +            if (!chr) {
> +                qemudReportError(conn, NULL, NULL, VIR_ERR_NO_MEMORY,
> +                                 "%s",
> +                                 _("failed to allocate space for char device"));
> +                goto error;
> +            }
> +
> +            if (qemudParseCharXML(conn, chr, i, obj->nodesetval->nodeTab[i]) < 0) {
> +                free(chr);
> +                goto error;
> +            }
> +            if (ndevs)
> +                (*ndevs)++;
> +            chr->next = NULL;
> +            if (i == 0) {
> +                *devs = chr;
> +            } else {
> +                prev->next = chr;
> +            }
> +            prev = chr;
> +        }
> +    }
> +    xmlXPathFreeObject(obj);
> +
> +    return 0;
> +
> +error:
> +    xmlXPathFreeObject(obj);
> +    return -1;
> +}

Barely worth mentioning, but it's slightly better to avoid the
duplicate xmlXPathFreeObject above -- esp. since it's easy.
Having a single point of "return" is a good feature, too.

...
> +static int qemudGenerateXMLChar(virBufferPtr buf,
> +                                struct qemud_vm_chr_def *dev,

Looks like dev can be const, too.

> +                                const char *type)
> +{
> +    const char *types[] = {

You can make it so the array is const as well as each string:

    const char *const types[] = {

Actually, this list of strings should be tied to the declaration
of the corresponding QEMUD_* enum values, so if you ever add one
there, you are forced to add the new one here, too.

E.g., with this:
(assuming you add QEMUD_CHR_SRC_TYPE_LAST as an enum member):

  #include "verify.h" /* from gnulib */
  #define ARRAY_CARDINALITY(Array) (sizeof (Array) / sizeof *(Array))
  verify (ARRAY_CARDINALITY (types) == QEMUD_CHR_SRC_TYPE_LAST);

or this with existing:

  verify (ARRAY_CARDINALITY (types) == QEMUD_CHR_SRC_TYPE_UNIX + 1);

That gives you a *compile-time* check to ensure that
the set of enum members and size of the array match.

gnulib's verify.h is pretty cool, but it's LGPL, not v2+.
I'll see about changing that.  The part we need is so small
(but surprisingly dense) that maybe it doesn't matter...

  #  define verify_true(R) \
       (!!sizeof \
        (struct { unsigned int verify_error_if_negative_size__: (R) ? 1 : -1; }))
  #define verify(R) extern int (* verify_function__ (void)) [verify_true (R)]

> +        "null",
> +        "vc",
> +        "pty",
> +        "dev",
> +        "file",
> +        "pipe",
> +        "stdio",
> +        "udp",
> +        "tcp",
> +        "unix"
> +    };

It'd be nice to make sure dev->srcType is in range
before using it as an index... just in case.

> +    if (virBufferVSprintf(buf, "    <%s type='%s'>\n",
> +                          type, types[dev->srcType]) < 0)
> +        return -1;
> +

...
>  /* Generate an XML document describing the guest's configuration */
>  char *qemudGenerateXML(virConnectPtr conn,
>                         struct qemud_driver *driver ATTRIBUTE_UNUSED,
> @@ -2850,6 +3447,7 @@ char *qemudGenerateXML(virConnectPtr con
>      struct qemud_vm_disk_def *disk;
>      struct qemud_vm_net_def *net;
>      struct qemud_vm_input_def *input;
> +    struct qemud_vm_chr_def *chr;

The above can all be const.

>      const char *type = NULL;
>      int n;

...
> Index: src/qemu_conf.h
> ===================================================================

...
>  enum qemu_vm_input_type {
>      QEMU_INPUT_TYPE_MOUSE,
> @@ -223,6 +269,12 @@ struct qemud_vm_def {
>
>      int ninputs;
>      struct qemud_vm_input_def *inputs;
> +
> +    int nserials;

This never has a negative value.
(it's initialized to 0 by the calloc in qemudParseCharXMLDevices,
and every other use is a read or increment)
If you intend to keep it that way,
it's slightly better to declare it to be an unsigned type.
Same goes for qemudParseCharXMLDevices's matching "ndev" parameter.

> +    struct qemud_vm_chr_def *serials;
> +
> +    int nparallels;

Likewise.


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