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