[libvirt] [PATCH] Allow chardev of type 'file' for UML domains.
From: Soren Hansen <soren@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. Signed-off-by: Soren Hansen <soren@linux2go.dk> --- src/uml/uml_conf.c | 30 +++++++++++++++++++++++++----- src/uml/uml_conf.h | 1 + src/uml/uml_driver.c | 6 +----- 3 files changed, 27 insertions(+), 10 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 */ 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..5f73ce2 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -737,10 +737,6 @@ static int umlMonitorCommand(const struct uml_driver *driver, virReportSystemError(errno, _("cannot read reply %s"), cmd); goto error; } - if (nbytes < sizeof res) { - virReportSystemError(0, _("incomplete reply %s"), cmd); - goto error; - } if (sizeof res.data < res.length) { virReportSystemError(0, _("invalid length in reply %s"), cmd); goto error; @@ -871,7 +867,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); -- 1.7.0.4
On 18-08-2010 12:45, soren@linux2go.dk wrote:
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 04493ba..5f73ce2 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -737,10 +737,6 @@ static int umlMonitorCommand(const struct uml_driver *driver, virReportSystemError(errno, _("cannot read reply %s"), cmd); goto error; } - if (nbytes < sizeof res) { - virReportSystemError(0, _("incomplete reply %s"), cmd); - goto error; - } if (sizeof res.data < res.length) { virReportSystemError(0, _("invalid length in reply %s"), cmd); goto error;
Whoops, this bit wasn't meant to be included. /me reposts without it. -- Soren Hansen Ubuntu Developer http://www.ubuntu.com/
From: Soren Hansen <soren@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. Signed-off-by: Soren Hansen <soren@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 */ 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); -- 1.7.0.4
On Wed, Aug 18, 2010 at 01:35:29PM +0200, soren@linux2go.dk wrote:
From: Soren Hansen <soren@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@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 :|
On 19-08-2010 15:41, Daniel P. Berrange wrote:
+ 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'.
Not in particular. I was just in a hurry, wanted to share the patch sooner rather than later and then attack the pipes later on. I'll fix it up, but perhaps I won't get to it today.
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.
True. I somehow thought virExecWhatever took care of that, but I see now that it doesn't, and I guess that would be troublesome. I'll handle this in the uml driver. -- Soren Hansen Ubuntu Developer http://www.ubuntu.com/
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@linux2go.dk> --- 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
On Mon, Aug 23, 2010 at 12:19:51PM +0200, Soren Hansen wrote:
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@linux2go.dk> --- 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_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); +
Unfortunately fdset is one of those limited types that can't represent all possible values. So you need to use FD_SETSIZE instead of _SC_OPEN_MAX here 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 :|
On 23-08-2010 12:42, Daniel P. Berrange wrote:
+ /* + * 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); + Unfortunately fdset is one of those limited types that can't represent all possible values. So you need to use FD_SETSIZE instead of _SC_OPEN_MAX here
Ok, I'll fix that up, but just so that I understand: Your concern is that there might be an open file descriptor between FD_SETSIZE and _SC_OPEN_MAX that we don't want to close? -- Soren Hansen Ubuntu Developer http://www.ubuntu.com/
On Mon, Aug 23, 2010 at 12:59:16PM +0200, Soren Hansen wrote:
On 23-08-2010 12:42, Daniel P. Berrange wrote:
+ /* + * 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); + Unfortunately fdset is one of those limited types that can't represent all possible values. So you need to use FD_SETSIZE instead of _SC_OPEN_MAX here
Ok, I'll fix that up, but just so that I understand: Your concern is that there might be an open file descriptor between FD_SETSIZE and _SC_OPEN_MAX that we don't want to close?
No, its that if you try to run FD_ISSET for i > FD_SETSIZE, you'll likely have an array overflow / out of bounds, so just stop at FD_SETSIZE. When we switch to the new virCommandPtr apis we'll remove this limitation. 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 :|
On 23-08-2010 13:04, Daniel P. Berrange wrote:
Ok, I'll fix that up, but just so that I understand: Your concern is that there might be an open file descriptor between FD_SETSIZE and _SC_OPEN_MAX that we don't want to close? No, its that if you try to run FD_ISSET for i > FD_SETSIZE, you'll likely have an array overflow / out of bounds, so just stop at FD_SETSIZE.
Oh, of course. How silly of me. Thanks :) -- Soren Hansen Ubuntu Developer http://www.ubuntu.com/
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@linux2go.dk> --- src/uml/uml_conf.c | 31 ++++++++++++++++++++++++++----- src/uml/uml_conf.h | 1 + src/uml/uml_driver.c | 10 +++++++++- 3 files changed, 36 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..6582d95 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -869,7 +869,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 +908,14 @@ 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. + */ + for (i = 0; i < FD_SETSIZE; 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
On Mon, Aug 23, 2010 at 01:25:50PM +0200, Soren Hansen wrote:
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@linux2go.dk> --- src/uml/uml_conf.c | 31 ++++++++++++++++++++++++++----- src/uml/uml_conf.h | 1 + src/uml/uml_driver.c | 10 +++++++++- 3 files changed, 36 insertions(+), 6 deletions(-)
ACK, looks good now. 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 :|
2010/8/23 Daniel P. Berrange <berrange@redhat.com>:
On Mon, Aug 23, 2010 at 01:25:50PM +0200, Soren Hansen wrote:
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@linux2go.dk> --- src/uml/uml_conf.c | 31 ++++++++++++++++++++++++++----- src/uml/uml_conf.h | 1 + src/uml/uml_driver.c | 10 +++++++++- 3 files changed, 36 insertions(+), 6 deletions(-)
ACK, looks good now.
I applied and pushed this patch. Matthias
participants (5)
-
Daniel P. Berrange -
Matthias Bolte -
Soren Hansen -
Soren Hansen -
soren＠linux2go.dk