[Qemu-devel] Re: [PATCH v7 4/6] char: Add qemu_chr_write_nb() for nonblocking writes

[Anthony Liguori <anthony@codemonkey.ws>]

On 05/04/2010 04:39 PM, Amit Shah wrote:
> For char devices whose backing files are open in non-blocking mode,
> non-blocking writes can now be made using qemu_chr_write_nb().
>
> For non-blocking chardevs, we can return -EAGAIN to callers of
> qemu_chr_write_nb(). When the backend is ready to accept more data,
> we can let the caller know via a callback.
>
> -EAGAIN is returned only when the backend's file is non-blocking
> and a callback is registered by the caller when invoking
> qemu_chr_add_handlers().
>
> In case a backend doesn't support non-blocking writes,
> qemu_chr_write_nb() invokes qemu_chr_write().
>
> Individual callers can update their code to add a callback handler,
> call qemu_chr_write_nb() instead of qemu_chr_write() if they wish to
> receive -EAGAIN notifications.
>
> No backend currently supports non-blocking writes.
>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
> ---
>   net/socket.c  |    4 ++--
>   qemu-char.c   |   31 ++++++++++++++++++++++++-------
>   qemu-char.h   |    8 ++++++++
>   qemu_socket.h |    3 ++-
>   4 files changed, 36 insertions(+), 10 deletions(-)
>
> diff --git a/net/socket.c b/net/socket.c
> index 1c4e153..8a401e6 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -56,8 +56,8 @@ static ssize_t net_socket_receive(VLANClientState *nc, const uint8_t *buf, size_
>       uint32_t len;
>       len = htonl(size);
>
> -    send_all(s->fd, (const uint8_t *)&len, sizeof(len));
> -    return send_all(s->fd, buf, size);
> +    send_all(s->fd, (const uint8_t *)&len, sizeof(len), false);
> +    return send_all(s->fd, buf, size, false);
>   }
>
>   static ssize_t net_socket_receive_dgram(VLANClientState *nc, const uint8_t *buf, size_t size)
> diff --git a/qemu-char.c b/qemu-char.c
> index decf687..5e4dec3 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -145,6 +145,16 @@ int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len)
>       return s->chr_write(s, buf, len);
>   }
>
> +ssize_t qemu_chr_write_nb(CharDriverState *s, const uint8_t *buf, size_t len)
> +{
> +    if (!s->nonblock) {
> +        /* Fallback to blocking write if no callback registered */
> +        return qemu_chr_write(s, buf, len);
> +    }
> +
> +    return s->chr_write_nb(s, buf, len);
> +}
>    

I really dislike the idea of adding another function for this.  Can you 
explain why you need this functionality for virtio-console and why this 
functionality isn't needed for everything else?

Regards,

Anthony Liguori

>   int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg)
>   {
>       if (!s->chr_ioctl)
> @@ -203,11 +213,15 @@ void qemu_chr_add_handlers(CharDriverState *s,
>       }
>       s->chr_can_read = handlers->fd_can_read;
>       s->chr_read = handlers->fd_read;
> +    s->chr_write_unblocked = handlers->fd_write_unblocked;
>       s->chr_event = handlers->fd_event;
>       s->handler_opaque = opaque;
>       if (s->chr_update_read_handler)
>           s->chr_update_read_handler(s);
>
> +    /* We'll set this at connect-time */
> +    s->nonblock = false;
> +
>       /* We're connecting to an already opened device, so let's make sure we
>          also get the open event */
>       if (s->opened) {
> @@ -499,7 +513,7 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
>
>
>   #ifdef _WIN32
> -int send_all(int fd, const void *buf, int len1)
> +int send_all(int fd, const void *buf, int len1, bool nonblock)
>   {
>       int ret, len;
>
> @@ -526,7 +540,7 @@ int send_all(int fd, const void *buf, int len1)
>
>   #else
>
> -static int unix_write(int fd, const uint8_t *buf, int len1)
> +static int unix_write(int fd, const uint8_t *buf, int len1, bool nonblock)
>   {
>       int ret, len;
>
> @@ -540,6 +554,9 @@ static int unix_write(int fd, const uint8_t *buf, int len1)
>               if (len1 - len) {
>                   return len1 - len;
>               }
> +            if (errno == EAGAIN&&  nonblock) {
> +                return -EAGAIN;
> +            }
>               if (errno != EAGAIN) {
>                   return -1;
>               }
> @@ -553,9 +570,9 @@ static int unix_write(int fd, const uint8_t *buf, int len1)
>       return len1 - len;
>   }
>
> -int send_all(int fd, const void *buf, int len1)
> +int send_all(int fd, const void *buf, int len1, bool nonblock)
>   {
> -    return unix_write(fd, buf, len1);
> +    return unix_write(fd, buf, len1, nonblock);
>   }
>   #endif /* !_WIN32 */
>
> @@ -572,7 +589,7 @@ static int stdio_nb_clients = 0;
>   static int fd_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>   {
>       FDCharDriver *s = chr->opaque;
> -    return send_all(s->fd_out, buf, len);
> +    return send_all(s->fd_out, buf, len, false);
>   }
>
>   static int fd_chr_read_poll(void *opaque)
> @@ -884,7 +901,7 @@ static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>           pty_chr_update_read_handler(chr);
>           return 0;
>       }
> -    return send_all(s->fd, buf, len);
> +    return send_all(s->fd, buf, len, false);
>   }
>
>   static int pty_chr_read_poll(void *opaque)
> @@ -1949,7 +1966,7 @@ static int tcp_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>   {
>       TCPCharDriver *s = chr->opaque;
>       if (s->connected) {
> -        return send_all(s->fd, buf, len);
> +        return send_all(s->fd, buf, len, false);
>       } else {
>           /* XXX: indicate an error ? */
>           return len;
> diff --git a/qemu-char.h b/qemu-char.h
> index eacb3b9..52f9ef1 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -1,6 +1,8 @@
>   #ifndef QEMU_CHAR_H
>   #define QEMU_CHAR_H
>
> +#include<stdbool.h>
> +
>   #include "qemu-common.h"
>   #include "qemu-queue.h"
>   #include "qemu-option.h"
> @@ -53,12 +55,15 @@ typedef void IOEventHandler(void *opaque, int event);
>   struct CharDriverState {
>       void (*init)(struct CharDriverState *s);
>       int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, int len);
> +    ssize_t (*chr_write_nb)(struct CharDriverState *s, const uint8_t *buf,
> +                            size_t 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;
>       IOCanReadHandler *chr_can_read;
>       IOReadHandler *chr_read;
> +    IOHandler *chr_write_unblocked;
>       void *handler_opaque;
>       void (*chr_send_event)(struct CharDriverState *chr, int event);
>       void (*chr_close)(struct CharDriverState *chr);
> @@ -68,6 +73,8 @@ struct CharDriverState {
>       char *label;
>       char *filename;
>       int opened;
> +    bool nonblock;
> +    bool write_blocked;
>       QTAILQ_ENTRY(CharDriverState) next;
>   };
>
> @@ -85,6 +92,7 @@ CharDriverState *qemu_chr_open(const char *label, const char *filename, void (*i
>   void qemu_chr_close(CharDriverState *chr);
>   void qemu_chr_printf(CharDriverState *s, const char *fmt, ...);
>   int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len);
> +ssize_t qemu_chr_write_nb(CharDriverState *s, const uint8_t *buf, size_t len);
>   void qemu_chr_send_event(CharDriverState *s, int event);
>   void qemu_chr_add_handlers(CharDriverState *s, const QemuChrHandlers *handlers,
>                              void *opaque);
> diff --git a/qemu_socket.h b/qemu_socket.h
> index 164ae3e..bdf878b 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -23,6 +23,7 @@ int inet_aton(const char *cp, struct in_addr *ia);
>   #include<arpa/inet.h>
>   #include<netdb.h>
>   #include<sys/un.h>
> +#include<stdbool.h>
>
>   #define socket_error() errno
>   #define closesocket(s) close(s)
> @@ -35,7 +36,7 @@ int inet_aton(const char *cp, struct in_addr *ia);
>   int qemu_socket(int domain, int type, int protocol);
>   int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
>   void socket_set_nonblock(int fd);
> -int send_all(int fd, const void *buf, int len1);
> +int send_all(int fd, const void *buf, int len1, bool nonblock);
>
>   /* New, ipv6-ready socket helper functions, see qemu-sockets.c */
>   int inet_listen_opts(QemuOpts *opts, int port_offset);
>