On Tue, Feb 13, 2007 at 07:03:42PM +0000, Daniel P. Berrange wrote: > + /* Block sending entire outgoing packet */ > + while (outLeft) { > + int got = write(conn->handle, out+outDone, outLeft); stylistic, I prefer to have the variables defined at the function level, but I could understand why one would argue otherwise :-) Appears in many other places, definitely not a blocker though !

> + nDomains = reply.data.listDomainsReply.numDomains; > + if (nDomains > maxids) > + return -1; Is the semantic really to error instead of truncating in that case ? it seems to me the code in other drivers just pass the first maxids ones.

> +virDomainPtr > +qemuDomainCreateLinux(virConnectPtr conn, const char *xmlDesc, > + unsigned int flags ATTRIBUTE_UNUSED){ > + struct qemud_packet req, reply; > + virDomainPtr dom; > + int len = strlen(xmlDesc); > + > + if (len > (QEMUD_MAX_XML_LEN-1)) { maybe we need to provide a clear error there

> +int qemuListDefinedDomains(virConnectPtr conn, > +int qemuDomainCreate(virDomainPtr dom) { > +virDomainPtr qemuDomainDefineXML(virConnectPtr conn, const char *xml) { > +int qemuUndefine(virDomainPtr dom) { Seems to me all of these drivers entry point should be made static since they are not exported from the .h, right ? Only the registration routine ought to be exported (very clean :-).

On Tue, Feb 13, 2007 at 07:08:19PM +0000, Daniel P. Berrange wrote: > +libvirt_qemud_CFLAGS = -D_XOPEN_SOURCE=600 -D_XOPEN_SOURCE_EXTENDED=1 -D_POSIX_C_SOURCE=199506L \ Hum, we really need all those flags ? Maybe one day libvirt should compile on some weird platform ;-)

> + if (macaddr) { > + sscanf((const char *)macaddr, "%02x:%02x:%02x:%02x:%02x:%02x", > + (unsigned int*)&net->mac[0], > + (unsigned int*)&net->mac[1], > + (unsigned int*)&net->mac[2], > + (unsigned int*)&net->mac[3], > + (unsigned int*)&net->mac[4], > + (unsigned int*)&net->mac[5]); > + } > + > + xmlFree(macaddr); let's move it into if (macaddr) { block

> + return net; > + > + /* > + error: > + if (macaddr) > + xmlFree(macaddr); > + free(net); > + return NULL; > + */ if not needed anymore remove, unless it was needed for something more advanced

> +/* > + * Parses a libvirt XML definition of a guest, and populates the > + * the qemud_vm struct with matching data about the guests config > + */ > +static int qemudParseXML(struct qemud_server *server, > + xmlDocPtr xml, > + struct qemud_vm *vm) { lot of that code could be cleaned up if I provided nicer high level function to do XPath extraction of values.

> +/* > + * Constructs a argv suitable for launching qemu with config defined > + * for a given virtual machine. > + */ > +int qemudBuildCommandLine(struct qemud_server *server, > + struct qemud_vm *vm, > + char ***argv, > + int *argc) { [...] > + (*argv)[++n] = NULL; how many args do we usually end up with :-) ?

> + return 0; > + > +/* Save a guest's config data into a persistent file */ > +static int qemudSaveConfig(struct qemud_server *server, > + struct qemud_vm *vm) { > + char *xml; > + int fd = -1, ret = -1; > + int towrite; > + struct stat sb; > + > + if (!(xml = qemudGenerateXML(server, vm))) { > + return -1; > + } > + > + if (stat(server->configDir, &sb) < 0) { > + if (errno == ENOENT) { > + if (mkdir(server->configDir, 0700) < 0) { > + qemudReportError(server, VIR_ERR_INTERNAL_ERROR, > + "cannot create config directory %s", > + server->configDir); > + return -1; > + } > + } else { > + qemudReportError(server, VIR_ERR_INTERNAL_ERROR, > + "cannot stat config directory %s", > + server->configDir); > + return -1; > + } Hum, should we check for ownership of that directory before writing in it, I'm not sure ...

> + if (vm->def.id >= 0) { > + if (qemudBufferPrintf(&buf, "<domain type='%s' id='%d'>\n", type, vm->def.id) < 0) > + goto no_memory; > + } else { > + if (qemudBufferPrintf(&buf, "<domain type='%s'>\n", type) < 0) > + goto no_memory; > + } Hum, did you change the format ? I remember you sent a mail about it and I'm afraid I forgot to answer at the time.

> Index: qemud/config.h [...] > +clientFunc funcsTransmitRW[QEMUD_PKT_MAX] = { > + NULL, /* FAILURE code */ > + qemudDispatchGetVersion, > + qemudDispatchGetNodeInfo, > + qemudDispatchListDomains, > + qemudDispatchNumDomains, > + qemudDispatchDomainCreate, > + qemudDispatchDomainLookupByID, > + qemudDispatchDomainLookupByUUID, > + qemudDispatchDomainLookupByName, > + qemudDispatchDomainSuspend, > + qemudDispatchDomainResume, > + qemudDispatchDomainDestroy, > + qemudDispatchDomainGetInfo, > + qemudDispatchDomainSave, > + qemudDispatchDomainRestore, > + qemudDispatchDumpXML, > + qemudDispatchListDefinedDomains, > + qemudDispatchNumDefinedDomains, > + qemudDispatchDomainStart, > + qemudDispatchDomainDefine, > + qemudDispatchDomainUndefine > +}; > + > +clientFunc funcsTransmitRO[QEMUD_PKT_MAX] = { > + NULL, /* FAILURE code */ > + qemudDispatchGetVersion, > + qemudDispatchGetNodeInfo, > + qemudDispatchListDomains, > + qemudDispatchNumDomains, > + NULL, > + qemudDispatchDomainLookupByID, > + qemudDispatchDomainLookupByUUID, > + qemudDispatchDomainLookupByName, > + NULL, > + NULL, > + NULL, > + qemudDispatchDomainGetInfo, > + NULL, > + NULL, > + qemudDispatchDumpXML, > + qemudDispatchListDefinedDomains, > + qemudDispatchNumDefinedDomains, > + NULL, > + NULL, > + NULL, > +}; I wonder if there would be any way to automatically exercise all that network glue code. Somthing like plugging the test driver on the server side for make check ...

[...] > Index: qemud/qemud.c > + for (i = 0; i < open_max; i++) > + if (i != STDIN_FILENO && > + i != STDOUT_FILENO && > + i != STDERR_FILENO) > + close(i); I usually try to keep expressions fully parenthetized to avoid mistake Overall I'm surpized by how minimalist the changes are from normal QEmu to KVM, code reuse is nice :-)