[Qemu-devel] [PATCH 0/3] Allow host_net_add monitor command accept file descriptors

[Qemu-devel] [PATCH 0/3] Allow host_net_add monitor command accept file descriptors

[Mark McLoughlin]

Hi,
	You can pass file descriptors to qemu via the command line using '-net
tap,fd=' or '-net socket,tap='. However, you cannot currently do this
via the monitor.

	libvirt always configures tap interfaces before passing them to qemu.
One reason for this is to allow libvirtd to have privileges
to /dev/net/tun without allowing qemu those privileges. Because of this,
libvirt currently does not support NIC hotplug for qemu.

	The following three patches add support for passing a file descriptor
to the monitor command by allowing file descriptors to be received over
monitor commands on a unix socket.

	For reference:

  https://fedoraproject.org/wiki/Features/KVM_NIC_Hotplug
  https://fedoraproject.org/wiki/Features/VirtPrivileges

Cheers,
Mark.

[Qemu-devel] [PATCH 1/3] Make tcp_chr_read() use recvmsg()

[Mark McLoughlin]

This makes no functional changes, just paves the way for the next
patch.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 qemu-char.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 287e0cd..e0d7220 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1911,15 +1911,25 @@ static void tcp_chr_read(void *opaque)
 {
     CharDriverState *chr = opaque;
     TCPCharDriver *s = chr->opaque;
+    struct msghdr msg = { 0, };
+    struct iovec iov[1];
     uint8_t buf[1024];
     int len, size;
 
     if (!s->connected || s->max_size <= 0)
         return;
+
     len = sizeof(buf);
     if (len > s->max_size)
         len = s->max_size;
-    size = recv(s->fd, (void *)buf, len, 0);
+
+    iov[0].iov_base = buf;
+    iov[0].iov_len = len;
+
+    msg.msg_iov = iov;
+    msg.msg_iovlen = 1;
+
+    size = recvmsg(s->fd, &msg, 0);
     if (size == 0) {
         /* connection closed */
         s->connected = 0;
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/3] Add SCM_RIGHTS support to unix socket character devices

[Mark McLoughlin]

If a file descriptor is passed via a message with SCM_RIGHTS ancillary
data on a unix socket, store the file descriptor for use in the
chr_read() handler. Close the file descriptor if it was not used.

The qemu_chr_get_msgfd() and monitor_get_msgfd() APIs provide access
to any passed descriptor.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 monitor.c   |    5 +++++
 monitor.h   |    1 +
 qemu-char.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-char.h |    2 ++
 4 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index bad79fe..f61f43d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2981,6 +2981,11 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size)
     cur_mon = old_mon;
 }
 
+int monitor_get_msgfd(Monitor *mon)
+{
+    return qemu_chr_get_msgfd(mon->chr);
+}
+
 static void monitor_command_cb(Monitor *mon, const char *cmdline, void *opaque)
 {
     monitor_suspend(mon);
diff --git a/monitor.h b/monitor.h
index 13e8cc7..af97a86 100644
--- a/monitor.h
+++ b/monitor.h
@@ -19,6 +19,7 @@ void monitor_resume(Monitor *mon);
 void monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
                                  BlockDriverCompletionFunc *completion_cb,
                                  void *opaque);
+int monitor_get_msgfd(Monitor *mon);
 
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap);
 void monitor_printf(Monitor *mon, const char *fmt, ...)
diff --git a/qemu-char.c b/qemu-char.c
index e0d7220..f06a614 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -168,6 +168,11 @@ void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len)
     s->chr_read(s->handler_opaque, buf, len);
 }
 
+int qemu_chr_get_msgfd(CharDriverState *s)
+{
+    return s->get_msgfd ? s->get_msgfd(s) : -1;
+}
+
 void qemu_chr_accept_input(CharDriverState *s)
 {
     if (s->chr_accept_input)
@@ -1832,6 +1837,7 @@ typedef struct {
     int do_telnetopt;
     int do_nodelay;
     int is_unix;
+    int msgfd;
 } TCPCharDriver;
 
 static void tcp_chr_accept(void *opaque);
@@ -1907,12 +1913,46 @@ static void tcp_chr_process_IAC_bytes(CharDriverState *chr,
     *size = j;
 }
 
+static int tcp_get_msgfd(CharDriverState *chr)
+{
+    TCPCharDriver *s = chr->opaque;
+
+    return s->msgfd;
+}
+
+static void unix_process_msgfd(CharDriverState *chr, struct msghdr *msg)
+{
+    TCPCharDriver *s = chr->opaque;
+    struct cmsghdr *cmsg;
+
+    for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
+        int fd;
+
+        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
+            cmsg->cmsg_level != SOL_SOCKET ||
+            cmsg->cmsg_type != SCM_RIGHTS)
+            continue;
+
+        fd = *((int *)CMSG_DATA(cmsg));
+        if (fd < 0)
+            continue;
+
+        if (s->msgfd != -1)
+            close(s->msgfd);
+        s->msgfd = fd;
+    }
+}
+
 static void tcp_chr_read(void *opaque)
 {
     CharDriverState *chr = opaque;
     TCPCharDriver *s = chr->opaque;
     struct msghdr msg = { 0, };
     struct iovec iov[1];
+    union {
+        struct cmsghdr cmsg;
+        char control[CMSG_SPACE(sizeof(int))];
+    } msg_control;
     uint8_t buf[1024];
     int len, size;
 
@@ -1928,6 +1968,8 @@ static void tcp_chr_read(void *opaque)
 
     msg.msg_iov = iov;
     msg.msg_iovlen = 1;
+    msg.msg_control = &msg_control;
+    msg.msg_controllen = sizeof(msg_control);
 
     size = recvmsg(s->fd, &msg, 0);
     if (size == 0) {
@@ -1940,10 +1982,16 @@ static void tcp_chr_read(void *opaque)
         closesocket(s->fd);
         s->fd = -1;
     } else if (size > 0) {
+        if (s->is_unix)
+            unix_process_msgfd(chr, &msg);
         if (s->do_telnetopt)
             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;
+        }
     }
 }
 
@@ -2105,12 +2153,14 @@ static CharDriverState *qemu_chr_open_tcp(const char *host_str,
     s->connected = 0;
     s->fd = -1;
     s->listen_fd = -1;
+    s->msgfd = -1;
     s->is_unix = is_unix;
     s->do_nodelay = do_nodelay && !is_unix;
 
     chr->opaque = s;
     chr->chr_write = tcp_chr_write;
     chr->chr_close = tcp_chr_close;
+    chr->get_msgfd = tcp_get_msgfd;
 
     if (is_listen) {
         s->listen_fd = fd;
diff --git a/qemu-char.h b/qemu-char.h
index e1aa8db..77d4eda 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -51,6 +51,7 @@ struct CharDriverState {
     int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, int len);
     void (*chr_update_read_handler)(struct CharDriverState *s);
     int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg);
+    int (*get_msgfd)(struct CharDriverState *s);
     IOEventHandler *chr_event;
     IOCanRWHandler *chr_can_read;
     IOReadHandler *chr_read;
@@ -81,6 +82,7 @@ void qemu_chr_reset(CharDriverState *s);
 void qemu_chr_initial_reset(void);
 int qemu_chr_can_read(CharDriverState *s);
 void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len);
+int qemu_chr_get_msgfd(CharDriverState *s);
 void qemu_chr_accept_input(CharDriverState *s);
 void qemu_chr_info(Monitor *mon);
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/3] Add support for fd=msgfd for tap and socket networking

[Mark McLoughlin]

This allows a program to initialize a host networking device using a
file descriptor passed over a unix monitor socket.

If the programs does e.g. "host_net_add tap fd=msgfd" it must pass
a file descriptor as part of that same message via SCM_RIGHTS ancillary
data.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |   39 ++++++++++++++++++++++++++++++++++-----
 1 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/net.c b/net.c
index 001ebcb..0bb8b52 100644
--- a/net.c
+++ b/net.c
@@ -2388,6 +2388,30 @@ void qemu_check_nic_model_list(NICInfo *nd, const char * const *models,
     exit(exit_status);
 }
 
+static int net_handle_fd_param(Monitor *mon, const char *param)
+{
+    if (!strcmp(param, "msgfd")) {
+        int fd;
+
+        fd = monitor_get_msgfd(mon);
+        if (fd == -1) {
+            config_error(mon, "No file descriptor found in ancillary data\n");
+            return -1;
+        }
+
+        fd = dup(fd);
+        if (fd == -1) {
+            config_error(mon, "Failed to dup() file descriptor: %s\n",
+                         strerror(errno));
+            return -1;
+        }
+
+        return fd;
+    } else {
+        return strtol(param, NULL, 0);
+    }
+}
+
 int net_client_init(Monitor *mon, const char *device, const char *p)
 {
     char buf[1024];
@@ -2625,12 +2649,15 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
             static const char * const fd_params[] = {
                 "vlan", "name", "fd", "sndbuf", NULL
             };
+            ret = -1;
             if (check_params(chkbuf, sizeof(chkbuf), fd_params, p) < 0) {
                 config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
-                ret = -1;
                 goto out;
             }
-            fd = strtol(buf, NULL, 0);
+            fd = net_handle_fd_param(mon, buf);
+            if (fd == -1) {
+                goto out;
+            }
             fcntl(fd, F_SETFL, O_NONBLOCK);
             s = net_tap_fd_init(vlan, device, name, fd);
         } else {
@@ -2670,13 +2697,15 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
                 "vlan", "name", "fd", NULL
             };
             int fd;
+            ret = -1;
             if (check_params(chkbuf, sizeof(chkbuf), fd_params, p) < 0) {
                 config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
-                ret = -1;
                 goto out;
             }
-            fd = strtol(buf, NULL, 0);
-            ret = -1;
+            fd = net_handle_fd_param(mon, buf);
+            if (fd == -1) {
+                goto out;
+            }
             if (net_socket_fd_init(vlan, device, name, fd, 1))
                 ret = 0;
         } else if (get_param_value(buf, sizeof(buf), "listen", p) > 0) {
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 0/3] Allow host_net_add monitor command accept file descriptors

[Avi Kivity]

On 07/06/2009 08:30 PM, Mark McLoughlin wrote:
> Hi,
> 	You can pass file descriptors to qemu via the command line using '-net
> tap,fd=' or '-net socket,tap='. However, you cannot currently do this
> via the monitor.
>
> 	libvirt always configures tap interfaces before passing them to qemu.
> One reason for this is to allow libvirtd to have privileges
> to /dev/net/tun without allowing qemu those privileges. Because of this,
> libvirt currently does not support NIC hotplug for qemu.
>
> 	The following three patches add support for passing a file descriptor
> to the monitor command by allowing file descriptors to be received over
> monitor commands on a unix socket.
>
>    

I'm a great fan of SCM_RIGHTS.  To make it easier to use in more 
commands, I think it makes sense to have a separate command to pass the fd:

   (qemu) getfd foo
   (qemu) getfd bar
   (qemu) getfd baz
   (qemu) pci_add ...,fd=foo
   (qemu) pci_add ...,fd=bar ...,fd=baz

A closefd command would be needed to deal with errors.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 0/3] Allow host_net_add monitor command accept file descriptors

[Mark McLoughlin]

On Tue, 2009-07-07 at 08:28 +0300, Avi Kivity wrote:
> On 07/06/2009 08:30 PM, Mark McLoughlin wrote:
> > Hi,
> > 	You can pass file descriptors to qemu via the command line using '-net
> > tap,fd=' or '-net socket,tap='. However, you cannot currently do this
> > via the monitor.
> >
> > 	libvirt always configures tap interfaces before passing them to qemu.
> > One reason for this is to allow libvirtd to have privileges
> > to /dev/net/tun without allowing qemu those privileges. Because of this,
> > libvirt currently does not support NIC hotplug for qemu.
> >
> > 	The following three patches add support for passing a file descriptor
> > to the monitor command by allowing file descriptors to be received over
> > monitor commands on a unix socket.
> >
> >    
> 
> I'm a great fan of SCM_RIGHTS.  To make it easier to use in more 
> commands, I think it makes sense to have a separate command to pass the fd:
> 
>    (qemu) getfd foo
>    (qemu) getfd bar
>    (qemu) getfd baz
>    (qemu) pci_add ...,fd=foo
>    (qemu) pci_add ...,fd=bar ...,fd=baz
> 
> A closefd command would be needed to deal with errors.

Nice idea, certainly.

However, since it's only currently useful for tap/socket networking, I'm
happier with not adding two new monitor commands and only supporting a
single fd for now.

Cheers,
Mark.

Re: [Qemu-devel] [PATCH 0/3] Allow host_net_add monitor command accept file descriptors

[Avi Kivity]

On 07/07/2009 10:43 AM, Mark McLoughlin wrote:
>
>>     (qemu) getfd foo
>>     (qemu) getfd bar
>>     (qemu) getfd baz
>>     (qemu) pci_add ...,fd=foo
>>     (qemu) pci_add ...,fd=bar ...,fd=baz
>>
>> A closefd command would be needed to deal with errors.
>>      
>
> Nice idea, certainly.
>
> However, since it's only currently useful for tap/socket networking, I'm
> happier with not adding two new monitor commands and only supporting a
> single fd for now.
>    

What happens when we do get more commands?  Do we then add the new 
commands and special-case fd=msgfd ?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 0/3] Allow host_net_add monitor command accept file descriptors

[Mark McLoughlin]

On Tue, 2009-07-07 at 10:52 +0300, Avi Kivity wrote:
> On 07/07/2009 10:43 AM, Mark McLoughlin wrote:
> >
> >>     (qemu) getfd foo
> >>     (qemu) getfd bar
> >>     (qemu) getfd baz
> >>     (qemu) pci_add ...,fd=foo
> >>     (qemu) pci_add ...,fd=bar ...,fd=baz
> >>
> >> A closefd command would be needed to deal with errors.
> >>      
> >
> > Nice idea, certainly.
> >
> > However, since it's only currently useful for tap/socket networking, I'm
> > happier with not adding two new monitor commands and only supporting a
> > single fd for now.
> >    
> 
> What happens when we do get more commands?

Any specific ideas around what else might use it?

> Do we then add the new commands and special-case fd=msgfd ?

Sounds like a fine plan to me. We could name an fd passed via the
current commands as "msgfd" and only the "getfd" command would have the
ability to assign another name.

Cheers,
Mark.

Re: [Qemu-devel] [PATCH 0/3] Allow host_net_add monitor command accept file descriptors

[Avi Kivity]

On 07/07/2009 11:13 AM, Mark McLoughlin wrote:
>>>
>>> Nice idea, certainly.
>>>
>>> However, since it's only currently useful for tap/socket networking, I'm
>>> happier with not adding two new monitor commands and only supporting a
>>> single fd for now.
>>>
>>>        
>> What happens when we do get more commands?
>>      
>
> Any specific ideas around what else might use it?
>    

Migration, usb, passing around eventfds for interguest communication.

>> Do we then add the new commands and special-case fd=msgfd ?
>>      
>
> Sounds like a fine plan to me. We could name an fd passed via the
> current commands as "msgfd" and only the "getfd" command would have the
> ability to assign another name.
>    

If we can't see ahead, it's fine to accumulate cruft.  When we can 
however we should avoid it.  The external interfaces are complicated enough.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 0/3] Allow host_net_add monitor command accept file descriptors

[Daniel P. Berrange]

On Tue, Jul 07, 2009 at 12:03:18PM +0300, Avi Kivity wrote:
> On 07/07/2009 11:13 AM, Mark McLoughlin wrote:
> >>>
> >>>Nice idea, certainly.
> >>>
> >>>However, since it's only currently useful for tap/socket networking, I'm
> >>>happier with not adding two new monitor commands and only supporting a
> >>>single fd for now.
> >>>
> >>>       
> >>What happens when we do get more commands?
> >>     
> >
> >Any specific ideas around what else might use it?
> >   
> 
> Migration, usb, passing around eventfds for interguest communication.

I must say the idea of being able to pass a FD for migration would be
very nice. We currently have to open a localhost TCP port and tell QEMU
to connect to is, which is less than desirable for security since we 
cannot guarentee the process we want is the one actually connecting.

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

Re: [Qemu-devel] [PATCH 0/3] Allow host_net_add monitor command accept file descriptors

[Mark McLoughlin]

On Tue, 2009-07-07 at 11:06 +0100, Daniel P. Berrange wrote:
> On Tue, Jul 07, 2009 at 12:03:18PM +0300, Avi Kivity wrote:
> > On 07/07/2009 11:13 AM, Mark McLoughlin wrote:
> > >>>
> > >>>Nice idea, certainly.
> > >>>
> > >>>However, since it's only currently useful for tap/socket networking, I'm
> > >>>happier with not adding two new monitor commands and only supporting a
> > >>>single fd for now.
> > >>>
> > >>>       
> > >>What happens when we do get more commands?
> > >>     
> > >
> > >Any specific ideas around what else might use it?
> > >   
> > 
> > Migration, usb, passing around eventfds for interguest communication.
> 
> I must say the idea of being able to pass a FD for migration would be
> very nice. We currently have to open a localhost TCP port and tell QEMU
> to connect to is, which is less than desirable for security since we 
> cannot guarentee the process we want is the one actually connecting.

Okay, I'm sold - following up with a version with getfd/closefd.

Cheers,
Mark.

[Qemu-devel] [PATCH 1/5] Make tcp_chr_read() use recvmsg()

[Mark McLoughlin]

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 qemu-char.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 287e0cd..e0d7220 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1911,15 +1911,25 @@ static void tcp_chr_read(void *opaque)
 {
     CharDriverState *chr = opaque;
     TCPCharDriver *s = chr->opaque;
+    struct msghdr msg = { 0, };
+    struct iovec iov[1];
     uint8_t buf[1024];
     int len, size;
 
     if (!s->connected || s->max_size <= 0)
         return;
+
     len = sizeof(buf);
     if (len > s->max_size)
         len = s->max_size;
-    size = recv(s->fd, (void *)buf, len, 0);
+
+    iov[0].iov_base = buf;
+    iov[0].iov_len = len;
+
+    msg.msg_iov = iov;
+    msg.msg_iovlen = 1;
+
+    size = recvmsg(s->fd, &msg, 0);
     if (size == 0) {
         /* connection closed */
         s->connected = 0;
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/5] Add SCM_RIGHTS support to unix socket character devices

[Mark McLoughlin]

If a file descriptor is passed via a message with SCM_RIGHTS ancillary
data on a unix socket, store the file descriptor for use in the
chr_read() handler. Close the file descriptor if it was not used.

The qemu_chr_get_msgfd() API provides access to the passed descriptor.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 qemu-char.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-char.h |    2 ++
 2 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index e0d7220..f06a614 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -168,6 +168,11 @@ void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len)
     s->chr_read(s->handler_opaque, buf, len);
 }
 
+int qemu_chr_get_msgfd(CharDriverState *s)
+{
+    return s->get_msgfd ? s->get_msgfd(s) : -1;
+}
+
 void qemu_chr_accept_input(CharDriverState *s)
 {
     if (s->chr_accept_input)
@@ -1832,6 +1837,7 @@ typedef struct {
     int do_telnetopt;
     int do_nodelay;
     int is_unix;
+    int msgfd;
 } TCPCharDriver;
 
 static void tcp_chr_accept(void *opaque);
@@ -1907,12 +1913,46 @@ static void tcp_chr_process_IAC_bytes(CharDriverState *chr,
     *size = j;
 }
 
+static int tcp_get_msgfd(CharDriverState *chr)
+{
+    TCPCharDriver *s = chr->opaque;
+
+    return s->msgfd;
+}
+
+static void unix_process_msgfd(CharDriverState *chr, struct msghdr *msg)
+{
+    TCPCharDriver *s = chr->opaque;
+    struct cmsghdr *cmsg;
+
+    for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
+        int fd;
+
+        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
+            cmsg->cmsg_level != SOL_SOCKET ||
+            cmsg->cmsg_type != SCM_RIGHTS)
+            continue;
+
+        fd = *((int *)CMSG_DATA(cmsg));
+        if (fd < 0)
+            continue;
+
+        if (s->msgfd != -1)
+            close(s->msgfd);
+        s->msgfd = fd;
+    }
+}
+
 static void tcp_chr_read(void *opaque)
 {
     CharDriverState *chr = opaque;
     TCPCharDriver *s = chr->opaque;
     struct msghdr msg = { 0, };
     struct iovec iov[1];
+    union {
+        struct cmsghdr cmsg;
+        char control[CMSG_SPACE(sizeof(int))];
+    } msg_control;
     uint8_t buf[1024];
     int len, size;
 
@@ -1928,6 +1968,8 @@ static void tcp_chr_read(void *opaque)
 
     msg.msg_iov = iov;
     msg.msg_iovlen = 1;
+    msg.msg_control = &msg_control;
+    msg.msg_controllen = sizeof(msg_control);
 
     size = recvmsg(s->fd, &msg, 0);
     if (size == 0) {
@@ -1940,10 +1982,16 @@ static void tcp_chr_read(void *opaque)
         closesocket(s->fd);
         s->fd = -1;
     } else if (size > 0) {
+        if (s->is_unix)
+            unix_process_msgfd(chr, &msg);
         if (s->do_telnetopt)
             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;
+        }
     }
 }
 
@@ -2105,12 +2153,14 @@ static CharDriverState *qemu_chr_open_tcp(const char *host_str,
     s->connected = 0;
     s->fd = -1;
     s->listen_fd = -1;
+    s->msgfd = -1;
     s->is_unix = is_unix;
     s->do_nodelay = do_nodelay && !is_unix;
 
     chr->opaque = s;
     chr->chr_write = tcp_chr_write;
     chr->chr_close = tcp_chr_close;
+    chr->get_msgfd = tcp_get_msgfd;
 
     if (is_listen) {
         s->listen_fd = fd;
diff --git a/qemu-char.h b/qemu-char.h
index e1aa8db..77d4eda 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -51,6 +51,7 @@ struct CharDriverState {
     int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, int len);
     void (*chr_update_read_handler)(struct CharDriverState *s);
     int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg);
+    int (*get_msgfd)(struct CharDriverState *s);
     IOEventHandler *chr_event;
     IOCanRWHandler *chr_can_read;
     IOReadHandler *chr_read;
@@ -81,6 +82,7 @@ void qemu_chr_reset(CharDriverState *s);
 void qemu_chr_initial_reset(void);
 int qemu_chr_can_read(CharDriverState *s);
 void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len);
+int qemu_chr_get_msgfd(CharDriverState *s);
 void qemu_chr_accept_input(CharDriverState *s);
 void qemu_chr_info(Monitor *mon);
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Mark McLoughlin]

Add monitor commands to support passing file descriptors via
SCM_RIGHTS.

getfd assigns the passed file descriptor a name for use with other
monitor commands.

closefd allows passed file descriptors to be closed. If a monitor
command actually uses a named file descriptor, closefd will not be
required.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 monitor.c       |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-monitor.hx |   18 ++++++++++++++
 2 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index bad79fe..85fa137 100644
--- a/monitor.c
+++ b/monitor.c
@@ -70,6 +70,14 @@ typedef struct mon_cmd_t {
     const char *help;
 } mon_cmd_t;
 
+/* file descriptors passed via SCM_RIGHTS */
+typedef struct mon_fd_t mon_fd_t;
+struct mon_fd_t {
+    char *name;
+    int fd;
+    LIST_ENTRY(mon_fd_t) next;
+};
+
 struct Monitor {
     CharDriverState *chr;
     int flags;
@@ -80,6 +88,7 @@ struct Monitor {
     CPUState *mon_cpu;
     BlockDriverCompletionFunc *password_completion_cb;
     void *password_opaque;
+    LIST_HEAD(,mon_fd_t) fds;
     LIST_ENTRY(Monitor) entry;
 };
 
@@ -1677,6 +1686,66 @@ static void do_acl_remove(Monitor *mon, const char *aclname, const char *match)
     }
 }
 
+static void do_getfd(Monitor *mon, const char *fdname)
+{
+    mon_fd_t *monfd;
+    int fd;
+
+    fd = qemu_chr_get_msgfd(mon->chr);
+    if (fd == -1) {
+        monitor_printf(mon, "getfd: no file descriptor supplied via SCM_RIGHTS\n");
+        return;
+    }
+
+    if (qemu_isdigit(fdname[0])) {
+        monitor_printf(mon, "getfd: monitor names may not begin with a number\n");
+        return;
+    }
+
+    fd = dup(fd);
+    if (fd == -1) {
+        monitor_printf(mon, "Failed to dup() file descriptor: %s\n",
+                       strerror(errno));
+        return;
+    }
+
+    LIST_FOREACH(monfd, &mon->fds, next) {
+        if (strcmp(monfd->name, fdname) != 0) {
+            continue;
+        }
+
+        close(monfd->fd);
+        monfd->fd = fd;
+        return;
+    }
+
+    monfd = qemu_mallocz(sizeof(mon_fd_t));
+    monfd->name = qemu_strdup(fdname);
+    monfd->fd = fd;
+
+    LIST_INSERT_HEAD(&mon->fds, monfd, next);
+}
+
+static void do_closefd(Monitor *mon, const char *fdname)
+{
+    mon_fd_t *monfd;
+
+    LIST_FOREACH(monfd, &mon->fds, next) {
+        if (strcmp(monfd->name, fdname) != 0) {
+            continue;
+        }
+
+        LIST_REMOVE(monfd, next);
+        close(monfd->fd);
+        qemu_free(monfd->name);
+        qemu_free(monfd);
+        return;
+    }
+
+    monitor_printf(mon, "Failed to find file descriptor named %s\n",
+                   fdname);
+}
+
 static const mon_cmd_t mon_cmds[] = {
 #include "qemu-monitor.h"
     { NULL, NULL, },
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index dc10b75..2279012 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -615,6 +615,24 @@ Remove all matches from the access control list, and set the default
 policy back to @code{deny}.
 ETEXI
 
+    { "getfd", "s", do_getfd, "getfd name",
+      "receive a file descriptor via SCM rights and assign it a name" },
+STEXI
+@item getfd @var{fdname}
+If a file descriptor is passed alongside this command using the SCM_RIGHTS
+mechanism on unix sockets, it is stored using the name @var{fdname} for
+later use by other monitor commands.
+ETEXI
+
+    { "closefd", "s", do_closefd, "closefd name",
+      "close a file descriptor previously passed via SCM rights" },
+STEXI
+@item closefd @var{fdname}
+Close the file descriptor previously assigned to @var{fdname} using the
+@code{getfd} command. This is only needed if the file descriptor was never
+used by another monitor command.
+ETEXI
+
 STEXI
 @end table
 ETEXI
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/5] Add monitor_get_fd() command for fetching named fds

[Mark McLoughlin]

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 monitor.c |   24 ++++++++++++++++++++++++
 monitor.h |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 85fa137..38e0da5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1746,6 +1746,30 @@ static void do_closefd(Monitor *mon, const char *fdname)
                    fdname);
 }
 
+int monitor_get_fd(Monitor *mon, const char *fdname)
+{
+    mon_fd_t *monfd;
+
+    LIST_FOREACH(monfd, &mon->fds, next) {
+        int fd;
+
+        if (strcmp(monfd->name, fdname) != 0) {
+            continue;
+        }
+
+        fd = monfd->fd;
+
+        /* caller takes ownership of fd */
+        LIST_REMOVE(monfd, next);
+        qemu_free(monfd->name);
+        qemu_free(monfd);
+
+        return fd;
+    }
+
+    return -1;
+}
+
 static const mon_cmd_t mon_cmds[] = {
 #include "qemu-monitor.h"
     { NULL, NULL, },
diff --git a/monitor.h b/monitor.h
index 13e8cc7..f6a43c0 100644
--- a/monitor.h
+++ b/monitor.h
@@ -20,6 +20,8 @@ void monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
                                  BlockDriverCompletionFunc *completion_cb,
                                  void *opaque);
 
+int monitor_get_fd(Monitor *mon, const char *fdname);
+
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap);
 void monitor_printf(Monitor *mon, const char *fmt, ...)
     __attribute__ ((__format__ (__printf__, 2, 3)));
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/5] Add support for fd=name to tap and socket networking

[Mark McLoughlin]

This allows a program to initialize a host networking device using a
file descriptor passed over a unix monitor socket.

The program must first pass the file descriptor using SCM_RIGHTS
ancillary data with the getfd monitor command. It then may do
"host_net_add tap fd=name" to use the named file descriptor.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |   42 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/net.c b/net.c
index 001ebcb..a97abb4 100644
--- a/net.c
+++ b/net.c
@@ -2388,6 +2388,23 @@ void qemu_check_nic_model_list(NICInfo *nd, const char * const *models,
     exit(exit_status);
 }
 
+static int net_handle_fd_param(Monitor *mon, const char *param)
+{
+    if (!qemu_isdigit(param[0])) {
+        int fd;
+
+        fd = monitor_get_fd(mon, param);
+        if (fd == -1) {
+            config_error(mon, "No file descriptor named %s found", param);
+            return -1;
+        }
+
+        return fd;
+    } else {
+        return strtol(param, NULL, 0);
+    }
+}
+
 int net_client_init(Monitor *mon, const char *device, const char *p)
 {
     char buf[1024];
@@ -2625,14 +2642,20 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
             static const char * const fd_params[] = {
                 "vlan", "name", "fd", "sndbuf", NULL
             };
+            ret = -1;
             if (check_params(chkbuf, sizeof(chkbuf), fd_params, p) < 0) {
                 config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
-                ret = -1;
                 goto out;
             }
-            fd = strtol(buf, NULL, 0);
+            fd = net_handle_fd_param(mon, buf);
+            if (fd == -1) {
+                goto out;
+            }
             fcntl(fd, F_SETFL, O_NONBLOCK);
             s = net_tap_fd_init(vlan, device, name, fd);
+            if (!s) {
+                close(fd);
+            }
         } else {
             static const char * const tap_params[] = {
                 "vlan", "name", "ifname", "script", "downscript", "sndbuf", NULL
@@ -2670,15 +2693,20 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
                 "vlan", "name", "fd", NULL
             };
             int fd;
+            ret = -1;
             if (check_params(chkbuf, sizeof(chkbuf), fd_params, p) < 0) {
                 config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
-                ret = -1;
                 goto out;
             }
-            fd = strtol(buf, NULL, 0);
-            ret = -1;
-            if (net_socket_fd_init(vlan, device, name, fd, 1))
-                ret = 0;
+            fd = net_handle_fd_param(mon, buf);
+            if (fd == -1) {
+                goto out;
+            }
+            if (!net_socket_fd_init(vlan, device, name, fd, 1)) {
+                close(fd);
+                goto out;
+            }
+            ret = 0;
         } else if (get_param_value(buf, sizeof(buf), "listen", p) > 0) {
             static const char * const listen_params[] = {
                 "vlan", "name", "listen", NULL
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Avi Kivity]

On 07/08/2009 05:57 PM, Mark McLoughlin wrote:
> Add monitor commands to support passing file descriptors via
> SCM_RIGHTS.
>
> getfd assigns the passed file descriptor a name for use with other
> monitor commands.
>
> closefd allows passed file descriptors to be closed. If a monitor
> command actually uses a named file descriptor, closefd will not be
> required.
>
>
>    

> @@ -70,6 +70,14 @@ typedef struct mon_cmd_t {
>       const char *help;
>   } mon_cmd_t;
>
> +/* file descriptors passed via SCM_RIGHTS */
> +typedef struct mon_fd_t mon_fd_t;
> +struct mon_fd_t {
> +    char *name;
> +    int fd;
> +    LIST_ENTRY(mon_fd_t) next;
> +};
> +
>    

The _t namespace is reserved by posix and not used for structures in 
qemu anyway.  I see there's precedent a few lines above but let's not 
introduce new violations.

> +
> +    fd = dup(fd);
>    

Why?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Mark McLoughlin]

On Wed, 2009-07-08 at 18:26 +0300, Avi Kivity wrote:
> On 07/08/2009 05:57 PM, Mark McLoughlin wrote:
> > Add monitor commands to support passing file descriptors via
> > SCM_RIGHTS.
> >
> > getfd assigns the passed file descriptor a name for use with other
> > monitor commands.
> >
> > closefd allows passed file descriptors to be closed. If a monitor
> > command actually uses a named file descriptor, closefd will not be
> > required.
> >
> >
> >    
> 
> > @@ -70,6 +70,14 @@ typedef struct mon_cmd_t {
> >       const char *help;
> >   } mon_cmd_t;
> >
> > +/* file descriptors passed via SCM_RIGHTS */
> > +typedef struct mon_fd_t mon_fd_t;
> > +struct mon_fd_t {
> > +    char *name;
> > +    int fd;
> > +    LIST_ENTRY(mon_fd_t) next;
> > +};
> > +
> >    
> 
> The _t namespace is reserved by posix and not used for structures in 
> qemu anyway.  I see there's precedent a few lines above but let's not 
> introduce new violations.

Bah, where there's a number of different styles used in a code base, and
where there's no guidelines in CODING_STYLE, I tend to stick with the
style of the code I'm editing. And, in this case, the closest structure
definition used this style.

> > +
> > +    fd = dup(fd);
> >    
> 
> Why?

Because it gets closed again after the chr_read() handler finishes. I
guess we could make qemu_chr_get_msgfd() pass ownership of the fd to the
caller.

Cheers,
Mark.

Re: [Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Avi Kivity]

On 07/08/2009 07:03 PM, Mark McLoughlin wrote:
>> The _t namespace is reserved by posix and not used for structures in
>> qemu anyway.  I see there's precedent a few lines above but let's not
>> introduce new violations.
>>      
>
> Bah, where there's a number of different styles used in a code base, and
> where there's no guidelines in CODING_STYLE, I tend to stick with the
> style of the code I'm editing. And, in this case, the closest structure
> definition used this style.
>
>    

Naming is one of the few things CODING_STYLE does talk about.

>>> +
>>> +    fd = dup(fd);
>>>
>>>        
>> Why?
>>      
>
> Because it gets closed again after the chr_read() handler finishes. I
> guess we could make qemu_chr_get_msgfd() pass ownership of the fd to the
> caller.
>    

I'd prefer the communication layer to queue fds and getfd to dequeue them.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Anthony Liguori]

Avi Kivity wrote:
> I'd prefer the communication layer to queue fds and getfd to dequeue 
> them.

How many do you queue?  The correct answer is one, btw ;-)

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Avi Kivity]

On 07/08/2009 09:08 PM, Anthony Liguori wrote:
> Avi Kivity wrote:
>> I'd prefer the communication layer to queue fds and getfd to dequeue 
>> them.
>
> How many do you queue?  The correct answer is one, btw ;-)

You queue as many as you receive, and you dequeue as many getfd commands 
as you get.

Nothing prevents the client from sending two getfd commands in a single 
packet.  We can either support it or start writing detailed 
documentation and handle the bug reports when people don't read it.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Anthony Liguori]

Avi Kivity wrote:
> On 07/08/2009 09:08 PM, Anthony Liguori wrote:
>> Avi Kivity wrote:
>>> I'd prefer the communication layer to queue fds and getfd to dequeue 
>>> them.
>>
>> How many do you queue?  The correct answer is one, btw ;-)
>
> You queue as many as you receive, and you dequeue as many getfd 
> commands as you get.

Then someone can connect to the monitor and consume an arbitrary number 
of fds?  I'd be very concerned about the potential to leak fds within 
QEMU from a poorly written client.  Seems like a very easy mistake to make.

> Nothing prevents the client from sending two getfd commands in a 
> single packet.  We can either support it or start writing detailed 
> documentation and handle the bug reports when people don't read it.

What would a client do that would result in this happening?  It's really 
a contrived example when you think about it pragmatically (at least 
given today's monitor).

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Avi Kivity]

On 07/08/2009 09:21 PM, Anthony Liguori wrote:
> Avi Kivity wrote:
>> On 07/08/2009 09:08 PM, Anthony Liguori wrote:
>>> Avi Kivity wrote:
>>>> I'd prefer the communication layer to queue fds and getfd to 
>>>> dequeue them.
>>>
>>> How many do you queue?  The correct answer is one, btw ;-)
>>
>> You queue as many as you receive, and you dequeue as many getfd 
>> commands as you get.
>
> Then someone can connect to the monitor and consume an arbitrary 
> number of fds?  I'd be very concerned about the potential to leak fds 
> within QEMU from a poorly written client.  Seems like a very easy 
> mistake to make.

Well, that's intrinsic to the getfd command.  We could limit it by 
saying we support a set number of fds, or even give them fixed names.

>
>> Nothing prevents the client from sending two getfd commands in a 
>> single packet.  We can either support it or start writing detailed 
>> documentation and handle the bug reports when people don't read it.
>
> What would a client do that would result in this happening?  It's 
> really a contrived example when you think about it pragmatically (at 
> least given today's monitor).

I am in fact thinking of tomorrow's monitor.  If indeed we follow an rpc 
model, it should be quite easy to have multiple threads (each doing an 
unrelated task) each issuing a series of commands and processing the 
replies.  There would be a lock protecting the socket, but there would 
be no reason to limit the number of outstanding commands.  I think 
that's a perfectly reasonable way to write a client.

If the client is written in a high level language it's also reasonable 
that some buffering would take place and you'd see a single packet 
containing multiple commands, or a command split into multiple packets.  
Therefore I'd like to avoid any assumptions in this area.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Anthony Liguori]

Avi Kivity wrote:
> On 07/08/2009 09:21 PM, Anthony Liguori wrote:
>> Then someone can connect to the monitor and consume an arbitrary 
>> number of fds?  I'd be very concerned about the potential to leak fds 
>> within QEMU from a poorly written client.  Seems like a very easy 
>> mistake to make.
>
> Well, that's intrinsic to the getfd command.  We could limit it by 
> saying we support a set number of fds, or even give them fixed names.

So what's the set number and why isn't 1 a reasonable fixed number?

>>> Nothing prevents the client from sending two getfd commands in a 
>>> single packet.  We can either support it or start writing detailed 
>>> documentation and handle the bug reports when people don't read it.
>>
>> What would a client do that would result in this happening?  It's 
>> really a contrived example when you think about it pragmatically (at 
>> least given today's monitor).
>
> I am in fact thinking of tomorrow's monitor.

Indeed, I figured as much :-)

>   If indeed we follow an rpc model, it should be quite easy to have 
> multiple threads (each doing an unrelated task) each issuing a series 
> of commands and processing the replies.  There would be a lock 
> protecting the socket, but there would be no reason to limit the 
> number of outstanding commands.  I think that's a perfectly reasonable 
> way to write a client.

You can't just use a lock on the socket for transmit because you have to 
deal with partial transmissions and the subsequent queuing.  You need a 
single queue mechanism shared by all writers and you need writers to 
submit the full rpcs so that they can be queued in the proper order.

When we get to the point of the RPC model though, we'll have one monitor 
state for each asynchronous message we're processing.  I contend that we 
only ever want to pass a single fd per command so if we stick with 
queueing 1 fd per monitor today, it'll map quite nicely to how we would 
handle things in the RPC model.

> If the client is written in a high level language it's also reasonable 
> that some buffering would take place and you'd see a single packet 
> containing multiple commands, or a command split into multiple 
> packets.  Therefore I'd like to avoid any assumptions in this area.

That misses the point though.  We process one command at a time in the 
monitor so we only need to buffer one fd at a time.  When we start to 
process multiple commands at once in the monitor, we'll do so with 
multiple monitor states and we'll want to have one fd per monitor state.

Message boundaries really don't matter.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Avi Kivity]

On 07/08/2009 09:50 PM, Anthony Liguori wrote:
> Avi Kivity wrote:
>> On 07/08/2009 09:21 PM, Anthony Liguori wrote:
>>> Then someone can connect to the monitor and consume an arbitrary 
>>> number of fds?  I'd be very concerned about the potential to leak 
>>> fds within QEMU from a poorly written client.  Seems like a very 
>>> easy mistake to make.
>>
>> Well, that's intrinsic to the getfd command.  We could limit it by 
>> saying we support a set number of fds, or even give them fixed names.
>
> So what's the set number 

Good question.

> and why isn't 1 a reasonable fixed number?
>

Parallel execution of commands (which I don't propose anytime soon) and 
commands that accept multiple fds (maybe a command that accepts an 
irqfd/ioeventfd pair from another guest and exposes the pair to the 
guest as part of a pci device).

>
>>   If indeed we follow an rpc model, it should be quite easy to have 
>> multiple threads (each doing an unrelated task) each issuing a series 
>> of commands and processing the replies.  There would be a lock 
>> protecting the socket, but there would be no reason to limit the 
>> number of outstanding commands.  I think that's a perfectly 
>> reasonable way to write a client.
>
> You can't just use a lock on the socket for transmit because you have 
> to deal with partial transmissions and the subsequent queuing.  You 
> need a single queue mechanism shared by all writers and you need 
> writers to submit the full rpcs so that they can be queued in the 
> proper order.
>
> When we get to the point of the RPC model though, we'll have one 
> monitor state for each asynchronous message we're processing.  I 
> contend that we only ever want to pass a single fd per command so if 
> we stick with queueing 1 fd per monitor today, it'll map quite nicely 
> to how we would handle things in the RPC model.

I'm thinking about the client, not qemu, but you're right that the fd 
and the message that refer to it must be in a single transaction.  So at 
the qemu_chr level, we'll never queue more than one fd.

>
>> If the client is written in a high level language it's also 
>> reasonable that some buffering would take place and you'd see a 
>> single packet containing multiple commands, or a command split into 
>> multiple packets.  Therefore I'd like to avoid any assumptions in 
>> this area.
>
> That misses the point though.  We process one command at a time in the 
> monitor so we only need to buffer one fd at a time.  When we start to 
> process multiple commands at once in the monitor, we'll do so with 
> multiple monitor states and we'll want to have one fd per monitor state.

Again I'm thinking of the client.  If two client threads issue commands 
in parallel there'd be >1 fds on the wire.  But qemu can consume them 
sequentially so I agree queue may hold just a single fd.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Jamie Lokier]

Avi Kivity wrote:
> >That misses the point though.  We process one command at a time in the 
> >monitor so we only need to buffer one fd at a time.  When we start to 
> >process multiple commands at once in the monitor, we'll do so with 
> >multiple monitor states and we'll want to have one fd per monitor state.
> 
> Again I'm thinking of the client.  If two client threads issue commands 
> in parallel there'd be >1 fds on the wire.  But qemu can consume them 
> sequentially so I agree queue may hold just a single fd.

No, even though QEMU processes commands sequentially.

If the client sends

    "command_1_with_fd" + FD1
    "command_2_with_fd" + FD2

and then waits for the results of commands 1 and 2,
QEMU calls recvmsg() and will read

    "command_1_with_fd\r\ncommand_2_with_fd\r\n" + FD1 + FD2

unless QEMU is consuming only one byte at a time from the socket, and
dispatching commands as soon as it sees the line terminator.

If QEMU reads more than one byte at a time, and the client sends more
than one command without waiting for the previous one's result, QEMU
needs to buffer more FDs otherwise some will get lost.

-- Jamie

Re: [Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Mark McLoughlin]

On Wed, 2009-07-08 at 22:52 +0300, Avi Kivity wrote:
> On 07/08/2009 09:50 PM, Anthony Liguori wrote:
> > Avi Kivity wrote:
> >> If the client is written in a high level language it's also 
> >> reasonable that some buffering would take place and you'd see a 
> >> single packet containing multiple commands, or a command split into 
> >> multiple packets.  Therefore I'd like to avoid any assumptions in 
> >> this area.
> >
> > That misses the point though.  We process one command at a time in the 
> > monitor so we only need to buffer one fd at a time.  When we start to 
> > process multiple commands at once in the monitor, we'll do so with 
> > multiple monitor states and we'll want to have one fd per monitor state.
> 
> Again I'm thinking of the client.  If two client threads issue commands 
> in parallel there'd be >1 fds on the wire.  But qemu can consume them 
> sequentially so I agree queue may hold just a single fd.

Okay, it sounds like these patches didn't get applied for 0.11 because
of confusion over the conclusion here.

My take on it is that we're all agreed that it's never valid with the
current monitor protocol for a client to queue up multiple fds. It's a
synchronous protocol, you can't send multiple commands at once.

I'll re-post the patches in reply to this.

Thanks,
Mark.

[Qemu-devel] [PATCH] Make tcp_chr_read() use recvmsg()

[Mark McLoughlin]

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 qemu-char.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 287e0cd..e0d7220 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1911,15 +1911,25 @@ static void tcp_chr_read(void *opaque)
 {
     CharDriverState *chr = opaque;
     TCPCharDriver *s = chr->opaque;
+    struct msghdr msg = { 0, };
+    struct iovec iov[1];
     uint8_t buf[1024];
     int len, size;
 
     if (!s->connected || s->max_size <= 0)
         return;
+
     len = sizeof(buf);
     if (len > s->max_size)
         len = s->max_size;
-    size = recv(s->fd, (void *)buf, len, 0);
+
+    iov[0].iov_base = buf;
+    iov[0].iov_len = len;
+
+    msg.msg_iov = iov;
+    msg.msg_iovlen = 1;
+
+    size = recvmsg(s->fd, &msg, 0);
     if (size == 0) {
         /* connection closed */
         s->connected = 0;
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH] Make tcp_chr_read() use recvmsg()

[Blue Swirl]

On Tue, Jul 21, 2009 at 7:53 PM, Mark McLoughlin<markmc@redhat.com> wrote:
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>

A quick grep with mingw32 headers didn't find recvmsg.

Re: [Qemu-devel] [PATCH] Make tcp_chr_read() use recvmsg()

[Jamie Lokier]

Blue Swirl wrote:
> On Tue, Jul 21, 2009 at 7:53 PM, Mark McLoughlin<markmc@redhat.com> wrote:
> > Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> 
> A quick grep with mingw32 headers didn't find recvmsg.

It's not surprising.

Win32 sockets (Winsock) don't have recvmsg, and don't support
SCM_RIGHTS.  Or even unix domain sockets :-)  File descriptors are
passed in a different way on Win32.

-- Jamie

Re: [Qemu-devel] [PATCH] Make tcp_chr_read() use recvmsg()

[Mark McLoughlin]

On Tue, 2009-07-21 at 20:13 +0300, Blue Swirl wrote:
> On Tue, Jul 21, 2009 at 7:53 PM, Mark McLoughlin<markmc@redhat.com> wrote:
> > Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> 
> A quick grep with mingw32 headers didn't find recvmsg.

Thanks, my bad - fixed version coming up.

Cheers,
Mark.

[Qemu-devel] [PATCH 1/5] Make tcp_chr_read() use recvmsg()

[Mark McLoughlin]

Split out tcp_chr_recv() out of tcp_chr_read() and implement it on
non-win32 using recvmsg(). This is needed for a subsequent patch
which implements SCM_RIGHTS support.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 qemu-char.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 287e0cd..9886228 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1907,6 +1907,29 @@ static void tcp_chr_process_IAC_bytes(CharDriverState *chr,
     *size = j;
 }
 
+#ifndef WIN32
+static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
+{
+    TCPCharDriver *s = chr->opaque;
+    struct msghdr msg = { 0, };
+    struct iovec iov[1];
+
+    iov[0].iov_base = buf;
+    iov[0].iov_len = len;
+
+    msg.msg_iov = iov;
+    msg.msg_iovlen = 1;
+
+    return recvmsg(s->fd, &msg, 0);
+}
+#else
+static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
+{
+    TCPCharDriver *s = chr->opaque;
+    return recv(s->fd, buf, len, 0);
+}
+#endif
+
 static void tcp_chr_read(void *opaque)
 {
     CharDriverState *chr = opaque;
@@ -1919,7 +1942,7 @@ static void tcp_chr_read(void *opaque)
     len = sizeof(buf);
     if (len > s->max_size)
         len = s->max_size;
-    size = recv(s->fd, (void *)buf, len, 0);
+    size = tcp_chr_recv(chr, (void *)buf, len);
     if (size == 0) {
         /* connection closed */
         s->connected = 0;
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/5] Add SCM_RIGHTS support to unix socket character devices

[Mark McLoughlin]

If a file descriptor is passed via a message with SCM_RIGHTS ancillary
data on a unix socket, store the file descriptor for use in the
chr_read() handler. Close the file descriptor if it was not used.

The qemu_chr_get_msgfd() API provides access to the passed descriptor.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 qemu-char.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 qemu-char.h |    2 ++
 2 files changed, 56 insertions(+), 1 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 9886228..6e2cd31 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -168,6 +168,11 @@ void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len)
     s->chr_read(s->handler_opaque, buf, len);
 }
 
+int qemu_chr_get_msgfd(CharDriverState *s)
+{
+    return s->get_msgfd ? s->get_msgfd(s) : -1;
+}
+
 void qemu_chr_accept_input(CharDriverState *s)
 {
     if (s->chr_accept_input)
@@ -1832,6 +1837,7 @@ typedef struct {
     int do_telnetopt;
     int do_nodelay;
     int is_unix;
+    int msgfd;
 } TCPCharDriver;
 
 static void tcp_chr_accept(void *opaque);
@@ -1907,20 +1913,61 @@ static void tcp_chr_process_IAC_bytes(CharDriverState *chr,
     *size = j;
 }
 
+static int tcp_get_msgfd(CharDriverState *chr)
+{
+    TCPCharDriver *s = chr->opaque;
+
+    return s->msgfd;
+}
+
 #ifndef WIN32
+static void unix_process_msgfd(CharDriverState *chr, struct msghdr *msg)
+{
+    TCPCharDriver *s = chr->opaque;
+    struct cmsghdr *cmsg;
+
+    for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
+        int fd;
+
+        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
+            cmsg->cmsg_level != SOL_SOCKET ||
+            cmsg->cmsg_type != SCM_RIGHTS)
+            continue;
+
+        fd = *((int *)CMSG_DATA(cmsg));
+        if (fd < 0)
+            continue;
+
+        if (s->msgfd != -1)
+            close(s->msgfd);
+        s->msgfd = fd;
+    }
+}
+
 static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
 {
     TCPCharDriver *s = chr->opaque;
     struct msghdr msg = { 0, };
     struct iovec iov[1];
+    union {
+        struct cmsghdr cmsg;
+        char control[CMSG_SPACE(sizeof(int))];
+    } msg_control;
+    ssize_t ret;
 
     iov[0].iov_base = buf;
     iov[0].iov_len = len;
 
     msg.msg_iov = iov;
     msg.msg_iovlen = 1;
+    msg.msg_control = &msg_control;
+    msg.msg_controllen = sizeof(msg_control);
+
+    ret = recvmsg(s->fd, &msg, 0);
+    if (ret > 0 && s->is_unix)
+        unix_process_msgfd(chr, &msg);
 
-    return recvmsg(s->fd, &msg, 0);
+    return ret;
 }
 #else
 static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
@@ -1957,6 +2004,10 @@ 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;
+        }
     }
 }
 
@@ -2118,12 +2169,14 @@ static CharDriverState *qemu_chr_open_tcp(const char *host_str,
     s->connected = 0;
     s->fd = -1;
     s->listen_fd = -1;
+    s->msgfd = -1;
     s->is_unix = is_unix;
     s->do_nodelay = do_nodelay && !is_unix;
 
     chr->opaque = s;
     chr->chr_write = tcp_chr_write;
     chr->chr_close = tcp_chr_close;
+    chr->get_msgfd = tcp_get_msgfd;
 
     if (is_listen) {
         s->listen_fd = fd;
diff --git a/qemu-char.h b/qemu-char.h
index e1aa8db..77d4eda 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -51,6 +51,7 @@ struct CharDriverState {
     int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, int len);
     void (*chr_update_read_handler)(struct CharDriverState *s);
     int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg);
+    int (*get_msgfd)(struct CharDriverState *s);
     IOEventHandler *chr_event;
     IOCanRWHandler *chr_can_read;
     IOReadHandler *chr_read;
@@ -81,6 +82,7 @@ void qemu_chr_reset(CharDriverState *s);
 void qemu_chr_initial_reset(void);
 int qemu_chr_can_read(CharDriverState *s);
 void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len);
+int qemu_chr_get_msgfd(CharDriverState *s);
 void qemu_chr_accept_input(CharDriverState *s);
 void qemu_chr_info(Monitor *mon);
 
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 2/5] Add SCM_RIGHTS support to unix socket character devices

[Cam Macdonell]

Mark McLoughlin wrote:
> If a file descriptor is passed via a message with SCM_RIGHTS ancillary
> data on a unix socket, store the file descriptor for use in the
> chr_read() handler. Close the file descriptor if it was not used.
> 
> The qemu_chr_get_msgfd() API provides access to the passed descriptor.
> 
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> ---
>  qemu-char.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  qemu-char.h |    2 ++
>  2 files changed, 56 insertions(+), 1 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 9886228..6e2cd31 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -168,6 +168,11 @@ void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len)
>      s->chr_read(s->handler_opaque, buf, len);
>  }
>  
> +int qemu_chr_get_msgfd(CharDriverState *s)
> +{
> +    return s->get_msgfd ? s->get_msgfd(s) : -1;
> +}
> +
>  void qemu_chr_accept_input(CharDriverState *s)
>  {
>      if (s->chr_accept_input)
> @@ -1832,6 +1837,7 @@ typedef struct {
>      int do_telnetopt;
>      int do_nodelay;
>      int is_unix;
> +    int msgfd;
>  } TCPCharDriver;
>  
>  static void tcp_chr_accept(void *opaque);
> @@ -1907,20 +1913,61 @@ static void tcp_chr_process_IAC_bytes(CharDriverState *chr,
>      *size = j;
>  }
>  
> +static int tcp_get_msgfd(CharDriverState *chr)
> +{
> +    TCPCharDriver *s = chr->opaque;
> +
> +    return s->msgfd;
> +}
> +
>  #ifndef WIN32
> +static void unix_process_msgfd(CharDriverState *chr, struct msghdr *msg)
> +{
> +    TCPCharDriver *s = chr->opaque;
> +    struct cmsghdr *cmsg;
> +
> +    for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
> +        int fd;
> +
> +        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
> +            cmsg->cmsg_level != SOL_SOCKET ||
> +            cmsg->cmsg_type != SCM_RIGHTS)
> +            continue;
> +
> +        fd = *((int *)CMSG_DATA(cmsg));
> +        if (fd < 0)
> +            continue;
> +
> +        if (s->msgfd != -1)
> +            close(s->msgfd);
> +        s->msgfd = fd;
> +    }
> +}
> +
>  static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
>  {
>      TCPCharDriver *s = chr->opaque;
>      struct msghdr msg = { 0, };
>      struct iovec iov[1];
> +    union {
> +        struct cmsghdr cmsg;
> +        char control[CMSG_SPACE(sizeof(int))];
> +    } msg_control;
> +    ssize_t ret;
>  
>      iov[0].iov_base = buf;
>      iov[0].iov_len = len;
>  
>      msg.msg_iov = iov;
>      msg.msg_iovlen = 1;
> +    msg.msg_control = &msg_control;
> +    msg.msg_controllen = sizeof(msg_control);
> +
> +    ret = recvmsg(s->fd, &msg, 0);
> +    if (ret > 0 && s->is_unix)
> +        unix_process_msgfd(chr, &msg);
>  
> -    return recvmsg(s->fd, &msg, 0);
> +    return ret;
>  }
>  #else
>  static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
> @@ -1957,6 +2004,10 @@ 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;
> +        }

Hi Mark,

I'm trying to understand the intended behaviour here so I can use your 
patch.  In your comments for the patch it reads "store the file 
descriptor for use in the chr_read() handler.  Close the file descriptor 
if it was not used."  It seems that upon returning from the chr_read() 
handler the descriptor is closed if s->msgfd is not set to -1.  Does 
this mean the handler should set the fd to -1 after retrieving it if the 
process wants to keep that fd open, otherwise it will be closed when 
chr_read() returns?  Should tcp_get_msgfd() perhaps set  s->msgfd to -1 
after it is retrieved?  Sort of a "read and clear" behaviour?

Thanks,
Cam

>      }
>  }
>  
> @@ -2118,12 +2169,14 @@ static CharDriverState *qemu_chr_open_tcp(const char *host_str,
>      s->connected = 0;
>      s->fd = -1;
>      s->listen_fd = -1;
> +    s->msgfd = -1;
>      s->is_unix = is_unix;
>      s->do_nodelay = do_nodelay && !is_unix;
>  
>      chr->opaque = s;
>      chr->chr_write = tcp_chr_write;
>      chr->chr_close = tcp_chr_close;
> +    chr->get_msgfd = tcp_get_msgfd;
>  
>      if (is_listen) {
>          s->listen_fd = fd;
> diff --git a/qemu-char.h b/qemu-char.h
> index e1aa8db..77d4eda 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -51,6 +51,7 @@ struct CharDriverState {
>      int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, int len);
>      void (*chr_update_read_handler)(struct CharDriverState *s);
>      int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg);
> +    int (*get_msgfd)(struct CharDriverState *s);
>      IOEventHandler *chr_event;
>      IOCanRWHandler *chr_can_read;
>      IOReadHandler *chr_read;
> @@ -81,6 +82,7 @@ void qemu_chr_reset(CharDriverState *s);
>  void qemu_chr_initial_reset(void);
>  int qemu_chr_can_read(CharDriverState *s);
>  void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len);
> +int qemu_chr_get_msgfd(CharDriverState *s);
>  void qemu_chr_accept_input(CharDriverState *s);
>  void qemu_chr_info(Monitor *mon);
>  

Re: [Qemu-devel] [PATCH 2/5] Add SCM_RIGHTS support to unix socket character devices

[Mark McLoughlin]

On Thu, 2009-08-13 at 10:20 -0600, Cam Macdonell wrote:
> > @@ -1957,6 +2004,10 @@ 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;
> > +        }
> 
> Hi Mark,
> 
> I'm trying to understand the intended behaviour here so I can use your 
> patch.  In your comments for the patch it reads "store the file 
> descriptor for use in the chr_read() handler.  Close the file descriptor 
> if it was not used."  It seems that upon returning from the chr_read() 
> handler the descriptor is closed if s->msgfd is not set to -1.  Does 
> this mean the handler should set the fd to -1 after retrieving it if the 
> process wants to keep that fd open, otherwise it will be closed when 
> chr_read() returns?  Should tcp_get_msgfd() perhaps set  s->msgfd to -1 
> after it is retrieved?  Sort of a "read and clear" behaviour?

Yes, that would work and TBH I can't recall why I chose not to do that.
It'd be like popping the fd from a one item queue.

The current caller of get_msgfd() dup()s the fd which achieves the same
thing.

Cheers,
Mark.

[Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Mark McLoughlin]

Add monitor commands to support passing file descriptors via
SCM_RIGHTS.

getfd assigns the passed file descriptor a name for use with other
monitor commands.

closefd allows passed file descriptors to be closed. If a monitor
command actually uses a named file descriptor, closefd will not be
required.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 monitor.c       |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-monitor.hx |   18 ++++++++++++++
 2 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index e38c86e..817e4b7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -70,6 +70,14 @@ typedef struct mon_cmd_t {
     const char *help;
 } mon_cmd_t;
 
+/* file descriptors passed via SCM_RIGHTS */
+typedef struct mon_fd_t mon_fd_t;
+struct mon_fd_t {
+    char *name;
+    int fd;
+    LIST_ENTRY(mon_fd_t) next;
+};
+
 struct Monitor {
     CharDriverState *chr;
     int flags;
@@ -80,6 +88,7 @@ struct Monitor {
     CPUState *mon_cpu;
     BlockDriverCompletionFunc *password_completion_cb;
     void *password_opaque;
+    LIST_HEAD(,mon_fd_t) fds;
     LIST_ENTRY(Monitor) entry;
 };
 
@@ -1705,6 +1714,66 @@ static void do_inject_mce(Monitor *mon,
 }
 #endif
 
+static void do_getfd(Monitor *mon, const char *fdname)
+{
+    mon_fd_t *monfd;
+    int fd;
+
+    fd = qemu_chr_get_msgfd(mon->chr);
+    if (fd == -1) {
+        monitor_printf(mon, "getfd: no file descriptor supplied via SCM_RIGHTS\n");
+        return;
+    }
+
+    if (qemu_isdigit(fdname[0])) {
+        monitor_printf(mon, "getfd: monitor names may not begin with a number\n");
+        return;
+    }
+
+    fd = dup(fd);
+    if (fd == -1) {
+        monitor_printf(mon, "Failed to dup() file descriptor: %s\n",
+                       strerror(errno));
+        return;
+    }
+
+    LIST_FOREACH(monfd, &mon->fds, next) {
+        if (strcmp(monfd->name, fdname) != 0) {
+            continue;
+        }
+
+        close(monfd->fd);
+        monfd->fd = fd;
+        return;
+    }
+
+    monfd = qemu_mallocz(sizeof(mon_fd_t));
+    monfd->name = qemu_strdup(fdname);
+    monfd->fd = fd;
+
+    LIST_INSERT_HEAD(&mon->fds, monfd, next);
+}
+
+static void do_closefd(Monitor *mon, const char *fdname)
+{
+    mon_fd_t *monfd;
+
+    LIST_FOREACH(monfd, &mon->fds, next) {
+        if (strcmp(monfd->name, fdname) != 0) {
+            continue;
+        }
+
+        LIST_REMOVE(monfd, next);
+        close(monfd->fd);
+        qemu_free(monfd->name);
+        qemu_free(monfd);
+        return;
+    }
+
+    monitor_printf(mon, "Failed to find file descriptor named %s\n",
+                   fdname);
+}
+
 static const mon_cmd_t mon_cmds[] = {
 #include "qemu-monitor.h"
     { NULL, NULL, },
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 70e2475..11bdb2c 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -628,6 +628,24 @@ STEXI
 Inject an MCE on the given CPU (x86 only).
 ETEXI
 
+    { "getfd", "s", do_getfd, "getfd name",
+      "receive a file descriptor via SCM rights and assign it a name" },
+STEXI
+@item getfd @var{fdname}
+If a file descriptor is passed alongside this command using the SCM_RIGHTS
+mechanism on unix sockets, it is stored using the name @var{fdname} for
+later use by other monitor commands.
+ETEXI
+
+    { "closefd", "s", do_closefd, "closefd name",
+      "close a file descriptor previously passed via SCM rights" },
+STEXI
+@item closefd @var{fdname}
+Close the file descriptor previously assigned to @var{fdname} using the
+@code{getfd} command. This is only needed if the file descriptor was never
+used by another monitor command.
+ETEXI
+
 STEXI
 @end table
 ETEXI
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/5] Add monitor_get_fd() command for fetching named fds

[Mark McLoughlin]

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 monitor.c |   24 ++++++++++++++++++++++++
 monitor.h |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 817e4b7..6ad2e14 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1774,6 +1774,30 @@ static void do_closefd(Monitor *mon, const char *fdname)
                    fdname);
 }
 
+int monitor_get_fd(Monitor *mon, const char *fdname)
+{
+    mon_fd_t *monfd;
+
+    LIST_FOREACH(monfd, &mon->fds, next) {
+        int fd;
+
+        if (strcmp(monfd->name, fdname) != 0) {
+            continue;
+        }
+
+        fd = monfd->fd;
+
+        /* caller takes ownership of fd */
+        LIST_REMOVE(monfd, next);
+        qemu_free(monfd->name);
+        qemu_free(monfd);
+
+        return fd;
+    }
+
+    return -1;
+}
+
 static const mon_cmd_t mon_cmds[] = {
 #include "qemu-monitor.h"
     { NULL, NULL, },
diff --git a/monitor.h b/monitor.h
index 13e8cc7..f6a43c0 100644
--- a/monitor.h
+++ b/monitor.h
@@ -20,6 +20,8 @@ void monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
                                  BlockDriverCompletionFunc *completion_cb,
                                  void *opaque);
 
+int monitor_get_fd(Monitor *mon, const char *fdname);
+
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap);
 void monitor_printf(Monitor *mon, const char *fmt, ...)
     __attribute__ ((__format__ (__printf__, 2, 3)));
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/5] Add support for fd=name to tap and socket networking

[Mark McLoughlin]

This allows a program to initialize a host networking device using a
file descriptor passed over a unix monitor socket.

The program must first pass the file descriptor using SCM_RIGHTS
ancillary data with the getfd monitor command. It then may do
"host_net_add tap fd=name" to use the named file descriptor.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |   42 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/net.c b/net.c
index 90cf912..1838692 100644
--- a/net.c
+++ b/net.c
@@ -2410,6 +2410,23 @@ void qemu_check_nic_model_list(NICInfo *nd, const char * const *models,
     exit(exit_status);
 }
 
+static int net_handle_fd_param(Monitor *mon, const char *param)
+{
+    if (!qemu_isdigit(param[0])) {
+        int fd;
+
+        fd = monitor_get_fd(mon, param);
+        if (fd == -1) {
+            config_error(mon, "No file descriptor named %s found", param);
+            return -1;
+        }
+
+        return fd;
+    } else {
+        return strtol(param, NULL, 0);
+    }
+}
+
 int net_client_init(Monitor *mon, const char *device, const char *p)
 {
     char buf[1024];
@@ -2650,14 +2667,20 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
             static const char * const fd_params[] = {
                 "vlan", "name", "fd", "sndbuf", NULL
             };
+            ret = -1;
             if (check_params(chkbuf, sizeof(chkbuf), fd_params, p) < 0) {
                 config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
-                ret = -1;
                 goto out;
             }
-            fd = strtol(buf, NULL, 0);
+            fd = net_handle_fd_param(mon, buf);
+            if (fd == -1) {
+                goto out;
+            }
             fcntl(fd, F_SETFL, O_NONBLOCK);
             s = net_tap_fd_init(vlan, device, name, fd);
+            if (!s) {
+                close(fd);
+            }
         } else {
             static const char * const tap_params[] = {
                 "vlan", "name", "ifname", "script", "downscript", "sndbuf", NULL
@@ -2697,15 +2720,20 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
                 "vlan", "name", "fd", NULL
             };
             int fd;
+            ret = -1;
             if (check_params(chkbuf, sizeof(chkbuf), fd_params, p) < 0) {
                 config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
-                ret = -1;
                 goto out;
             }
-            fd = strtol(buf, NULL, 0);
-            ret = -1;
-            if (net_socket_fd_init(vlan, device, name, fd, 1))
-                ret = 0;
+            fd = net_handle_fd_param(mon, buf);
+            if (fd == -1) {
+                goto out;
+            }
+            if (!net_socket_fd_init(vlan, device, name, fd, 1)) {
+                close(fd);
+                goto out;
+            }
+            ret = 0;
         } else if (get_param_value(buf, sizeof(buf), "listen", p) > 0) {
             static const char * const listen_params[] = {
                 "vlan", "name", "listen", NULL
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 5/5] Add support for fd=name to tap and socket networking

[Mark McLoughlin]

On Wed, 2009-07-22 at 09:11 +0100, Mark McLoughlin wrote:
> This allows a program to initialize a host networking device using a
> file descriptor passed over a unix monitor socket.
> 
> The program must first pass the file descriptor using SCM_RIGHTS
> ancillary data with the getfd monitor command. It then may do
> "host_net_add tap fd=name" to use the named file descriptor.

Again begging for this to be pulled into stable-0.11 :-)

These exact patches were posted in time for the branch but got dropped
mainly because of confusion over whether all the concerns had been
addressed

It's a nice feature for a relatively small set of changes, and without
them libvirt can't do NIC hotplug

Cheers,
Mark.

[Qemu-devel] [PATCH] Add SCM_RIGHTS support to unix socket character devices

[Mark McLoughlin]

If a file descriptor is passed via a message with SCM_RIGHTS ancillary
data on a unix socket, store the file descriptor for use in the
chr_read() handler. Close the file descriptor if it was not used.

The qemu_chr_get_msgfd() API provides access to the passed descriptor.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 qemu-char.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-char.h |    2 ++
 2 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index e0d7220..f06a614 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -168,6 +168,11 @@ void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len)
     s->chr_read(s->handler_opaque, buf, len);
 }
 
+int qemu_chr_get_msgfd(CharDriverState *s)
+{
+    return s->get_msgfd ? s->get_msgfd(s) : -1;
+}
+
 void qemu_chr_accept_input(CharDriverState *s)
 {
     if (s->chr_accept_input)
@@ -1832,6 +1837,7 @@ typedef struct {
     int do_telnetopt;
     int do_nodelay;
     int is_unix;
+    int msgfd;
 } TCPCharDriver;
 
 static void tcp_chr_accept(void *opaque);
@@ -1907,12 +1913,46 @@ static void tcp_chr_process_IAC_bytes(CharDriverState *chr,
     *size = j;
 }
 
+static int tcp_get_msgfd(CharDriverState *chr)
+{
+    TCPCharDriver *s = chr->opaque;
+
+    return s->msgfd;
+}
+
+static void unix_process_msgfd(CharDriverState *chr, struct msghdr *msg)
+{
+    TCPCharDriver *s = chr->opaque;
+    struct cmsghdr *cmsg;
+
+    for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
+        int fd;
+
+        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
+            cmsg->cmsg_level != SOL_SOCKET ||
+            cmsg->cmsg_type != SCM_RIGHTS)
+            continue;
+
+        fd = *((int *)CMSG_DATA(cmsg));
+        if (fd < 0)
+            continue;
+
+        if (s->msgfd != -1)
+            close(s->msgfd);
+        s->msgfd = fd;
+    }
+}
+
 static void tcp_chr_read(void *opaque)
 {
     CharDriverState *chr = opaque;
     TCPCharDriver *s = chr->opaque;
     struct msghdr msg = { 0, };
     struct iovec iov[1];
+    union {
+        struct cmsghdr cmsg;
+        char control[CMSG_SPACE(sizeof(int))];
+    } msg_control;
     uint8_t buf[1024];
     int len, size;
 
@@ -1928,6 +1968,8 @@ static void tcp_chr_read(void *opaque)
 
     msg.msg_iov = iov;
     msg.msg_iovlen = 1;
+    msg.msg_control = &msg_control;
+    msg.msg_controllen = sizeof(msg_control);
 
     size = recvmsg(s->fd, &msg, 0);
     if (size == 0) {
@@ -1940,10 +1982,16 @@ static void tcp_chr_read(void *opaque)
         closesocket(s->fd);
         s->fd = -1;
     } else if (size > 0) {
+        if (s->is_unix)
+            unix_process_msgfd(chr, &msg);
         if (s->do_telnetopt)
             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;
+        }
     }
 }
 
@@ -2105,12 +2153,14 @@ static CharDriverState *qemu_chr_open_tcp(const char *host_str,
     s->connected = 0;
     s->fd = -1;
     s->listen_fd = -1;
+    s->msgfd = -1;
     s->is_unix = is_unix;
     s->do_nodelay = do_nodelay && !is_unix;
 
     chr->opaque = s;
     chr->chr_write = tcp_chr_write;
     chr->chr_close = tcp_chr_close;
+    chr->get_msgfd = tcp_get_msgfd;
 
     if (is_listen) {
         s->listen_fd = fd;
diff --git a/qemu-char.h b/qemu-char.h
index e1aa8db..77d4eda 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -51,6 +51,7 @@ struct CharDriverState {
     int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, int len);
     void (*chr_update_read_handler)(struct CharDriverState *s);
     int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg);
+    int (*get_msgfd)(struct CharDriverState *s);
     IOEventHandler *chr_event;
     IOCanRWHandler *chr_can_read;
     IOReadHandler *chr_read;
@@ -81,6 +82,7 @@ void qemu_chr_reset(CharDriverState *s);
 void qemu_chr_initial_reset(void);
 int qemu_chr_can_read(CharDriverState *s);
 void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len);
+int qemu_chr_get_msgfd(CharDriverState *s);
 void qemu_chr_accept_input(CharDriverState *s);
 void qemu_chr_info(Monitor *mon);
 
-- 
1.6.2.5

[Qemu-devel] [PATCH] Add getfd and closefd monitor commands

[Mark McLoughlin]

Add monitor commands to support passing file descriptors via
SCM_RIGHTS.

getfd assigns the passed file descriptor a name for use with other
monitor commands.

closefd allows passed file descriptors to be closed. If a monitor
command actually uses a named file descriptor, closefd will not be
required.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 monitor.c       |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-monitor.hx |   18 ++++++++++++++
 2 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index e38c86e..817e4b7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -70,6 +70,14 @@ typedef struct mon_cmd_t {
     const char *help;
 } mon_cmd_t;
 
+/* file descriptors passed via SCM_RIGHTS */
+typedef struct mon_fd_t mon_fd_t;
+struct mon_fd_t {
+    char *name;
+    int fd;
+    LIST_ENTRY(mon_fd_t) next;
+};
+
 struct Monitor {
     CharDriverState *chr;
     int flags;
@@ -80,6 +88,7 @@ struct Monitor {
     CPUState *mon_cpu;
     BlockDriverCompletionFunc *password_completion_cb;
     void *password_opaque;
+    LIST_HEAD(,mon_fd_t) fds;
     LIST_ENTRY(Monitor) entry;
 };
 
@@ -1705,6 +1714,66 @@ static void do_inject_mce(Monitor *mon,
 }
 #endif
 
+static void do_getfd(Monitor *mon, const char *fdname)
+{
+    mon_fd_t *monfd;
+    int fd;
+
+    fd = qemu_chr_get_msgfd(mon->chr);
+    if (fd == -1) {
+        monitor_printf(mon, "getfd: no file descriptor supplied via SCM_RIGHTS\n");
+        return;
+    }
+
+    if (qemu_isdigit(fdname[0])) {
+        monitor_printf(mon, "getfd: monitor names may not begin with a number\n");
+        return;
+    }
+
+    fd = dup(fd);
+    if (fd == -1) {
+        monitor_printf(mon, "Failed to dup() file descriptor: %s\n",
+                       strerror(errno));
+        return;
+    }
+
+    LIST_FOREACH(monfd, &mon->fds, next) {
+        if (strcmp(monfd->name, fdname) != 0) {
+            continue;
+        }
+
+        close(monfd->fd);
+        monfd->fd = fd;
+        return;
+    }
+
+    monfd = qemu_mallocz(sizeof(mon_fd_t));
+    monfd->name = qemu_strdup(fdname);
+    monfd->fd = fd;
+
+    LIST_INSERT_HEAD(&mon->fds, monfd, next);
+}
+
+static void do_closefd(Monitor *mon, const char *fdname)
+{
+    mon_fd_t *monfd;
+
+    LIST_FOREACH(monfd, &mon->fds, next) {
+        if (strcmp(monfd->name, fdname) != 0) {
+            continue;
+        }
+
+        LIST_REMOVE(monfd, next);
+        close(monfd->fd);
+        qemu_free(monfd->name);
+        qemu_free(monfd);
+        return;
+    }
+
+    monitor_printf(mon, "Failed to find file descriptor named %s\n",
+                   fdname);
+}
+
 static const mon_cmd_t mon_cmds[] = {
 #include "qemu-monitor.h"
     { NULL, NULL, },
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 70e2475..11bdb2c 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -628,6 +628,24 @@ STEXI
 Inject an MCE on the given CPU (x86 only).
 ETEXI
 
+    { "getfd", "s", do_getfd, "getfd name",
+      "receive a file descriptor via SCM rights and assign it a name" },
+STEXI
+@item getfd @var{fdname}
+If a file descriptor is passed alongside this command using the SCM_RIGHTS
+mechanism on unix sockets, it is stored using the name @var{fdname} for
+later use by other monitor commands.
+ETEXI
+
+    { "closefd", "s", do_closefd, "closefd name",
+      "close a file descriptor previously passed via SCM rights" },
+STEXI
+@item closefd @var{fdname}
+Close the file descriptor previously assigned to @var{fdname} using the
+@code{getfd} command. This is only needed if the file descriptor was never
+used by another monitor command.
+ETEXI
+
 STEXI
 @end table
 ETEXI
-- 
1.6.2.5

[Qemu-devel] [PATCH] Add monitor_get_fd() command for fetching named fds

[Mark McLoughlin]

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 monitor.c |   24 ++++++++++++++++++++++++
 monitor.h |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 817e4b7..6ad2e14 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1774,6 +1774,30 @@ static void do_closefd(Monitor *mon, const char *fdname)
                    fdname);
 }
 
+int monitor_get_fd(Monitor *mon, const char *fdname)
+{
+    mon_fd_t *monfd;
+
+    LIST_FOREACH(monfd, &mon->fds, next) {
+        int fd;
+
+        if (strcmp(monfd->name, fdname) != 0) {
+            continue;
+        }
+
+        fd = monfd->fd;
+
+        /* caller takes ownership of fd */
+        LIST_REMOVE(monfd, next);
+        qemu_free(monfd->name);
+        qemu_free(monfd);
+
+        return fd;
+    }
+
+    return -1;
+}
+
 static const mon_cmd_t mon_cmds[] = {
 #include "qemu-monitor.h"
     { NULL, NULL, },
diff --git a/monitor.h b/monitor.h
index 13e8cc7..f6a43c0 100644
--- a/monitor.h
+++ b/monitor.h
@@ -20,6 +20,8 @@ void monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
                                  BlockDriverCompletionFunc *completion_cb,
                                  void *opaque);
 
+int monitor_get_fd(Monitor *mon, const char *fdname);
+
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap);
 void monitor_printf(Monitor *mon, const char *fmt, ...)
     __attribute__ ((__format__ (__printf__, 2, 3)));
-- 
1.6.2.5

[Qemu-devel] [PATCH] Add support for fd=name to tap and socket networking

[Mark McLoughlin]

This allows a program to initialize a host networking device using a
file descriptor passed over a unix monitor socket.

The program must first pass the file descriptor using SCM_RIGHTS
ancillary data with the getfd monitor command. It then may do
"host_net_add tap fd=name" to use the named file descriptor.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 net.c |   42 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/net.c b/net.c
index 90cf912..1838692 100644
--- a/net.c
+++ b/net.c
@@ -2410,6 +2410,23 @@ void qemu_check_nic_model_list(NICInfo *nd, const char * const *models,
     exit(exit_status);
 }
 
+static int net_handle_fd_param(Monitor *mon, const char *param)
+{
+    if (!qemu_isdigit(param[0])) {
+        int fd;
+
+        fd = monitor_get_fd(mon, param);
+        if (fd == -1) {
+            config_error(mon, "No file descriptor named %s found", param);
+            return -1;
+        }
+
+        return fd;
+    } else {
+        return strtol(param, NULL, 0);
+    }
+}
+
 int net_client_init(Monitor *mon, const char *device, const char *p)
 {
     char buf[1024];
@@ -2650,14 +2667,20 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
             static const char * const fd_params[] = {
                 "vlan", "name", "fd", "sndbuf", NULL
             };
+            ret = -1;
             if (check_params(chkbuf, sizeof(chkbuf), fd_params, p) < 0) {
                 config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
-                ret = -1;
                 goto out;
             }
-            fd = strtol(buf, NULL, 0);
+            fd = net_handle_fd_param(mon, buf);
+            if (fd == -1) {
+                goto out;
+            }
             fcntl(fd, F_SETFL, O_NONBLOCK);
             s = net_tap_fd_init(vlan, device, name, fd);
+            if (!s) {
+                close(fd);
+            }
         } else {
             static const char * const tap_params[] = {
                 "vlan", "name", "ifname", "script", "downscript", "sndbuf", NULL
@@ -2697,15 +2720,20 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
                 "vlan", "name", "fd", NULL
             };
             int fd;
+            ret = -1;
             if (check_params(chkbuf, sizeof(chkbuf), fd_params, p) < 0) {
                 config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
-                ret = -1;
                 goto out;
             }
-            fd = strtol(buf, NULL, 0);
-            ret = -1;
-            if (net_socket_fd_init(vlan, device, name, fd, 1))
-                ret = 0;
+            fd = net_handle_fd_param(mon, buf);
+            if (fd == -1) {
+                goto out;
+            }
+            if (!net_socket_fd_init(vlan, device, name, fd, 1)) {
+                close(fd);
+                goto out;
+            }
+            ret = 0;
         } else if (get_param_value(buf, sizeof(buf), "listen", p) > 0) {
             static const char * const listen_params[] = {
                 "vlan", "name", "listen", NULL
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Anthony Liguori]

Mark McLoughlin wrote:
> My take on it is that we're all agreed that it's never valid with the
> current monitor protocol for a client to queue up multiple fds. It's a
> synchronous protocol, you can't send multiple commands at once.
>   

I think there's a subtle point that was lost in the thread.  While we'll 
only need to queue one fd per MonitorState, we'll still need to deal 
with multiple fds per CharDriverState.

I think the way this would most logically work would be a push interface 
verses a pull interface so that there wasn't double queuing.  That is, 
whenever the CharDriverState gets an fd, it calls a function registered 
by the consumer of the CharDriverState.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 3/5] Add getfd and closefd monitor commands

[Mark McLoughlin]

On Tue, 2009-07-21 at 21:20 -0500, Anthony Liguori wrote:
> Mark McLoughlin wrote:
> > My take on it is that we're all agreed that it's never valid with the
> > current monitor protocol for a client to queue up multiple fds. It's a
> > synchronous protocol, you can't send multiple commands at once.
> >   
> 
> I think there's a subtle point that was lost in the thread.  While we'll 
> only need to queue one fd per MonitorState, we'll still need to deal 
> with multiple fds per CharDriverState.

Why? For a theoretical use case, or for the current monitor interface?

If the latter, then I don't follow.

The flow is as follows:

  client                     server
  ------                     ------
  send request ->
  wait for response          
                             read request
                             process msgfd, allowing only a single fd
                             monitor handles request
                          <- send reply
  handle response            close msgfd if unused
  send next request ->

If the client sends multiple requests at once, it is broken - it must
wait for a "(qemu) " reply before sending another request.

If the client sends multiple fds in a single request, it is broken - no
monitor commands support multiple fds.

If the client is broken and sends multiple fds, we only read the first
one and the kernel frees the others.

If the client sends an fd along with a request which does not require an
fd, we just close that fd after we've processed the request.

If we get a new protocol which allows multiple requests per message or
multiple fds per request, we can easily add a queue then.

Cheers,
Mark.

[Qemu-devel] Re: [PATCH 3/5] Add getfd and closefd monitor commands

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1896 bytes --]

Mark McLoughlin wrote:
> On Tue, 2009-07-21 at 21:20 -0500, Anthony Liguori wrote:
>> Mark McLoughlin wrote:
>>> My take on it is that we're all agreed that it's never valid with the
>>> current monitor protocol for a client to queue up multiple fds. It's a
>>> synchronous protocol, you can't send multiple commands at once.
>>>   
>> I think there's a subtle point that was lost in the thread.  While we'll 
>> only need to queue one fd per MonitorState, we'll still need to deal 
>> with multiple fds per CharDriverState.
> 
> Why? For a theoretical use case, or for the current monitor interface?
> 

There can be multiple monitors attached at the same time, already today.

> If the latter, then I don't follow.
> 
> The flow is as follows:
> 
>   client                     server
>   ------                     ------
>   send request ->
>   wait for response          
>                              read request
>                              process msgfd, allowing only a single fd
>                              monitor handles request
>                           <- send reply
>   handle response            close msgfd if unused
>   send next request ->
> 
> If the client sends multiple requests at once, it is broken - it must
> wait for a "(qemu) " reply before sending another request.
> 
> If the client sends multiple fds in a single request, it is broken - no
> monitor commands support multiple fds.
> 
> If the client is broken and sends multiple fds, we only read the first
> one and the kernel frees the others.
> 
> If the client sends an fd along with a request which does not require an
> fd, we just close that fd after we've processed the request.
> 
> If we get a new protocol which allows multiple requests per message or
> multiple fds per request, we can easily add a queue then.
> 
> Cheers,
> Mark.
> 

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 3/5] Add getfd and closefd monitor commands

[Mark McLoughlin]

On Thu, 2009-07-23 at 09:00 +0200, Jan Kiszka wrote:
> Mark McLoughlin wrote:
> > On Tue, 2009-07-21 at 21:20 -0500, Anthony Liguori wrote:
> >> Mark McLoughlin wrote:
> >>> My take on it is that we're all agreed that it's never valid with the
> >>> current monitor protocol for a client to queue up multiple fds. It's a
> >>> synchronous protocol, you can't send multiple commands at once.
> >>>   
> >> I think there's a subtle point that was lost in the thread.  While we'll 
> >> only need to queue one fd per MonitorState, we'll still need to deal 
> >> with multiple fds per CharDriverState.
> > 
> > Why? For a theoretical use case, or for the current monitor interface?
> > 
> 
> There can be multiple monitors attached at the same time, already today.

But each monitor has its own socket, CharDriverState and Monitor ...
multiple monitors is irrelevant to the need for an fd queue on
CharDriverState.

Cheers,
Mark.

Re: [Qemu-devel] [PATCH 2/5] Add SCM_RIGHTS support to unix socket character devices

[Avi Kivity]

On 07/08/2009 05:57 PM, Mark McLoughlin wrote:
> If a file descriptor is passed via a message with SCM_RIGHTS ancillary
> data on a unix socket, store the file descriptor for use in the
> chr_read() handler. Close the file descriptor if it was not used.
>
> The qemu_chr_get_msgfd() API provides access to the passed descriptor.
>
> Signed-off-by: Mark McLoughlin<markmc@redhat.com>
> ---
>   qemu-char.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   qemu-char.h |    2 ++
>   2 files changed, 52 insertions(+), 0 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index e0d7220..f06a614 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -168,6 +168,11 @@ void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len)
>       s->chr_read(s->handler_opaque, buf, len);
>   }
>
> +int qemu_chr_get_msgfd(CharDriverState *s)
> +{
> +    return s->get_msgfd ? s->get_msgfd(s) : -1;
> +}
> +
>   void qemu_chr_accept_input(CharDriverState *s)
>   {
>       if (s->chr_accept_input)
> @@ -1832,6 +1837,7 @@ typedef struct {
>       int do_telnetopt;
>       int do_nodelay;
>       int is_unix;
> +    int msgfd;
>   } TCPCharDriver;
>    

SCM_RIGHTS messages can contain multiple fds, and multiple messages can 
arrive.  I think you need to queue fds here in case the client sends two 
getfd commands back-to-back and does buffering.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/5] Add SCM_RIGHTS support to unix socket character devices

[Mark McLoughlin]

On Wed, 2009-07-08 at 18:25 +0300, Avi Kivity wrote:
> On 07/08/2009 05:57 PM, Mark McLoughlin wrote:
> > If a file descriptor is passed via a message with SCM_RIGHTS ancillary
> > data on a unix socket, store the file descriptor for use in the
> > chr_read() handler. Close the file descriptor if it was not used.
> >
> > The qemu_chr_get_msgfd() API provides access to the passed descriptor.
> >
> > Signed-off-by: Mark McLoughlin<markmc@redhat.com>
> > ---
> >   qemu-char.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   qemu-char.h |    2 ++
> >   2 files changed, 52 insertions(+), 0 deletions(-)
> >
> > diff --git a/qemu-char.c b/qemu-char.c
> > index e0d7220..f06a614 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -168,6 +168,11 @@ void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len)
> >       s->chr_read(s->handler_opaque, buf, len);
> >   }
> >
> > +int qemu_chr_get_msgfd(CharDriverState *s)
> > +{
> > +    return s->get_msgfd ? s->get_msgfd(s) : -1;
> > +}
> > +
> >   void qemu_chr_accept_input(CharDriverState *s)
> >   {
> >       if (s->chr_accept_input)
> > @@ -1832,6 +1837,7 @@ typedef struct {
> >       int do_telnetopt;
> >       int do_nodelay;
> >       int is_unix;
> > +    int msgfd;
> >   } TCPCharDriver;
> >    
> 
> SCM_RIGHTS messages can contain multiple fds,

It only makes sense to have one per getfd command and ...

> and multiple messages can arrive. I think you need to queue fds here
> in case the client sends two getfd commands back-to-back and does
> buffering.

it doesn't look to me like the current monitor code can handle multiple
commands in the one message.

Cheers,
Mark.

Re: [Qemu-devel] [PATCH 2/5] Add SCM_RIGHTS support to unix socket character devices

[Avi Kivity]

On 07/08/2009 07:04 PM, Mark McLoughlin wrote:
>
>> and multiple messages can arrive. I think you need to queue fds here
>> in case the client sends two getfd commands back-to-back and does
>> buffering.
>>      
>
> it doesn't look to me like the current monitor code can handle multiple
> commands in the one message.
>    

How does qemu have any control over the division of messages and 
packets?  I can send commands byte by byte or queue them up and send a 
gigabyte's worth in one go (when we have -monitor ib:blah).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/5] Add SCM_RIGHTS support to unix socket character devices

[Anthony Liguori]

Mark McLoughlin wrote:
> It only makes sense to have one per getfd command and ...
>   

It would be silly to pass many fds.  fds are scarce resources and you 
don't want to get into a position where you're queuing fds.  If a 
management tool wants to send an fd, it should issue the monitor command 
that needs the fd along with the fd via SCM_RIGHTS, and then wait for 
that command to complete before issuing another command that needs an fd.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/5] Add SCM_RIGHTS support to unix socket character devices

[Avi Kivity]

On 07/08/2009 09:11 PM, Anthony Liguori wrote:
> Mark McLoughlin wrote:
>> It only makes sense to have one per getfd command and ...
>
> It would be silly to pass many fds.  fds are scarce resources and you 
> don't want to get into a position where you're queuing fds. 

If you issue N getfd commands, you've queued N fds.

> If a management tool wants to send an fd, it should issue the monitor 
> command that needs the fd along with the fd via SCM_RIGHTS, and then 
> wait for that command to complete before issuing another command that 
> needs an fd.
>

Why impose unnecessary restrictions?  I agree that the guest shouldn't 
use qemu as an fd store.  But putting such limits in the code is laying 
a trap for the client, which will show up in rare cases, for example two 
client threads issuing unrelated getfd commands with the client author 
not knowing it should serialize them.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 2/5] Add SCM_RIGHTS support to unix socket character devices

[Jamie Lokier]

Avi Kivity wrote:
> Why impose unnecessary restrictions?  I agree that the guest shouldn't 
> use qemu as an fd store.  But putting such limits in the code is laying 
> a trap for the client, which will show up in rare cases, for example two 
> client threads issuing unrelated getfd commands with the client author 
> not knowing it should serialize them.

If there is an fd queue, then it clearly must be limited on the QEMU
side to avoid becoming an fd store.

A client issuing unrelated commands must be aware of that limitation.
The queue length could be 1, or it could be 10 per monitor to allow
some command overlap, but the client will need to be aware of it and
limit overlap.

That means the limit must be documented in any (future :-) monitor
protocol documentation.

-- Jamie