On 18-08-2010 12:45, soren(a)linux2go.dk wrote: Whoops, this bit wasn't meant to be included. /me reposts without it. -- Soren Hansen Ubuntu Developer http://www.ubuntu.com/

On Wed, Aug 18, 2010 at 01:35:29PM +0200, soren(a)linux2go.dk wrote: Yep, this looks fine. 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'. 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 :|

Like the 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. Signed-off-by: Soren Hansen <soren(a)linux2go.dk&gt; --- src/uml/uml_conf.c | 31 ++++++++++++++++++++++++++----- src/uml/uml_conf.h | 1 + src/uml/uml_driver.c | 12 +++++++++++- 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 42193e4..65b06c5 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -295,7 +295,8 @@ error: static char * umlBuildCommandLineChr(virDomainChrDefPtr def, - const char *dev) + const char *dev, + fd_set *keepfd) { char *ret = NULL; @@ -344,8 +345,27 @@ 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. Be wary of + * the effects of blocking I/O, though. */ case VIR_DOMAIN_CHR_TYPE_VC: case VIR_DOMAIN_CHR_TYPE_UDP: @@ -391,6 +411,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) { @@ -513,7 +534,7 @@ int umlBuildCommandLine(virConnectPtr conn, for (i = 0 ; i < UML_MAX_CHAR_DEVICE ; i++) { char *ret = NULL; if (i == 0 && vm->def->console) - ret = umlBuildCommandLineChr(vm->def->console, "con"); + ret = umlBuildCommandLineChr(vm->def->console, "con", keepfd); if (!ret) if (virAsprintf(&ret, "con%d=none", i) < 0) goto no_memory; @@ -527,7 +548,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); if (!ret) 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 9cad7f1..e926a9f 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -811,6 +811,7 @@ static int umlStartVMDaemon(virConnectPtr conn, char *logfile; int logfd = -1; struct stat sb; + int openmax; fd_set keepfd; char ebuf[1024]; umlDomainObjPrivatePtr priv = vm->privateData; @@ -869,7 +870,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); @@ -908,6 +909,15 @@ static int umlStartVMDaemon(virConnectPtr conn, NULL, NULL, NULL); close(logfd); + /* + * At the moment, the only thing that populates keepfd is + * umlBuildCommandLineChr. We want to close every fd it opens. + */ + openmax = sysconf (_SC_OPEN_MAX); + for (i = 0; i < openmax; i++) + if (FD_ISSET(i, &keepfd)) + close(i); + for (i = 0 ; argv[i] ; i++) VIR_FREE(argv[i]); VIR_FREE(argv); -- 1.7.0.4