What follows is yet another iteration of the QEMU patches. Based on recent discussions about a generic remote management daemon, this patch set has been stripped to remove all TCP functionality, and TLS encryption. It is now local only unix domain sockets, with no encyption. THe idea being that the generic libvirtd will be able to expose this over the network securely. The patchset is split into two halves - the driver and the daemon. There will be further explanation alongside each patch. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
The attached patch implements the library driver for QEMU. The driver is pretty much identical in style to the xen proxy driver. There are two supported URLs: qemu:///session - a per-user (private) daemon.Can be run by unprivileged users. Config files kept in $HOME/.qemu and the socket is in the abstract namespace at $HOME/.qemu/sock The daemon for session instance is spawned on demand qemu:///system - a per-machine (public) daemon. Must be launched ahead of time by root. Config files kept in /etc/qemu and socket on the FS at /var/run/qemu/sock & sock-ro The read-write socket is restricted to root only, while the read-only socket is public. This patch also: - makes virsh use a read-write connection by default - adds extra error info to virterror & friends Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Tue, Feb 13, 2007 at 07:03:42PM +0000, Daniel P. Berrange wrote:
The attached patch implements the library driver for QEMU.
The driver is pretty much identical in style to the xen proxy driver. There are two supported URLs:
qemu:///session - a per-user (private) daemon.Can be run by unprivileged users. Config files kept in $HOME/.qemu and the socket is in the abstract namespace at $HOME/.qemu/sock The daemon for session instance is spawned on demand
qemu:///system - a per-machine (public) daemon. Must be launched ahead of time by root. Config files kept in /etc/qemu and socket on the FS at /var/run/qemu/sock & sock-ro The read-write socket is restricted to root only, while the read-only socket is public.
This patch also:
- makes virsh use a read-write connection by default - adds extra error info to virterror & friends
Looks just fine, but I still have a few comments see below :-) [...]
+ /* 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 !
+ +int qemuListDomains(virConnectPtr conn, + int *ids, + int maxids){ + struct qemud_packet req, reply; + int i, nDomains; + + req.header.type = QEMUD_PKT_LIST_DOMAINS; + req.header.dataSize = 0; + + if (qemuProcessRequest(conn, NULL, &req, &reply) < 0) { + return -1; + } + + 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.
+ for (i = 0 ; i < nDomains ; i++) { + ids[i] = reply.data.listDomainsReply.domains[i]; + } + + return nDomains; +} + + +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
+ return NULL; + } +
+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 :-).
+#ifndef __VIR_QEMU_INTERNAL_H__ +#define __VIR_QEMU_INTERNAL_H__ + +#include <libvirt/virterror.h> + +#ifdef __cplusplus +extern "C" { +#endif + + void qemuRegister(void); + +#ifdef __cplusplus +} +#endif +#endif /* __VIR_QEMU_INTERNAL_H__ */
Feel free to push to CVS, thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Tue, Feb 13, 2007 at 04:06:17PM -0500, Daniel Veillard wrote:
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 !
Personally I'd define them at point of first use, but we're not using C99 in libvirt, so I'll settle for start of nearest nested code block
+ 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.
Yeah, I'll just clamp it to maxids.
+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
Definitely. There's a bunch of other places which need more error reporting in both driver & daemon. I've fixed up all the critical path ones which you will encounter commonly, but still more low priority ones to deal with.
+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 :-).
Sounds reasonable. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
The attached patch implements the library driver for QEMU. ... +int qemuClose (virConnectPtr conn) { + if (conn->handle != -1) { + close(conn->handle); + conn->handle = -1; + } + return 0; +}
Hi Dan, In follow-up discussion you said you knew of other places where it'd be good to check for less-common failures. This "close" is probably one of them.
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. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Tue, Feb 13, 2007 at 05:04:24PM -0500, Daniel Veillard wrote:
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 ;-)
I can't remember why I needed those originally - it was probably for the TLS / TCP stuff I ripped out. Anyway I removed these all and it still compiled just fine.
+ 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
Done.
+ return net; + + /* + error: + if (macaddr) + xmlFree(macaddr); + free(net); + return NULL; + */
if not needed anymore remove, unless it was needed for something more advanced
Killed. Its obsolete and easily re-added if ever needed again/
+/* + * 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.
Yeah, we can definitely do with a couple of convenince functions for extracting data from the XML in libvirt - we basically only ever look for an int or char * in libvirt.
+/* + * 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 :-) ?
A hell of a lot :-)
+ 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 we're going to do that, we should also check ownership when initially loading the domains at startup. I'll add that to the future list of things todo.
+ 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.
Yes, the 'type' can be one of 'qemu' or 'kqemu' or 'kvm'.
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 ...
It certainly lends itself to doing that. All these functions just de-marshall the data from the wire & then call the real functions to actual work. This stuff may well need to change after Rich Jones' libvirtd stuff is ready, so I think we should wait before thinking about generalizing this bit.
[...]
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 :-)
Its very nice - KVM support basically comes down to spawing /usr/bin/qemu-kvm Instead of /usr/bin/qemu Based on the top level 'type' attribute. When KVM bits merge in mainline QEMU it'll be even simpler :-) The benefit of working with/close to upstream instead of forking it Xen style... Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering