On Tue, Feb 13, 2007 at 07:08:19PM +0000, Daniel P. Berrange wrote:
> The attached patch implements the daemon for managing QEMU instances. The
> main changes from previous versions are:
>
> - We can now talk to the QEMU monitor command line interface. This allows
> us to implement pause/resume/save/restore. Only the first two of those
> are done so far, and it needs more work to be truely robust.
>
> - We now grok /proc/meminfo, /proc/cpuinfo and /proc/{pid}/stat to pull
> out node information, and guest runtime state. This needs a little more
> work for correct info on NUMA boxes and hyperthreading. The basic
> operation is correct though.
>
> - All TCP / TLS stuff is removed, in favour of using the generic libvirtd
> for remote access.
just a few remarks to the code, feel free to commit to CVS,
thanks !
[...]
> +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 ;-)
> +static int qemudParseUUID(const char *uuid,
> + unsigned char *rawuuid) {
need to be factored out somewhere but I'm sure we already agreed on that
> + 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 ...
> + } else if (!S_ISDIR(sb.st_mode)) {
> + qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
> + "config directory %s is not a directory",
> + server->configDir);
> + return -1;
> + }
> +
> + if ((fd = open(vm->configFile,
> + O_WRONLY | O_CREAT | O_TRUNC,
> + S_IRUSR | S_IWUSR )) < 0) {
> + qemudReportError(server, VIR_ERR_INTERNAL_ERROR,
> + "cannot create config file %s",
> + vm->configFile);
> + goto cleanup;
[...]
> +/* Simple grow-on-demand string buffer */
> +/* XXX re-factor to shared library */
+1
[...]
> + 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 :-)
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/