[libvirt] [PATCH] Allow chardev of type 'file' for UML domains.

[Daniel P. Berrange]

On Wed, Aug 18, 2010 at 01:35:29PM +0200, soren at linux2go.dk wrote:
> From: Soren Hansen <soren at linux2go.dk>
> 
> Like that comment suggested, we just open the file and pass the file
> descriptor to uml. The input "stream" is set to "null", since I couldn't
> find any useful way to actually use a file for input for a chardev and
> this also mimics what e.g. QEmu does internally.

Yep, this looks fine.

> Signed-off-by: Soren Hansen <soren at linux2go.dk>
> ---
>  src/uml/uml_conf.c   |   30 +++++++++++++++++++++++++-----
>  src/uml/uml_conf.h   |    1 +
>  src/uml/uml_driver.c |    2 +-
>  3 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
> index bc8cbce..659f881 100644
> --- a/src/uml/uml_conf.c
> +++ b/src/uml/uml_conf.c
> @@ -297,7 +297,8 @@ error:
>  
>  static char *
>  umlBuildCommandLineChr(virDomainChrDefPtr def,
> -                       const char *dev)
> +                       const char *dev,
> +                       fd_set *keepfd)
>  {
>      char *ret = NULL;
>  
> @@ -346,8 +347,26 @@ umlBuildCommandLineChr(virDomainChrDefPtr def,
>          break;
>  
>      case VIR_DOMAIN_CHR_TYPE_FILE:
> -    case VIR_DOMAIN_CHR_TYPE_PIPE:
> -        /* XXX could open the file/pipe & just pass the FDs */
> +         {
> +            int fd_out;
> +
> +            if ((fd_out = open(def->data.file.path,
> +                               O_WRONLY | O_APPEND | O_CREAT, 0660)) < 0) {
> +                virReportSystemError(errno,
> +                                     _("failed to open chardev file: %s"),
> +                                     def->data.file.path);
> +                return NULL;
> +            }
> +            if (virAsprintf(&ret, "%s%d=null,fd:%d", dev, def->target.port, fd_out) < 0) {
> +                virReportOOMError();
> +                close(fd_out);
> +                return NULL;
> +            }
> +            FD_SET(fd_out, keepfd);
> +        }
> +        break;
> +   case VIR_DOMAIN_CHR_TYPE_PIPE:
> +        /* XXX could open the pipe & just pass the FDs */

Any reason not to let the code deal with PIPE too ? It seems
like the code should work equally well for both PIPE & FILE.
(well drop the O_CREATE|O_APPEND - assume the user has got
the pipe pre-created with 'mkfifo'.

>      case VIR_DOMAIN_CHR_TYPE_VC:
>      case VIR_DOMAIN_CHR_TYPE_UDP:
> @@ -393,6 +412,7 @@ static char *umlNextArg(char *args)
>  int umlBuildCommandLine(virConnectPtr conn,
>                          struct uml_driver *driver ATTRIBUTE_UNUSED,
>                          virDomainObjPtr vm,
> +                        fd_set *keepfd,
>                          const char ***retargv,
>                          const char ***retenv)
>  {
> @@ -515,7 +535,7 @@ int umlBuildCommandLine(virConnectPtr conn,
>      for (i = 0 ; i < UML_MAX_CHAR_DEVICE ; i++) {
>          char *ret;
>          if (i == 0 && vm->def->console)
> -            ret = umlBuildCommandLineChr(vm->def->console, "con");
> +            ret = umlBuildCommandLineChr(vm->def->console, "con", keepfd);
>          else
>              if (virAsprintf(&ret, "con%d=none", i) < 0)
>                  goto no_memory;
> @@ -529,7 +549,7 @@ int umlBuildCommandLine(virConnectPtr conn,
>              if (vm->def->serials[j]->target.port == i)
>                  chr = vm->def->serials[j];
>          if (chr)
> -            ret = umlBuildCommandLineChr(chr, "ssl");
> +            ret = umlBuildCommandLineChr(chr, "ssl", keepfd);
>          else
>              if (virAsprintf(&ret, "ssl%d=none", i) < 0)
>                  goto no_memory;
> diff --git a/src/uml/uml_conf.h b/src/uml/uml_conf.h
> index b33acc8..d8b2349 100644
> --- a/src/uml/uml_conf.h
> +++ b/src/uml/uml_conf.h
> @@ -71,6 +71,7 @@ virCapsPtr  umlCapsInit               (void);
>  int         umlBuildCommandLine       (virConnectPtr conn,
>                                         struct uml_driver *driver,
>                                         virDomainObjPtr dom,
> +                                       fd_set *keepfd,
>                                         const char ***retargv,
>                                         const char ***retenv);
>  
> diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
> index 04493ba..73f77f8 100644
> --- a/src/uml/uml_driver.c
> +++ b/src/uml/uml_driver.c
> @@ -871,7 +871,7 @@ static int umlStartVMDaemon(virConnectPtr conn,
>          return -1;
>      }
>  
> -    if (umlBuildCommandLine(conn, driver, vm,
> +    if (umlBuildCommandLine(conn, driver, vm, &keepfd,
>                              &argv, &progenv) < 0) {
>          close(logfd);
>          umlCleanupTapDevices(conn, vm);

I think this leaks file descriptors in libvirtd. There's no existing
code which ever closes the FDs in keepfd after spawning UML later
on in the function.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|