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 -=|