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

2010-08-24 Thread Matthias Bolte 
2010/8/23 Daniel P. Berrange berra...@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 so...@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2010-08-23 Thread Soren Hansen 
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 so...@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

--
libvir-list mailing list
libvir-list@redhat.com

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

2010-08-23 Thread Daniel P. Berrange 
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 so...@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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2010-08-23 Thread Soren Hansen 
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/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2010-08-23 Thread Daniel P. Berrange 
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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2010-08-23 Thread Soren Hansen 
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/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2010-08-23 Thread Soren Hansen 
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 so...@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2010-08-23 Thread Daniel P. Berrange 
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 so...@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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2010-08-19 Thread Daniel P. Berrange 
On Wed, Aug 18, 2010 at 01:35:29PM +0200, so...@linux2go.dk wrote:
 From: Soren Hansen so...@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 so...@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

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

2010-08-19 Thread Soren Hansen 
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/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2010-08-18 Thread soren 
From: Soren Hansen so...@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 so...@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2010-08-18 Thread Soren Hansen 
On 18-08-2010 12:45, so...@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/



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2010-08-18 Thread soren 
From: Soren Hansen so...@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 so...@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list