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

17 Apr 2008


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

    

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

Jim Meyering