[Qemu-devel] [FOR 0.12][PATCH] monitor: Accept input only byte-wise

[Qemu-devel] [FOR 0.12][PATCH] monitor: Accept input only byte-wise

[Jan Kiszka]

This allows to suspend command interpretation and execution
synchronously, e.g. during migration.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 monitor.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 3286ba2..a3be1c8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3418,7 +3418,7 @@ static int monitor_can_read(void *opaque)
 {
     Monitor *mon = opaque;
 
-    return (mon->suspend_cnt == 0) ? 128 : 0;
+    return (mon->suspend_cnt == 0) ? 1 : 0;
 }
 
 static void monitor_read(void *opaque, const uint8_t *buf, int size)

Re: [Qemu-devel] [FOR 0.12][PATCH] monitor: Accept input only byte-wise

[Daniel P. Berrange]

On Fri, Dec 04, 2009 at 02:05:29PM +0100, Jan Kiszka wrote:
> This allows to suspend command interpretation and execution
> synchronously, e.g. during migration.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  monitor.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 3286ba2..a3be1c8 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3418,7 +3418,7 @@ static int monitor_can_read(void *opaque)
>  {
>      Monitor *mon = opaque;
>  
> -    return (mon->suspend_cnt == 0) ? 128 : 0;
> +    return (mon->suspend_cnt == 0) ? 1 : 0;
>  }
>  
>  static void monitor_read(void *opaque, const uint8_t *buf, int size)

FYI, this change seems to have broken the 'getfd' command in the monitor. 
Any attempt to use this command (in both text + json modes) is currently 
returning

  "No file descriptor supplied via SCM_RIGHTS"

Reverting this change makes getfd work again for me.

Strace of libvirtd shows it sending the command + FD in one message:

 sendmsg(18, {msg_name(0)=NULL, msg_iov(1)=[{"getfd fd-net2\r", 14}],
  msg_controllen=24, {cmsg_len=20, cmsg_level=SOL_SOCKET, cmsg_type=SCM_RIGHTS,
 {16}}, msg_flags=0}, 0) = 14

Strace of qemu shows that it receives the FD ok, but immediately closes it
before finishing reading the actual monitor command with which it is associated:

[pid 31941] recvmsg(16, {msg_name(0)=NULL, msg_iov(1)=[{"g", 1}], msg_controllen=24, {cmsg_len=20, cmsg_level=SOL_SOCKET, cmsg_type=SCM_RIGHTS, {17}}, msg_flags=0}, 0) = 1
[pid 31941] close(17)                   = 0
[pid 31941] recvmsg(16, {msg_name(0)=NULL, msg_iov(1)=[{"e", 1}], msg_controllen=0, msg_flags=0}, 0) = 1
[pid 31941] recvmsg(16, {msg_name(0)=NULL, msg_iov(1)=[{"t", 1}], msg_controllen=0, msg_flags=0}, 0) = 1
[pid 31941] recvmsg(16, {msg_name(0)=NULL, msg_iov(1)=[{"f", 1}], msg_controllen=0, msg_flags=0}, 0) = 1
[pid 31941] recvmsg(16, {msg_name(0)=NULL, msg_iov(1)=[{"d", 1}], msg_controllen=0, msg_flags=0}, 0) = 1
[pid 31941] recvmsg(16, {msg_name(0)=NULL, msg_iov(1)=[{" ", 1}], msg_controllen=0, msg_flags=0}, 0) = 1
[pid 31941] recvmsg(16, {msg_name(0)=NULL, msg_iov(1)=[{"f", 1}], msg_controllen=0, msg_flags=0}, 0) = 1
[pid 31941] recvmsg(16, {msg_name(0)=NULL, msg_iov(1)=[{"d", 1}], msg_controllen=0, msg_flags=0}, 0) = 1
[pid 31941] recvmsg(16, {msg_name(0)=NULL, msg_iov(1)=[{"-", 1}], msg_controllen=0, msg_flags=0}, 0) = 1
[pid 31941] recvmsg(16, {msg_name(0)=NULL, msg_iov(1)=[{"n", 1}], msg_controllen=0, msg_flags=0}, 0) = 1
[pid 31941] recvmsg(16, {msg_name(0)=NULL, msg_iov(1)=[{"e", 1}], msg_controllen=0, msg_flags=0}, 0) = 1
[pid 31941] recvmsg(16, {msg_name(0)=NULL, msg_iov(1)=[{"t", 1}], msg_controllen=0, msg_flags=0}, 0) = 1
[pid 31941] recvmsg(16, {msg_name(0)=NULL, msg_iov(1)=[{"2", 1}], msg_controllen=0, msg_flags=0}, 0) = 1
[pid 31941] recvmsg(16, {msg_name(0)=NULL, msg_iov(1)=[{"\r", 1}], msg_controllen=0, msg_flags=0}, 0) = 1

The QEMU code appears to be written to assume that it will recvmsg() a
complete monitor command in one go + process that, because it closes the
FD the moment the data from any recvmsg() is dealt with.

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 :|

[Qemu-devel] Re: [FOR 0.12][PATCH] monitor: Accept input only byte-wise

[Paolo Bonzini]

> The QEMU code appears to be written to assume that it will recvmsg() a
> complete monitor command in one go + process that, because it closes the
> FD the moment the data from any recvmsg() is dealt with.

This is buggy anyway.  This should fix it too:

diff --git a/monitor.c b/monitor.c
index 5659991..225a922 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2408,15 +2408,6 @@
          return -1;
      }

-    fd = dup(fd);
-    if (fd == -1) {
-        if (errno == EMFILE)
-            qerror_report(QERR_TOO_MANY_FILES);
-        else
-            qerror_report(QERR_UNDEFINED_ERROR);
-        return -1;
-    }
-
      QLIST_FOREACH(monfd, &mon->fds, next) {
          if (strcmp(monfd->name, fdname) != 0) {
              continue;
diff --git a/qemu-char.c b/qemu-char.c
index 05df971..ac65a1c 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2000,8 +2000,9 @@
  static int tcp_get_msgfd(CharDriverState *chr)
  {
      TCPCharDriver *s = chr->opaque;
-
-    return s->msgfd;
+    int fd = s->msgfd;
+    s->msgfd = -1;
+    return fd;
  }

  #ifndef _WIN32
@@ -2089,10 +2090,6 @@ static void tcp_chr_read(void *opaque)
              tcp_chr_process_IAC_bytes(chr, s, buf, &size);
          if (size > 0)
              qemu_chr_read(chr, buf, size);
-        if (s->msgfd != -1) {
-            close(s->msgfd);
-            s->msgfd = -1;
-        }
      }
  }


Paolo

[Qemu-devel] Re: [FOR 0.12][PATCH] monitor: Accept input only byte-wise

[Daniel P. Berrange]

On Fri, Apr 16, 2010 at 01:14:11PM +0200, Paolo Bonzini wrote:
> 
> >The QEMU code appears to be written to assume that it will recvmsg() a
> >complete monitor command in one go + process that, because it closes the
> >FD the moment the data from any recvmsg() is dealt with.
> 
> This is buggy anyway.  This should fix it too:

Yep, this makes it work too, but if a client is evil they could
pass a FD to qemu with any other non-getfd command & it'd remain 
open for ever. Probably not important though.

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 :|

[Qemu-devel] Re: [FOR 0.12][PATCH] monitor: Accept input only byte-wise

[Paolo Bonzini]

On 04/16/2010 03:17 PM, Daniel P. Berrange wrote:
> On Fri, Apr 16, 2010 at 01:14:11PM +0200, Paolo Bonzini wrote:
>>
>>> The QEMU code appears to be written to assume that it will recvmsg() a
>>> complete monitor command in one go + process that, because it closes the
>>> FD the moment the data from any recvmsg() is dealt with.
>>
>> This is buggy anyway.  This should fix it too:
>
> Yep, this makes it work too, but if a client is evil they could
> pass a FD to qemu with any other non-getfd command&  it'd remain
> open for ever. Probably not important though.

No, it wouldn't: outside the part that I patched there is this:

         if (s->msgfd != -1)
             close(s->msgfd);
         s->msgfd = fd;

Only one file descriptor could "leak".

Paolo

[Qemu-devel] [FOR 0.12] [PATCH] stash away SCM_RIGHTS fd until a getfd command arrives

[Paolo Bonzini]

If there is already a fd in s->msgfd before recvmsg it is
closed by parts that this patch does not touch.  So, only
one descriptor can be "leaked" by attaching it to a command
other than getfd.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor.c   |    9 ---------
 qemu-char.c |    9 +++------
 2 files changed, 3 insertions(+), 15 deletions(-)

        Tested by Daniel, identical to the previous one except for
        the Signed-off-by line.

diff --git a/monitor.c b/monitor.c
index 5659991..225a922 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2408,15 +2408,6 @@ static int do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
         return -1;
     }
 
-    fd = dup(fd);
-    if (fd == -1) {
-        if (errno == EMFILE)
-            qerror_report(QERR_TOO_MANY_FILES);
-        else
-            qerror_report(QERR_UNDEFINED_ERROR);
-        return -1;
-    }
-
     QLIST_FOREACH(monfd, &mon->fds, next) {
         if (strcmp(monfd->name, fdname) != 0) {
             continue;
diff --git a/qemu-char.c b/qemu-char.c
index 05df971..ac65a1c 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2000,8 +2000,9 @@ static void tcp_chr_process_IAC_bytes(CharDriverState *chr,
 static int tcp_get_msgfd(CharDriverState *chr)
 {
     TCPCharDriver *s = chr->opaque;
-
-    return s->msgfd;
+    int fd = s->msgfd;
+    s->msgfd = -1;
+    return fd;
 }
 
 #ifndef _WIN32
@@ -2089,10 +2090,6 @@ static void tcp_chr_read(void *opaque)
             tcp_chr_process_IAC_bytes(chr, s, buf, &size);
         if (size > 0)
             qemu_chr_read(chr, buf, size);
-        if (s->msgfd != -1) {
-            close(s->msgfd);
-            s->msgfd = -1;
-        }
     }
 }
 
-- 
1.6.6.1