[libvirt] PATCH: User mode linux driver

[Daniel Veillard]

On Wed, Nov 19, 2008 at 01:12:55PM +0000, Daniel P. Berrange wrote:
> This patch is an update of
> 
> http://www.redhat.com/archives/libvir-list/2008-October/msg00355.html
> 
> providing a user mode linux driver. I've fixed a number of stupid bugs
> since the first posting, so its more reliable & less crash prone on
> startup. I also added a short driver doc page for the website. Even
> though this driver isn't finished (it still lacks networking), I'd
> like to get this info 0.5.0 release so it can at least be tried out by
> interested people. It shouldn't impact any users of existing drivers.

  Okay, principle sounds right, it's not bad to push this now even if
there is a few sharp edges :-)

[...]
> @@ -2016,32 +2023,30 @@ static virDomainDefPtr virDomainDefParse
>      }
>      VIR_FREE(nodes);
>  
> -    /*
> -     * If no serial devices were listed, then look for console
> -     * devices which is the legacy syntax for the same thing
> -     */
> -    if (def->nserials == 0) {
> -        if ((node = virXPathNode(conn, "./devices/console[1]", ctxt)) != NULL) {
> -            virDomainChrDefPtr chr = virDomainChrDefParseXML(conn,
> -                                                             node);
> -            if (!chr)
> -                goto error;
> +    if ((node = virXPathNode(conn, "./devices/console[1]", ctxt)) != NULL) {
> +        virDomainChrDefPtr chr = virDomainChrDefParseXML(conn,
> +                                                         node);
> +        if (!chr)
> +            goto error;
>  
> -            chr->dstPort = 0;
> -            /*
> -             * For HVM console actually created a serial device
> -             * while for non-HVM it was a parvirt console
> -             */
> -            if (STREQ(def->os.type, "hvm")) {
> +        chr->dstPort = 0;
> +        /*
> +         * For HVM console actually created a serial device
> +         * while for non-HVM it was a parvirt console
> +         */
> +        if (STREQ(def->os.type, "hvm")) {
> +            if (def->nserials != 0) {
> +                virDomainChrDefFree(chr);
> +            } else {
>                  if (VIR_ALLOC_N(def->serials, 1) < 0) {
>                      virDomainChrDefFree(chr);
>                      goto no_memory;
>                  }
>                  def->nserials = 1;
>                  def->serials[0] = chr;
> -            } else {
> -                def->console = chr;
>              }
> +        } else {
> +            def->console = chr;
>          }
>      }

  Hummm, that change to console code parsing seems rather unrelated,
isn't it ?

[...]
> +int umlBuildCommandLine(virConnectPtr conn,
> +                        struct uml_driver *driver ATTRIBUTE_UNUSED,
> +                        virDomainObjPtr vm,
> +                        const char ***retargv,
> +                        const char ***retenv,
> +                        int **tapfds,
> +                        int *ntapfds) {
[...]
> +#define ADD_ARG_SPACE                                                   \
> +    do { \

  Align \\ with others
In general I prefer to see macros definitions outside of C code blocks,
I don't know why that makes me a bit nervous.


> +    for (i = 0 ; i < 16 ; i++) {
> +        char *ret;
> +        if (i == 0 && vm->def->console)
> +            ret = umlBuildCommandLineChr(conn, vm->def->console, "con");
> +        else
> +            if (asprintf(&ret, "con%d=none", i) < 0)
> +                goto no_memory;
> +        ADD_ARG(ret);
> +    }
> +
> +    for (i = 0 ; i < 16 ; i++) {
> +        virDomainChrDefPtr chr = NULL;
> +        char *ret;
> +        for (j = 0 ; j < vm->def->nserials ; j++)
> +            if (vm->def->serials[j]->dstPort == i)
> +                chr = vm->def->serials[j];
> +        if (chr)
> +            ret = umlBuildCommandLineChr(conn, chr, "ssl");
> +        else
> +            if (asprintf(&ret, "ssl%d=none", i) < 0)
> +                goto no_memory;
> +        ADD_ARG(ret);
> +    }

  I'm a bit puzzled by 16 is that an internal UML limit ?

[...]
> --- /dev/null
> +++ b/src/uml_conf.h
[...]
> +#define umlDebug(fmt, ...) do {} while(0)

  humpf we really need to get this debug internal API set up, my fault
I'm too slow !

[...]
> +++ b/src/uml_driver.c
> @@ -0,0 +1,1671 @@
> +/*
> + * driver.c: core driver methods for managing qemu guests

  s/qemu/uml/

  maybe we could abstract even more internal APIs ;-)

[...]
> +/* umlDebug statements should be changed to use this macro instead. */

  we should have fixed the qemu driver before doing the copy it seems

> +#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
> +#define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg)
> +
> +#define umlLog(level, msg...) fprintf(stderr, msg)
[...]
> +    if (fscanf(pidinfo, "%*d %*s %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu %llu", &usertime, &systime) != 2) {

  heh, I didn't knew the * assignment-suppression character, I learned
  something !


  okay, no surprises,

   fine by me, +1

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/