"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list