Re: [Qemu-devel] [PATCH v4 2/4] char: convert from GIOChannel to QIOChannel

[Laurent Vivier]

Hi,

testing something else (migration...) I've discovered (by bisecting)
that this patch can allow to lock the machine. I'm using the pseries
machine, but I think it should happen with PC too.

I start a machine with:

...
-device virtio-serial-pci,id=serial0 \
-chardev socket,id=channel0,path=/tmp/serial_socket,server,nowait \
-device virtserialport,bus=serial0.0,nr=1,chardev=channel0

and I open the unix socket /tmp/serial_socket without reading it:

$ python
import socket
sock = socket.socket(socket.AF_UNIX)
sock.connect("/tmp/serial_socket_1")

Then in the guest:

cat /dev/zero > /dev/vport1p1

-> at this point, the machine hangs until we read data in unix socket
(we can't interact with monitor, we can't ping the machine...)

Laurent

On 19/01/2016 12:14, Daniel P. Berrange wrote:
> In preparation for introducing TLS support to the TCP chardev
> backend, convert existing chardev code from using GIOChannel
> to QIOChannel. This simplifies the chardev code by removing
> most of the OS platform conditional code for dealing with
> file descriptor passing.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  qemu-char.c| 648 
> ++---
>  tests/Makefile |   2 +-
>  2 files changed, 254 insertions(+), 396 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 8e96f90..8e9156a 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -33,6 +33,8 @@
>  #include "qapi/qmp-output-visitor.h"
>  #include "qapi-visit.h"
>  #include "qemu/base64.h"
> +#include "io/channel-socket.h"
> +#include "io/channel-file.h"
>  
>  #include 
>  #include 
> @@ -766,7 +768,7 @@ typedef struct IOWatchPoll
>  {
>  GSource parent;
>  
> -GIOChannel *channel;
> +QIOChannel *ioc;
>  GSource *src;
>  
>  IOCanReadHandler *fd_can_read;
> @@ -789,8 +791,8 @@ static gboolean io_watch_poll_prepare(GSource *source, 
> gint *timeout_)
>  }
>  
>  if (now_active) {
> -iwp->src = g_io_create_watch(iwp->channel,
> - G_IO_IN | G_IO_ERR | G_IO_HUP | 
> G_IO_NVAL);
> +iwp->src = qio_channel_create_watch(
> +iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
>  g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
>  g_source_attach(iwp->src, NULL);
>  } else {
> @@ -836,9 +838,9 @@ static GSourceFuncs io_watch_poll_funcs = {
>  };
>  
>  /* Can only be used for read */
> -static guint io_add_watch_poll(GIOChannel *channel,
> +static guint io_add_watch_poll(QIOChannel *ioc,
> IOCanReadHandler *fd_can_read,
> -   GIOFunc fd_read,
> +   QIOChannelFunc fd_read,
> gpointer user_data)
>  {
>  IOWatchPoll *iwp;
> @@ -847,7 +849,7 @@ static guint io_add_watch_poll(GIOChannel *channel,
>  iwp = (IOWatchPoll *) g_source_new(_watch_poll_funcs, 
> sizeof(IOWatchPoll));
>  iwp->fd_can_read = fd_can_read;
>  iwp->opaque = user_data;
> -iwp->channel = channel;
> +iwp->ioc = ioc;
>  iwp->fd_read = (GSourceFunc) fd_read;
>  iwp->src = NULL;
>  
> @@ -883,79 +885,50 @@ static void remove_fd_in_watch(CharDriverState *chr)
>  }
>  }
>  
> -#ifndef _WIN32
> -static GIOChannel *io_channel_from_fd(int fd)
> -{
> -GIOChannel *chan;
> -
> -if (fd == -1) {
> -return NULL;
> -}
>  
> -chan = g_io_channel_unix_new(fd);
> -
> -g_io_channel_set_encoding(chan, NULL, NULL);
> -g_io_channel_set_buffered(chan, FALSE);
> -
> -return chan;
> -}
> -#endif
> -
> -static GIOChannel *io_channel_from_socket(int fd)
> +static int io_channel_send_full(QIOChannel *ioc,
> +const void *buf, size_t len,
> +int *fds, size_t nfds)
>  {
> -GIOChannel *chan;
> +size_t offset = 0;
>  
> -if (fd == -1) {
> -return NULL;
> -}
> +while (offset < len) {
> +ssize_t ret = 0;
> +struct iovec iov = { .iov_base = (char *)buf + offset,
> + .iov_len = len - offset };
> +
> +ret = qio_channel_writev_full(
> +ioc, , 1,
> +fds, nfds, NULL);
> +if (ret == QIO_CHANNEL_ERR_BLOCK) {
> +errno = EAGAIN;
> +return -1;
> +} else if (ret < 0) {
> +if (offset) {
> +return offset;
> +}
>  
> -#ifdef _WIN32
> -chan = g_io_channel_win32_new_socket(fd);
> -#else
> -chan = g_io_channel_unix_new(fd);
> -#endif
> +errno = EINVAL;
> +return -1;
> +}
>  
> -g_io_channel_set_encoding(chan, NULL, NULL);
> -g_io_channel_set_buffered(chan, FALSE);
> +offset += ret;
> +}
>  
> -return chan;
> +return offset;
>  }
>  
> -static int io_channel_send(GIOChannel *fd, const void *buf, size_t len)
> -{
> -

Re: [Qemu-devel] [PATCH v4 2/4] char: convert from GIOChannel to QIOChannel

[Laurent Vivier]

On 18/03/2016 17:56, Daniel P. Berrange wrote:
> On Fri, Mar 18, 2016 at 05:43:42PM +0100, Laurent Vivier wrote:
>> Hi,
>>
>> testing something else (migration...) I've discovered (by bisecting)
>> that this patch can allow to lock the machine. I'm using the pseries
>> machine, but I think it should happen with PC too.
>>
>> I start a machine with:
>>
>>  ...
>>  -device virtio-serial-pci,id=serial0 \
>>  -chardev socket,id=channel0,path=/tmp/serial_socket,server,nowait \
>>  -device virtserialport,bus=serial0.0,nr=1,chardev=channel0
>>
>> and I open the unix socket /tmp/serial_socket without reading it:
>>
>> $ python
>> import socket
>> sock = socket.socket(socket.AF_UNIX)
>> sock.connect("/tmp/serial_socket_1")
>>
>> Then in the guest:
>>
>> cat /dev/zero > /dev/vport1p1
>>
>> -> at this point, the machine hangs until we read data in unix socket
>> (we can't interact with monitor, we can't ping the machine...)
> 
> Pretty sure that'll be the same issue Andrew reported here
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg02843.html
> 
> can you see if his suggested addition works for you too

Yes, it works :)

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v4 2/4] char: convert from GIOChannel to QIOChannel

[Daniel P. Berrange]

On Fri, Mar 18, 2016 at 05:43:42PM +0100, Laurent Vivier wrote:
> Hi,
> 
> testing something else (migration...) I've discovered (by bisecting)
> that this patch can allow to lock the machine. I'm using the pseries
> machine, but I think it should happen with PC too.
> 
> I start a machine with:
> 
>   ...
>   -device virtio-serial-pci,id=serial0 \
>   -chardev socket,id=channel0,path=/tmp/serial_socket,server,nowait \
>   -device virtserialport,bus=serial0.0,nr=1,chardev=channel0
> 
> and I open the unix socket /tmp/serial_socket without reading it:
> 
> $ python
> import socket
> sock = socket.socket(socket.AF_UNIX)
> sock.connect("/tmp/serial_socket_1")
> 
> Then in the guest:
> 
> cat /dev/zero > /dev/vport1p1
> 
> -> at this point, the machine hangs until we read data in unix socket
> (we can't interact with monitor, we can't ping the machine...)

Pretty sure that'll be the same issue Andrew reported here

https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg02843.html

can you see if his suggested addition works for you too

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

[Qemu-devel] [PATCH v4 2/4] char: convert from GIOChannel to QIOChannel

[Daniel P. Berrange]

In preparation for introducing TLS support to the TCP chardev
backend, convert existing chardev code from using GIOChannel
to QIOChannel. This simplifies the chardev code by removing
most of the OS platform conditional code for dealing with
file descriptor passing.

Signed-off-by: Daniel P. Berrange 
---
 qemu-char.c| 648 ++---
 tests/Makefile |   2 +-
 2 files changed, 254 insertions(+), 396 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 8e96f90..8e9156a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -33,6 +33,8 @@
 #include "qapi/qmp-output-visitor.h"
 #include "qapi-visit.h"
 #include "qemu/base64.h"
+#include "io/channel-socket.h"
+#include "io/channel-file.h"
 
 #include 
 #include 
@@ -766,7 +768,7 @@ typedef struct IOWatchPoll
 {
 GSource parent;
 
-GIOChannel *channel;
+QIOChannel *ioc;
 GSource *src;
 
 IOCanReadHandler *fd_can_read;
@@ -789,8 +791,8 @@ static gboolean io_watch_poll_prepare(GSource *source, gint 
*timeout_)
 }
 
 if (now_active) {
-iwp->src = g_io_create_watch(iwp->channel,
- G_IO_IN | G_IO_ERR | G_IO_HUP | 
G_IO_NVAL);
+iwp->src = qio_channel_create_watch(
+iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
 g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
 g_source_attach(iwp->src, NULL);
 } else {
@@ -836,9 +838,9 @@ static GSourceFuncs io_watch_poll_funcs = {
 };
 
 /* Can only be used for read */
-static guint io_add_watch_poll(GIOChannel *channel,
+static guint io_add_watch_poll(QIOChannel *ioc,
IOCanReadHandler *fd_can_read,
-   GIOFunc fd_read,
+   QIOChannelFunc fd_read,
gpointer user_data)
 {
 IOWatchPoll *iwp;
@@ -847,7 +849,7 @@ static guint io_add_watch_poll(GIOChannel *channel,
 iwp = (IOWatchPoll *) g_source_new(_watch_poll_funcs, 
sizeof(IOWatchPoll));
 iwp->fd_can_read = fd_can_read;
 iwp->opaque = user_data;
-iwp->channel = channel;
+iwp->ioc = ioc;
 iwp->fd_read = (GSourceFunc) fd_read;
 iwp->src = NULL;
 
@@ -883,79 +885,50 @@ static void remove_fd_in_watch(CharDriverState *chr)
 }
 }
 
-#ifndef _WIN32
-static GIOChannel *io_channel_from_fd(int fd)
-{
-GIOChannel *chan;
-
-if (fd == -1) {
-return NULL;
-}
 
-chan = g_io_channel_unix_new(fd);
-
-g_io_channel_set_encoding(chan, NULL, NULL);
-g_io_channel_set_buffered(chan, FALSE);
-
-return chan;
-}
-#endif
-
-static GIOChannel *io_channel_from_socket(int fd)
+static int io_channel_send_full(QIOChannel *ioc,
+const void *buf, size_t len,
+int *fds, size_t nfds)
 {
-GIOChannel *chan;
+size_t offset = 0;
 
-if (fd == -1) {
-return NULL;
-}
+while (offset < len) {
+ssize_t ret = 0;
+struct iovec iov = { .iov_base = (char *)buf + offset,
+ .iov_len = len - offset };
+
+ret = qio_channel_writev_full(
+ioc, , 1,
+fds, nfds, NULL);
+if (ret == QIO_CHANNEL_ERR_BLOCK) {
+errno = EAGAIN;
+return -1;
+} else if (ret < 0) {
+if (offset) {
+return offset;
+}
 
-#ifdef _WIN32
-chan = g_io_channel_win32_new_socket(fd);
-#else
-chan = g_io_channel_unix_new(fd);
-#endif
+errno = EINVAL;
+return -1;
+}
 
-g_io_channel_set_encoding(chan, NULL, NULL);
-g_io_channel_set_buffered(chan, FALSE);
+offset += ret;
+}
 
-return chan;
+return offset;
 }
 
-static int io_channel_send(GIOChannel *fd, const void *buf, size_t len)
-{
-size_t offset = 0;
-GIOStatus status = G_IO_STATUS_NORMAL;
-
-while (offset < len && status == G_IO_STATUS_NORMAL) {
-gsize bytes_written = 0;
 
-status = g_io_channel_write_chars(fd, buf + offset, len - offset,
-  _written, NULL);
-offset += bytes_written;
-}
-
-if (offset > 0) {
-return offset;
-}
-switch (status) {
-case G_IO_STATUS_NORMAL:
-g_assert(len == 0);
-return 0;
-case G_IO_STATUS_AGAIN:
-errno = EAGAIN;
-return -1;
-default:
-break;
-}
-errno = EINVAL;
-return -1;
+static int io_channel_send(QIOChannel *ioc, const void *buf, size_t len)
+{
+return io_channel_send_full(ioc, buf, len, NULL, 0);
 }
 
 #ifndef _WIN32
 
 typedef struct FDCharDriver {
 CharDriverState *chr;
-GIOChannel *fd_in, *fd_out;
+QIOChannel *ioc_in, *ioc_out;
 int max_size;
 } FDCharDriver;
 
@@ -964,17 +937,16 @@ static int fd_chr_write(CharDriverState *chr, const 
uint8_t *buf, int len)
 {
 FDCharDriver *s = chr->opaque;
 
-