[Libvir] PATCH 2/2 QEMU driver - backend daemon

[Daniel Veillard]

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 at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/