9:37 a.m.
On Tue, Jan 09, 2007 at 07:00:57AM -0500, Daniel Veillard wrote:
On Fri, Jan 05, 2007 at 09:16:54PM +0000, Daniel P. Berrange wrote:
> The attached patch provides the QEMU daemon for managing the QEMU instances
> and providing a network protocol for the libvirt driver to talk to over
> UNIX domain sockets or IPv4/6.
> +static int qemudParseUUID(const char *uuid,
> + unsigned char *rawuuid) {
need to go in lib/
Yes, +1
> +/* The list of possible machine types for various
architectures,
> + as supported by QEMU - taken from 'qemu -M ?' for each arch */
> +static const char *arch_info_x86_machines[] = {
> + "pc", "isapc"
> +};
> +static const char *arch_info_mips_machines[] = {
> + "mips"
> +};
> +static const char *arch_info_sparc_machines[] = {
> + "sun4m"
> +};
> +static const char *arch_info_ppc_machines[] = {
> + "g3bw", "mac99", "prep"
> +};
Hum, I wonder how we are gonna keep the sync :-)
I figure its not much hassle, since the list of supported machine
types hasn't significantly changed over time I've been monitoring
QEMU development. If neccessary we can switch to spawning 'qemu -M ?'
and parsing the list of machines.
> + /* XXX lame. should actually use $PATH ... */
> + path = malloc(strlen(name) + strlen("/usr/bin/") + 1);
> + if (!path) {
> + qemudReportError(server, VIR_ERR_NO_MEMORY, "path");
> + return NULL;
> + }
> + strcpy(path, "/usr/bin/");
> + strcat(path, name);
> + return path;
> +}
Shouldn't we walk $PATH to look up ?
Yes, for sure.
> + obj = xmlXPathEval(BAD_CAST
"string(/domain/os/type[1]/@machine)", ctxt);
> + if ((obj == NULL) || (obj->type != XPATH_STRING) ||
> + (obj->stringval == NULL) || (obj->stringval[0] == 0)) {
> + const char *defaultMachine =
qemudDefaultMachineForArch(vm->def.os.arch);
> + if (strlen(defaultMachine) >= (QEMUD_OS_MACHINE_MAX_LEN-1)) {
> + qemudReportError(server, VIR_ERR_INTERNAL_ERROR, "%s",
"machine type too long");
> + goto error;
> + }
> + strcpy(vm->def.os.machine, defaultMachine);
> + } else {
> + if (strlen((const char *)obj->stringval) >=
(QEMUD_OS_MACHINE_MAX_LEN-1)) {
> + qemudReportError(server, VIR_ERR_INTERNAL_ERROR, "%s",
"architecture type too long");
> + goto error;
> + }
> + strcpy(vm->def.os.machine, (const char *)obj->stringval);
> + }
> + if (obj)
> + xmlXPathFreeObject(obj);
Hum, there is a lot of duplication in the parsing, at least I should try to
provide libxml2 XPath lookup based functions instead of duplicating
xmlXPathEval all over the place with relatively complex cleanup etc...
Yeah, there's quite alot of this similar code across all the libvirt backends.
At the very least it probably makes sense to provide some convenience functions
in libvirt to simplify this repeated code. I think just 3-4 helper functions would
make a huge difference.
Can we try to keep the fuction comments in line
/**
* funcname:
* @arg1: ...
*
* ....
*
* Returns ....
*/
even for internal ones, don't block commiting for that but I will probably
make a pass over this once commited, that will force me to get deeper
understanding of the code
Yeah, i just added in the comments at the last minute before sending the patch
so didn't get around to formatting them nicely.
> + towrite = strlen(xml);
> + if (write(fd, xml, towrite) != towrite) {
> + qemudReportError(server, VIR_ERR_INTERNAL_ERROR, "cannot write config
file %s", vm->configFile);
> + goto cleanup;
> + }
> +
> + if (close(fd) < 0) {
> + qemudReportError(server, VIR_ERR_INTERNAL_ERROR, "cannot save config
file %s", vm->configFile);
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> + cleanup:
Shouldn't it close fd if write() fails ?
Yes, bugtastic.
> + uuid = vm->def.uuid;
> + if (qemudBufferPrintf(&buf, "
<uuid>%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x</uuid>\n",
> + uuid[0], uuid[1], uuid[2], uuid[3],
> + uuid[4], uuid[5], uuid[6], uuid[7],
> + uuid[8], uuid[9], uuid[10], uuid[11],
> + uuid[12], uuid[13], uuid[14], uuid[15]) < 0)
Hum, do we really want to use the format with '-' and not just the raw
hex dump by default, I'm still wondering what this brings...
I don't really mind. We should just provide a convenience function
'virFormatUUID'
so we can be consistent everywhere in libvirt. The dashes make it slightly easier
to read when comparing two UUIDs, but that's basically only difference.
> +static int qemudDispatchDomainCreate(struct qemud_server
*server, struct qemud_client *client,
> + struct qemud_packet *in, struct qemud_packet
*out) {
> + if (in->header.dataSize != sizeof(in->data.domainCreateRequest))
> + return -1;
> +
> + in->data.domainCreateRequest.xml[QEMUD_MAX_XML_LEN-1] ='\0';
> +
> + struct qemud_vm *vm = qemudDomainCreate(server,
in->data.domainCreateRequest.xml);
> + if (!vm) {
> + if (qemudDispatchFailure(server, client, out) < 0)
> + return -1;
> + } else {
> + out->header.type = QEMUD_PKT_DOMAIN_CREATE;
> + out->header.dataSize = sizeof(out->data.domainCreateReply);
> + out->data.domainCreateReply.id = vm->def.id;
> + memcpy(out->data.domainCreateReply.uuid, vm->def.uuid,
QEMUD_UUID_RAW_LEN);
> + strncpy(out->data.domainCreateReply.name, vm->def.name,
QEMUD_MAX_NAME_LEN-1);
> + out->data.domainCreateReply.name[QEMUD_MAX_NAME_LEN-1] = '\0';
> + }
> + return 0;
> +}
Hum, when qemudDomainCreate returns, we get a new vm, is that a new structure
which is gonna be ref-counted and cleaned up at some point ? I'm a bit lost
there, is that exactly the same scheme as for Xen domain instances ?
The qemudDomainCreate method is within the libvirt_qemud daemon, so we don't need
any reference counting in the server itself. The memory associated with the object
allocated to 'struct qemud_vm *' is free'd later in qemud.c when the guest
shuts
down. This function is only used for transient guests. Persistent guests (ie those
with an explicit saved config file) are started by qemudDomainStart.
> +/* List of different packet types which can be sent */
> +enum {
> + QEMUD_PKT_FAILURE,
> + QEMUD_PKT_GET_PROTOCOL_VERSION,
I always feel safer to initialize at least the first values in an enum.
for example QEMUD_PKT_FAILURE = 0, not strictly necessary but for client/server
stuff values I feel just better ... somehow I don't trust the way compilers
allocate enums .
Yeah, it also makes it explicit that it starts at 0 for those who aren't so
familiar with C rules for enums.
> + if (readonly)
> + oldmask = umask(~(S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH |
S_IWOTH));
> + else
> + oldmask = umask(~(S_IRUSR | S_IWUSR));
> + if (bind(sock->fd, (struct sockaddr *)&addr, sizeof(addr)) < 0)
sock is not freed here, it's still linked in the server sockets list,
rather bizarre, no ?
Yeha, its a little odd - complicated by the fact that there can be many sockets
open. The caller, qemudInitializer, iterates over all the addresses it needs to
listen on, and if any one fails, then it takes care of free'ing the qemud_socket
structure. This means we don't have to duplicate the funky cleanup code in both
qemudListenUNIX and qemudListenAddr.
> + if (listen(sock->fd, 30) < 0)
Same here, I don't understand the error handling :-)
Same rule as above - caller cleans up.
> + if (!strncmp(listenAddr, "ro:", 3)) {
> + readonly = 1;
> + listenAddr += 3;
> + }
wouldn't "ro-" (or ro+ or ro.) be cleaner, allowing to stick it in front
of any URI and still keeping it URI-valid ?
Yeah, I'm not entirely happy with current syntax here
> + goto cleanup;
> + }
> +
> + if ((ret = snprintf(server->configDir, PATH_MAX, "%s/.qemud.d",
pw->pw_dir)) > PATH_MAX) {
> + goto cleanup;
> + }
Hum, at some point the directory modes and ownership should be tested and
possibly created, and I guess the sooner the better, why not there ? But I
may have missed the check (or not gone there yet :-)
Yeah, actually I need to set umask to ~0600 here to ensure the config files are
writeable only by the user.
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 -=|