Re: [Qemu-devel] [PATCH v3 02/13] qemu-nbd: Switch to qemu_set_fd_handler

Re: [Qemu-devel] [PATCH v3 02/13] qemu-nbd: Switch to qemu_set_fd_handler

[Amit Shah]

On (Tue) 19 May 2015 [10:50:59], Fam Zheng wrote:
> Achieved by:
> 
> - Remembering the server fd with a global variable, in order to access
>   it from nbd_client_closed.
> 
> - Checking nbd_can_accept() and updating server_fd handler whenever
>   client connects or disconnects.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

(snip)

> +static void nbd_update_server_fd_handler(int fd)
> +{
> +    if (nbd_can_accept()) {
> +        qemu_set_fd_handler(fd, nbd_accept, NULL, (void *)(uintptr_t)fd);
> +    } else {
> +        qemu_set_fd_handler(fd, NULL, NULL, NULL);

The previous patch just introduces a stub for qemu_set_fd_handler(),
so this functionality will break (during bisect) -- can you modify
patch 1 so that things keep working (even by just copying over
qemu_set_fd_handler2() to qemu_set_fd_handler(), and then re-working
that function later?

Or, it may even be possible to rename the function in patch 1, and
then modify it later.  Depends on the flow of the following patches,
but one of these approaches looks better to me.

		Amit

Re: [Qemu-devel] [PATCH v3 02/13] qemu-nbd: Switch to qemu_set_fd_handler

[Amit Shah]

On (Tue) 19 May 2015 [11:32:56], Amit Shah wrote:
> On (Tue) 19 May 2015 [10:50:59], Fam Zheng wrote:
> > Achieved by:
> > 
> > - Remembering the server fd with a global variable, in order to access
> >   it from nbd_client_closed.
> > 
> > - Checking nbd_can_accept() and updating server_fd handler whenever
> >   client connects or disconnects.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> (snip)
> 
> > +static void nbd_update_server_fd_handler(int fd)
> > +{
> > +    if (nbd_can_accept()) {
> > +        qemu_set_fd_handler(fd, nbd_accept, NULL, (void *)(uintptr_t)fd);
> > +    } else {
> > +        qemu_set_fd_handler(fd, NULL, NULL, NULL);
> 
> The previous patch just introduces a stub for qemu_set_fd_handler(),
> so this functionality will break (during bisect) -- can you modify
> patch 1 so that things keep working (even by just copying over
> qemu_set_fd_handler2() to qemu_set_fd_handler(), and then re-working
> that function later?
> 
> Or, it may even be possible to rename the function in patch 1, and
> then modify it later.  Depends on the flow of the following patches,
> but one of these approaches looks better to me.

Please ignore!  I didn't notice the first patch was to stubs, not
iohandler.

		Amit

[Qemu-devel] [PATCH v3 00/13] main-loop: Get rid of fd_read_poll and qemu_set_fd_handler2

[Fam Zheng]

v3: Replace previous 13 with a simple return type conversion patch.
    Drop RFC.

This carries out the mandate in the comment of qemu_set_fd_handler2 and removes
fd_read_poll from the code base, because it will make the work easier to
convert ppoll to epoll in main loop.  Also, the aio interface doesn't have a
read poll callback, which means this conversion woule be necessary if we want
to move iohandler fds from main loop to AioContext.

There are five users of the read poll callback now: qemu-nbd, l2tpv3, netmap,
socket and tap, which are all covered.

Patch 1 adds a stub for qemu_set_fd_handler which will be referenced in coming
patches.

Patch 2 converts qemu-nbd which compares two global numbers in the fd_read_poll
callback.

Patches 3~6 converts the four net devices, all of which checks
qemu_can_send_packet() in the callback.

Patch 7 and 8 finally removes the function.

The rest patches are cleanup for the unuseful return value of
qemu_set_fd_handler.

Please review!



Fam Zheng (13):
  stubs: Add qemu_set_fd_handler
  qemu-nbd: Switch to qemu_set_fd_handler
  l2tpv3: Drop l2tpv3_can_send
  netmap: Drop netmap_can_send
  net/socket: Drop net_socket_can_send
  tap: Drop tap_can_send
  Change qemu_set_fd_handler2(..., NULL, ...) to qemu_set_fd_handler
  main-loop: Drop qemu_set_fd_handler2
  alsaaudio: Remove unused error handling of qemu_set_fd_handler
  oss: Remove unused error handling of qemu_set_fd_handler
  xen_backend: Remove unused error handling of qemu_set_fd_handler
  event-notifier: Always return 0 for posix implementation
  iohandler: Change return type of qemu_set_fd_handler to "void"

 audio/alsaaudio.c           | 16 ++-----------
 audio/ossaudio.c            | 14 ++++++-----
 blockdev-nbd.c              |  4 ++--
 hw/xen/xen_backend.c        |  4 +---
 include/block/aio.h         |  2 +-
 include/qemu/main-loop.h    | 57 ++++-----------------------------------------
 iohandler.c                 | 21 ++---------------
 main-loop.c                 |  3 +--
 migration/exec.c            |  6 ++---
 migration/fd.c              |  4 ++--
 migration/rdma.c            |  7 +++---
 migration/tcp.c             |  6 ++---
 migration/unix.c            |  6 ++---
 net/l2tpv3.c                | 17 ++++----------
 net/netmap.c                | 20 ++++------------
 net/socket.c                | 37 +++++++++++++++++------------
 net/tap.c                   | 28 +++++++++++-----------
 qemu-nbd.c                  | 21 +++++++++++++----
 stubs/set-fd-handler.c      |  3 +--
 ui/vnc-auth-sasl.c          |  2 +-
 ui/vnc-auth-vencrypt.c      |  2 +-
 ui/vnc-ws.c                 |  6 ++---
 ui/vnc.c                    | 27 ++++++++++-----------
 util/event_notifier-posix.c |  3 ++-
 util/qemu-sockets.c         |  8 +++----
 25 files changed, 120 insertions(+), 204 deletions(-)

-- 
2.4.1

[Qemu-devel] [PATCH v3 01/13] stubs: Add qemu_set_fd_handler

[Fam Zheng]

Some qemu_set_fd_handler2 stub callers will be converted to
call qemu_set_fd_handler, add this stub for them before making the
change.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 stubs/set-fd-handler.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/stubs/set-fd-handler.c b/stubs/set-fd-handler.c
index fc874d3..25cca8c 100644
--- a/stubs/set-fd-handler.c
+++ b/stubs/set-fd-handler.c
@@ -1,6 +1,14 @@
 #include "qemu-common.h"
 #include "qemu/main-loop.h"
 
+int qemu_set_fd_handler(int fd,
+                        IOHandler *fd_read,
+                        IOHandler *fd_write,
+                        void *opaque)
+{
+    abort();
+}
+
 int qemu_set_fd_handler2(int fd,
                          IOCanReadHandler *fd_read_poll,
                          IOHandler *fd_read,
-- 
2.4.1

[Qemu-devel] [PATCH v3 02/13] qemu-nbd: Switch to qemu_set_fd_handler

[Fam Zheng]

Achieved by:

- Remembering the server fd with a global variable, in order to access
  it from nbd_client_closed.

- Checking nbd_can_accept() and updating server_fd handler whenever
  client connects or disconnects.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-nbd.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 7e690ff..5af6d11 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -53,6 +53,7 @@ static int persistent = 0;
 static enum { RUNNING, TERMINATE, TERMINATING, TERMINATED } state;
 static int shared = 1;
 static int nb_fds;
+static int server_fd;
 
 static void usage(const char *name)
 {
@@ -340,7 +341,7 @@ out:
     return (void *) EXIT_FAILURE;
 }
 
-static int nbd_can_accept(void *opaque)
+static int nbd_can_accept(void)
 {
     return nb_fds < shared;
 }
@@ -351,19 +352,21 @@ static void nbd_export_closed(NBDExport *exp)
     state = TERMINATED;
 }
 
+static void nbd_update_server_fd_handler(int fd);
+
 static void nbd_client_closed(NBDClient *client)
 {
     nb_fds--;
     if (nb_fds == 0 && !persistent && state == RUNNING) {
         state = TERMINATE;
     }
+    nbd_update_server_fd_handler(server_fd);
     qemu_notify_event();
     nbd_client_put(client);
 }
 
 static void nbd_accept(void *opaque)
 {
-    int server_fd = (uintptr_t) opaque;
     struct sockaddr_in addr;
     socklen_t addr_len = sizeof(addr);
 
@@ -380,12 +383,22 @@ static void nbd_accept(void *opaque)
 
     if (nbd_client_new(exp, fd, nbd_client_closed)) {
         nb_fds++;
+        nbd_update_server_fd_handler(server_fd);
     } else {
         shutdown(fd, 2);
         close(fd);
     }
 }
 
+static void nbd_update_server_fd_handler(int fd)
+{
+    if (nbd_can_accept()) {
+        qemu_set_fd_handler(fd, nbd_accept, NULL, (void *)(uintptr_t)fd);
+    } else {
+        qemu_set_fd_handler(fd, NULL, NULL, NULL);
+    }
+}
+
 int main(int argc, char **argv)
 {
     BlockBackend *blk;
@@ -761,8 +774,8 @@ int main(int argc, char **argv)
         memset(&client_thread, 0, sizeof(client_thread));
     }
 
-    qemu_set_fd_handler2(fd, nbd_can_accept, nbd_accept, NULL,
-                         (void *)(uintptr_t)fd);
+    server_fd = fd;
+    nbd_update_server_fd_handler(fd);
 
     /* now when the initialization is (almost) complete, chdir("/")
      * to free any busy filesystems */
-- 
2.4.1

[Qemu-devel] [PATCH v3 03/13] l2tpv3: Drop l2tpv3_can_send

[Fam Zheng]

This callback is called by main loop before polling s->fd, if it returns
false, the fd will not be polled in this iteration.

This is redundant with checks inside read callback. After this patch,
the data will be copied from s->fd to s->msgvec when it arrives. If the
device can't receive, it will be queued to incoming_queue, and when the
device status changes, this queue will be flushed.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 net/l2tpv3.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index 8c598b0..8eed06b 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -133,14 +133,12 @@ typedef struct NetL2TPV3State {
 
 } NetL2TPV3State;
 
-static int l2tpv3_can_send(void *opaque);
 static void net_l2tpv3_send(void *opaque);
 static void l2tpv3_writable(void *opaque);
 
 static void l2tpv3_update_fd_handler(NetL2TPV3State *s)
 {
-    qemu_set_fd_handler2(s->fd,
-                         s->read_poll ? l2tpv3_can_send : NULL,
+    qemu_set_fd_handler2(s->fd, NULL,
                          s->read_poll ? net_l2tpv3_send     : NULL,
                          s->write_poll ? l2tpv3_writable : NULL,
                          s);
@@ -169,13 +167,6 @@ static void l2tpv3_writable(void *opaque)
     qemu_flush_queued_packets(&s->nc);
 }
 
-static int l2tpv3_can_send(void *opaque)
-{
-    NetL2TPV3State *s = opaque;
-
-    return qemu_can_send_packet(&s->nc);
-}
-
 static void l2tpv3_send_completed(NetClientState *nc, ssize_t len)
 {
     NetL2TPV3State *s = DO_UPCAST(NetL2TPV3State, nc, nc);
-- 
2.4.1

[Qemu-devel] [PATCH v3 04/13] netmap: Drop netmap_can_send

[Fam Zheng]

This callback is called by main loop before polling s->fd, if it returns
false, the fd will not be polled in this iteration.

This is redundant with checks inside read callback. After this patch,
the data will be copied from s->fd to s->iov when it arrives. If the
device can't receive, it will be queued to incoming_queue, and when the
device status changes, this queue will be flushed.

Also remove the qemu_can_send_packet() check in netmap_send. If it's
true, we are good; if it's false, the qemu_sendv_packet_async would
return 0 and read poll will be disabled until netmap_send_completed is
called.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 net/netmap.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/net/netmap.c b/net/netmap.c
index 0c1772b..b3efb5b 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -132,23 +132,13 @@ error:
     return -1;
 }
 
-/* Tell the event-loop if the netmap backend can send packets
-   to the frontend. */
-static int netmap_can_send(void *opaque)
-{
-    NetmapState *s = opaque;
-
-    return qemu_can_send_packet(&s->nc);
-}
-
 static void netmap_send(void *opaque);
 static void netmap_writable(void *opaque);
 
 /* Set the event-loop handlers for the netmap backend. */
 static void netmap_update_fd_handler(NetmapState *s)
 {
-    qemu_set_fd_handler2(s->me.fd,
-                         s->read_poll  ? netmap_can_send : NULL,
+    qemu_set_fd_handler2(s->me.fd, NULL,
                          s->read_poll  ? netmap_send     : NULL,
                          s->write_poll ? netmap_writable : NULL,
                          s);
@@ -317,7 +307,7 @@ static void netmap_send(void *opaque)
 
     /* Keep sending while there are available packets into the netmap
        RX ring and the forwarding path towards the peer is open. */
-    while (!nm_ring_empty(ring) && qemu_can_send_packet(&s->nc)) {
+    while (!nm_ring_empty(ring)) {
         uint32_t i;
         uint32_t idx;
         bool morefrag;
-- 
2.4.1

[Qemu-devel] [PATCH v3 05/13] net/socket: Drop net_socket_can_send

[Fam Zheng]

This callback is called by main loop before polling s->fd, if it returns
false, the fd will not be polled in this iteration.

This is redundant with checks inside read callback. After this patch,
the data will be sent to peer when it arrives. If the device can't
receive, it will be queued to incoming_queue, and when the device status
changes, this queue will be flushed.

If the peer is not ready, disable the read poll until send completes.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 net/socket.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index c30e03f..82fa175 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -51,18 +51,9 @@ typedef struct NetSocketState {
 static void net_socket_accept(void *opaque);
 static void net_socket_writable(void *opaque);
 
-/* Only read packets from socket when peer can receive them */
-static int net_socket_can_send(void *opaque)
-{
-    NetSocketState *s = opaque;
-
-    return qemu_can_send_packet(&s->nc);
-}
-
 static void net_socket_update_fd_handler(NetSocketState *s)
 {
-    qemu_set_fd_handler2(s->fd,
-                         s->read_poll  ? net_socket_can_send : NULL,
+    qemu_set_fd_handler2(s->fd, NULL,
                          s->read_poll  ? s->send_fn : NULL,
                          s->write_poll ? net_socket_writable : NULL,
                          s);
@@ -142,6 +133,15 @@ static ssize_t net_socket_receive_dgram(NetClientState *nc, const uint8_t *buf,
     return ret;
 }
 
+static void net_socket_send_completed(NetClientState *nc, ssize_t len)
+{
+    NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc);
+
+    if (!s->read_poll) {
+        net_socket_read_poll(s, true);
+    }
+}
+
 static void net_socket_send(void *opaque)
 {
     NetSocketState *s = opaque;
@@ -211,9 +211,13 @@ static void net_socket_send(void *opaque)
             buf += l;
             size -= l;
             if (s->index >= s->packet_len) {
-                qemu_send_packet(&s->nc, s->buf, s->packet_len);
                 s->index = 0;
                 s->state = 0;
+                if (qemu_send_packet_async(&s->nc, s->buf, size,
+                                           net_socket_send_completed) == 0) {
+                    net_socket_read_poll(s, false);
+                    break;
+                }
             }
             break;
         }
@@ -234,7 +238,10 @@ static void net_socket_send_dgram(void *opaque)
         net_socket_write_poll(s, false);
         return;
     }
-    qemu_send_packet(&s->nc, s->buf, size);
+    if (qemu_send_packet_async(&s->nc, s->buf, size,
+                               net_socket_send_completed) == 0) {
+        net_socket_read_poll(s, false);
+    }
 }
 
 static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr *localaddr)
-- 
2.4.1

[Qemu-devel] [PATCH v3 06/13] tap: Drop tap_can_send

[Fam Zheng]

This callback is called by main loop before polling s->fd, if it returns
false, the fd will not be polled in this iteration.

This is redundant with checks inside read callback. After this patch,
the data will be sent to peer when it arrives. If the device can't
receive, it will be queued to incoming_queue, and when the device status
changes, this queue will be flushed.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 net/tap.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index 968df46..8407aea 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -61,14 +61,12 @@ typedef struct TAPState {
 
 static int launch_script(const char *setup_script, const char *ifname, int fd);
 
-static int tap_can_send(void *opaque);
 static void tap_send(void *opaque);
 static void tap_writable(void *opaque);
 
 static void tap_update_fd_handler(TAPState *s)
 {
-    qemu_set_fd_handler2(s->fd,
-                         s->read_poll && s->enabled ? tap_can_send : NULL,
+    qemu_set_fd_handler2(s->fd, NULL,
                          s->read_poll && s->enabled ? tap_send     : NULL,
                          s->write_poll && s->enabled ? tap_writable : NULL,
                          s);
@@ -165,13 +163,6 @@ static ssize_t tap_receive(NetClientState *nc, const uint8_t *buf, size_t size)
     return tap_write_packet(s, iov, 1);
 }
 
-static int tap_can_send(void *opaque)
-{
-    TAPState *s = opaque;
-
-    return qemu_can_send_packet(&s->nc);
-}
-
 #ifndef __sun__
 ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
 {
@@ -191,9 +182,12 @@ static void tap_send(void *opaque)
     int size;
     int packets = 0;
 
-    while (qemu_can_send_packet(&s->nc)) {
+    while (true) {
+        bool can_send;
         uint8_t *buf = s->buf;
 
+        can_send = qemu_can_send_packet(&s->nc);
+
         size = tap_read_packet(s->fd, s->buf, sizeof(s->buf));
         if (size <= 0) {
             break;
@@ -204,8 +198,12 @@ static void tap_send(void *opaque)
             size -= s->host_vnet_hdr_len;
         }
 
+        /* If !can_send, we will want to disable the read poll, but we still
+         * need the send completion callback to enable it again, which is a
+         * sign of peer becoming ready.  So call the send function anyway.
+         */
         size = qemu_send_packet_async(&s->nc, buf, size, tap_send_completed);
-        if (size == 0) {
+        if (size == 0 || !can_send) {
             tap_read_poll(s, false);
             break;
         } else if (size < 0) {
-- 
2.4.1

[Qemu-devel] [PATCH v3 07/13] Change qemu_set_fd_handler2(..., NULL, ...) to qemu_set_fd_handler

[Fam Zheng]

Done with following Coccinelle semantic patch, plus manual cosmetic changes in
net/*.c.

    @@
    expression E1, E2, E3, E4;
    @@
    -   qemu_set_fd_handler2(E1, NULL, E2, E3, E4);
    +   qemu_set_fd_handler(E1, E2, E3, E4);

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev-nbd.c         |  4 ++--
 main-loop.c            |  3 +--
 migration/exec.c       |  6 +++---
 migration/fd.c         |  4 ++--
 migration/rdma.c       |  7 +++----
 migration/tcp.c        |  6 +++---
 migration/unix.c       |  6 +++---
 net/l2tpv3.c           |  8 ++++----
 net/netmap.c           |  8 ++++----
 net/socket.c           |  8 ++++----
 net/tap.c              |  8 ++++----
 ui/vnc-auth-sasl.c     |  2 +-
 ui/vnc-auth-vencrypt.c |  2 +-
 ui/vnc-ws.c            |  6 +++---
 ui/vnc.c               | 27 ++++++++++++---------------
 util/qemu-sockets.c    |  8 +++-----
 16 files changed, 53 insertions(+), 60 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 85cda4c..0d9df47 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -43,7 +43,7 @@ void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
 
     server_fd = socket_listen(addr, errp);
     if (server_fd != -1) {
-        qemu_set_fd_handler2(server_fd, NULL, nbd_accept, NULL, NULL);
+        qemu_set_fd_handler(server_fd, nbd_accept, NULL, NULL);
     }
 }
 
@@ -129,7 +129,7 @@ void qmp_nbd_server_stop(Error **errp)
     }
 
     if (server_fd != -1) {
-        qemu_set_fd_handler2(server_fd, NULL, NULL, NULL, NULL);
+        qemu_set_fd_handler(server_fd, NULL, NULL, NULL);
         close(server_fd);
         server_fd = -1;
     }
diff --git a/main-loop.c b/main-loop.c
index 981bcb5..82875a4 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -100,8 +100,7 @@ static int qemu_signal_init(void)
 
     fcntl_setfl(sigfd, O_NONBLOCK);
 
-    qemu_set_fd_handler2(sigfd, NULL, sigfd_handler, NULL,
-                         (void *)(intptr_t)sigfd);
+    qemu_set_fd_handler(sigfd, sigfd_handler, NULL, (void *)(intptr_t)sigfd);
 
     return 0;
 }
diff --git a/migration/exec.c b/migration/exec.c
index 4790247..8406d2b 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -49,7 +49,7 @@ static void exec_accept_incoming_migration(void *opaque)
 {
     QEMUFile *f = opaque;
 
-    qemu_set_fd_handler2(qemu_get_fd(f), NULL, NULL, NULL, NULL);
+    qemu_set_fd_handler(qemu_get_fd(f), NULL, NULL, NULL);
     process_incoming_migration(f);
 }
 
@@ -64,6 +64,6 @@ void exec_start_incoming_migration(const char *command, Error **errp)
         return;
     }
 
-    qemu_set_fd_handler2(qemu_get_fd(f), NULL,
-			 exec_accept_incoming_migration, NULL, f);
+    qemu_set_fd_handler(qemu_get_fd(f), exec_accept_incoming_migration, NULL,
+                        f);
 }
diff --git a/migration/fd.c b/migration/fd.c
index 129da99..3e4bed0 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -62,7 +62,7 @@ static void fd_accept_incoming_migration(void *opaque)
 {
     QEMUFile *f = opaque;
 
-    qemu_set_fd_handler2(qemu_get_fd(f), NULL, NULL, NULL, NULL);
+    qemu_set_fd_handler(qemu_get_fd(f), NULL, NULL, NULL);
     process_incoming_migration(f);
 }
 
@@ -84,5 +84,5 @@ void fd_start_incoming_migration(const char *infd, Error **errp)
         return;
     }
 
-    qemu_set_fd_handler2(fd, NULL, fd_accept_incoming_migration, NULL, f);
+    qemu_set_fd_handler(fd, fd_accept_incoming_migration, NULL, f);
 }
diff --git a/migration/rdma.c b/migration/rdma.c
index 77e3444..171c23f 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -2834,7 +2834,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
         }
     }
 
-    qemu_set_fd_handler2(rdma->channel->fd, NULL, NULL, NULL, NULL);
+    qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
 
     ret = rdma_accept(rdma->cm_id, &conn_param);
     if (ret) {
@@ -3331,9 +3331,8 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp)
 
     trace_rdma_start_incoming_migration_after_rdma_listen();
 
-    qemu_set_fd_handler2(rdma->channel->fd, NULL,
-                         rdma_accept_incoming_migration, NULL,
-                            (void *)(intptr_t) rdma);
+    qemu_set_fd_handler(rdma->channel->fd, rdma_accept_incoming_migration,
+                        NULL, (void *)(intptr_t)rdma);
     return;
 err:
     error_propagate(errp, local_err);
diff --git a/migration/tcp.c b/migration/tcp.c
index 91c9cf3..ae89172 100644
--- a/migration/tcp.c
+++ b/migration/tcp.c
@@ -65,7 +65,7 @@ static void tcp_accept_incoming_migration(void *opaque)
         c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
         err = socket_error();
     } while (c < 0 && err == EINTR);
-    qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
+    qemu_set_fd_handler(s, NULL, NULL, NULL);
     closesocket(s);
 
     DPRINTF("accepted migration\n");
@@ -98,6 +98,6 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp)
         return;
     }
 
-    qemu_set_fd_handler2(s, NULL, tcp_accept_incoming_migration, NULL,
-                         (void *)(intptr_t)s);
+    qemu_set_fd_handler(s, tcp_accept_incoming_migration, NULL,
+                        (void *)(intptr_t)s);
 }
diff --git a/migration/unix.c b/migration/unix.c
index 1cdadfb..b591813 100644
--- a/migration/unix.c
+++ b/migration/unix.c
@@ -65,7 +65,7 @@ static void unix_accept_incoming_migration(void *opaque)
         c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
         err = errno;
     } while (c < 0 && err == EINTR);
-    qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
+    qemu_set_fd_handler(s, NULL, NULL, NULL);
     close(s);
 
     DPRINTF("accepted migration\n");
@@ -98,6 +98,6 @@ void unix_start_incoming_migration(const char *path, Error **errp)
         return;
     }
 
-    qemu_set_fd_handler2(s, NULL, unix_accept_incoming_migration, NULL,
-                         (void *)(intptr_t)s);
+    qemu_set_fd_handler(s, unix_accept_incoming_migration, NULL,
+                        (void *)(intptr_t)s);
 }
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index 8eed06b..34aa3da 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -138,10 +138,10 @@ static void l2tpv3_writable(void *opaque);
 
 static void l2tpv3_update_fd_handler(NetL2TPV3State *s)
 {
-    qemu_set_fd_handler2(s->fd, NULL,
-                         s->read_poll ? net_l2tpv3_send     : NULL,
-                         s->write_poll ? l2tpv3_writable : NULL,
-                         s);
+    qemu_set_fd_handler(s->fd,
+                        s->read_poll ? net_l2tpv3_send : NULL,
+                        s->write_poll ? l2tpv3_writable : NULL,
+                        s);
 }
 
 static void l2tpv3_read_poll(NetL2TPV3State *s, bool enable)
diff --git a/net/netmap.c b/net/netmap.c
index b3efb5b..8103222 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -138,10 +138,10 @@ static void netmap_writable(void *opaque);
 /* Set the event-loop handlers for the netmap backend. */
 static void netmap_update_fd_handler(NetmapState *s)
 {
-    qemu_set_fd_handler2(s->me.fd, NULL,
-                         s->read_poll  ? netmap_send     : NULL,
-                         s->write_poll ? netmap_writable : NULL,
-                         s);
+    qemu_set_fd_handler(s->me.fd,
+                        s->read_poll ? netmap_send : NULL,
+                        s->write_poll ? netmap_writable : NULL,
+                        s);
 }
 
 /* Update the read handler. */
diff --git a/net/socket.c b/net/socket.c
index 82fa175..3514d2d 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -53,10 +53,10 @@ static void net_socket_writable(void *opaque);
 
 static void net_socket_update_fd_handler(NetSocketState *s)
 {
-    qemu_set_fd_handler2(s->fd, NULL,
-                         s->read_poll  ? s->send_fn : NULL,
-                         s->write_poll ? net_socket_writable : NULL,
-                         s);
+    qemu_set_fd_handler(s->fd,
+                        s->read_poll ? s->send_fn : NULL,
+                        s->write_poll ? net_socket_writable : NULL,
+                        s);
 }
 
 static void net_socket_read_poll(NetSocketState *s, bool enable)
diff --git a/net/tap.c b/net/tap.c
index 8407aea..5220cd1 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -66,10 +66,10 @@ static void tap_writable(void *opaque);
 
 static void tap_update_fd_handler(TAPState *s)
 {
-    qemu_set_fd_handler2(s->fd, NULL,
-                         s->read_poll && s->enabled ? tap_send     : NULL,
-                         s->write_poll && s->enabled ? tap_writable : NULL,
-                         s);
+    qemu_set_fd_handler(s->fd,
+                        s->read_poll && s->enabled ? tap_send : NULL,
+                        s->write_poll && s->enabled ? tap_writable : NULL,
+                        s);
 }
 
 static void tap_read_poll(TAPState *s, bool enable)
diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 2ddd259..62a5fc4 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -86,7 +86,7 @@ long vnc_client_write_sasl(VncState *vs)
      * SASL encoded output
      */
     if (vs->output.offset == 0) {
-        qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
+        qemu_set_fd_handler(vs->csock, vnc_client_read, NULL, vs);
     }
 
     return ret;
diff --git a/ui/vnc-auth-vencrypt.c b/ui/vnc-auth-vencrypt.c
index 03ea48a..8fc965b 100644
--- a/ui/vnc-auth-vencrypt.c
+++ b/ui/vnc-auth-vencrypt.c
@@ -94,7 +94,7 @@ static int vnc_start_vencrypt_handshake(VncState *vs)
     }
 
     VNC_DEBUG("Handshake done, switching to TLS data mode\n");
-    qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, vnc_client_write, vs);
+    qemu_set_fd_handler(vs->csock, vnc_client_read, vnc_client_write, vs);
 
     start_auth_vencrypt_subauth(vs);
 
diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c
index 38a1b8b..8c18268 100644
--- a/ui/vnc-ws.c
+++ b/ui/vnc-ws.c
@@ -56,7 +56,7 @@ static int vncws_start_tls_handshake(VncState *vs)
     }
 
     VNC_DEBUG("Handshake done, switching to TLS data mode\n");
-    qemu_set_fd_handler2(vs->csock, NULL, vncws_handshake_read, NULL, vs);
+    qemu_set_fd_handler(vs->csock, vncws_handshake_read, NULL, vs);
 
     return 0;
 }
@@ -98,7 +98,7 @@ void vncws_handshake_read(void *opaque)
     handshake_end = (uint8_t *)g_strstr_len((char *)vs->ws_input.buffer,
             vs->ws_input.offset, WS_HANDSHAKE_END);
     if (handshake_end) {
-        qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
+        qemu_set_fd_handler(vs->csock, vnc_client_read, NULL, vs);
         vncws_process_handshake(vs, vs->ws_input.buffer, vs->ws_input.offset);
         buffer_advance(&vs->ws_input, handshake_end - vs->ws_input.buffer +
                 strlen(WS_HANDSHAKE_END));
@@ -176,7 +176,7 @@ long vnc_client_write_ws(VncState *vs)
     buffer_advance(&vs->ws_output, ret);
 
     if (vs->ws_output.offset == 0) {
-        qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
+        qemu_set_fd_handler(vs->csock, vnc_client_read, NULL, vs);
     }
 
     return ret;
diff --git a/ui/vnc.c b/ui/vnc.c
index 9f8ecd0..229bce8 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1213,7 +1213,7 @@ static void vnc_disconnect_start(VncState *vs)
     if (vs->csock == -1)
         return;
     vnc_set_share_mode(vs, VNC_SHARE_MODE_DISCONNECTED);
-    qemu_set_fd_handler2(vs->csock, NULL, NULL, NULL, NULL);
+    qemu_set_fd_handler(vs->csock, NULL, NULL, NULL);
     closesocket(vs->csock);
     vs->csock = -1;
 }
@@ -1387,7 +1387,7 @@ static long vnc_client_write_plain(VncState *vs)
     buffer_advance(&vs->output, ret);
 
     if (vs->output.offset == 0) {
-        qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
+        qemu_set_fd_handler(vs->csock, vnc_client_read, NULL, vs);
     }
 
     return ret;
@@ -1434,7 +1434,7 @@ void vnc_client_write(void *opaque)
             ) {
         vnc_client_write_locked(opaque);
     } else if (vs->csock != -1) {
-        qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
+        qemu_set_fd_handler(vs->csock, vnc_client_read, NULL, vs);
     }
     vnc_unlock_output(vs);
 }
@@ -1581,7 +1581,7 @@ void vnc_write(VncState *vs, const void *data, size_t len)
     buffer_reserve(&vs->output, len);
 
     if (vs->csock != -1 && buffer_empty(&vs->output)) {
-        qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, vnc_client_write, vs);
+        qemu_set_fd_handler(vs->csock, vnc_client_read, vnc_client_write, vs);
     }
 
     buffer_append(&vs->output, data, len);
@@ -3022,18 +3022,16 @@ static void vnc_connect(VncDisplay *vd, int csock,
         vs->websocket = 1;
 #ifdef CONFIG_VNC_TLS
         if (vd->ws_tls) {
-            qemu_set_fd_handler2(vs->csock, NULL, vncws_tls_handshake_io,
-                                 NULL, vs);
+            qemu_set_fd_handler(vs->csock, vncws_tls_handshake_io, NULL, vs);
         } else
 #endif /* CONFIG_VNC_TLS */
         {
-            qemu_set_fd_handler2(vs->csock, NULL, vncws_handshake_read,
-                                 NULL, vs);
+            qemu_set_fd_handler(vs->csock, vncws_handshake_read, NULL, vs);
         }
     } else
 #endif /* CONFIG_VNC_WS */
     {
-        qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
+        qemu_set_fd_handler(vs->csock, vnc_client_read, NULL, vs);
     }
 
     vnc_client_cache_addr(vs);
@@ -3182,14 +3180,14 @@ static void vnc_display_close(VncDisplay *vs)
     vs->enabled = false;
     vs->is_unix = false;
     if (vs->lsock != -1) {
-        qemu_set_fd_handler2(vs->lsock, NULL, NULL, NULL, NULL);
+        qemu_set_fd_handler(vs->lsock, NULL, NULL, NULL);
         close(vs->lsock);
         vs->lsock = -1;
     }
 #ifdef CONFIG_VNC_WS
     vs->ws_enabled = false;
     if (vs->lwebsock != -1) {
-        qemu_set_fd_handler2(vs->lwebsock, NULL, NULL, NULL, NULL);
+        qemu_set_fd_handler(vs->lwebsock, NULL, NULL, NULL);
         close(vs->lwebsock);
         vs->lwebsock = -1;
     }
@@ -3705,12 +3703,11 @@ void vnc_display_open(const char *id, Error **errp)
 #endif /* CONFIG_VNC_WS */
         }
         vs->enabled = true;
-        qemu_set_fd_handler2(vs->lsock, NULL,
-                vnc_listen_regular_read, NULL, vs);
+        qemu_set_fd_handler(vs->lsock, vnc_listen_regular_read, NULL, vs);
 #ifdef CONFIG_VNC_WS
         if (vs->ws_enabled) {
-            qemu_set_fd_handler2(vs->lwebsock, NULL,
-                    vnc_listen_websocket_read, NULL, vs);
+            qemu_set_fd_handler(vs->lwebsock, vnc_listen_websocket_read,
+                                NULL, vs);
         }
 #endif /* CONFIG_VNC_WS */
     }
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 87c9bc6..ca83379 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -238,7 +238,7 @@ static void wait_for_connect(void *opaque)
     bool in_progress;
     Error *err = NULL;
 
-    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+    qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
 
     do {
         rc = qemu_getsockopt(s->fd, SOL_SOCKET, SO_ERROR, &val, &valsize);
@@ -310,8 +310,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
 
     if (connect_state != NULL && QEMU_SOCKET_RC_INPROGRESS(rc)) {
         connect_state->fd = sock;
-        qemu_set_fd_handler2(sock, NULL, NULL, wait_for_connect,
-                             connect_state);
+        qemu_set_fd_handler(sock, NULL, wait_for_connect, connect_state);
         *in_progress = true;
     } else if (rc < 0) {
         error_setg_errno(errp, errno, "Failed to connect socket");
@@ -785,8 +784,7 @@ int unix_connect_opts(QemuOpts *opts, Error **errp,
 
     if (connect_state != NULL && QEMU_SOCKET_RC_INPROGRESS(rc)) {
         connect_state->fd = sock;
-        qemu_set_fd_handler2(sock, NULL, NULL, wait_for_connect,
-                             connect_state);
+        qemu_set_fd_handler(sock, NULL, wait_for_connect, connect_state);
         return sock;
     } else if (rc >= 0) {
         /* non blocking socket immediate success, call callback */
-- 
2.4.1

[Qemu-devel] [PATCH v3 08/13] main-loop: Drop qemu_set_fd_handler2

[Fam Zheng]

All users are converted to qemu_set_fd_handler now, drop
qemu_set_fd_handler2 and IOHandlerRecord.fd_read_poll.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/block/aio.h      |  2 +-
 include/qemu/main-loop.h | 49 +-----------------------------------------------
 iohandler.c              | 26 +++++--------------------
 stubs/set-fd-handler.c   |  9 ---------
 4 files changed, 7 insertions(+), 79 deletions(-)

diff --git a/include/block/aio.h b/include/block/aio.h
index d2bb423..b46103e 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -241,7 +241,7 @@ bool aio_dispatch(AioContext *ctx);
 bool aio_poll(AioContext *ctx, bool blocking);
 
 /* Register a file descriptor and associated callbacks.  Behaves very similarly
- * to qemu_set_fd_handler2.  Unlike qemu_set_fd_handler2, these callbacks will
+ * to qemu_set_fd_handler.  Unlike qemu_set_fd_handler, these callbacks will
  * be invoked when using aio_poll().
  *
  * Code that invokes AIO completion functions should rely on this function
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 62c68c0..7da1d63 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -96,8 +96,7 @@ AioContext *qemu_get_aio_context(void);
  * that the main loop waits for.
  *
  * Calling qemu_notify_event is rarely necessary, because main loop
- * services (bottom halves and timers) call it themselves.  One notable
- * exception occurs when using qemu_set_fd_handler2 (see below).
+ * services (bottom halves and timers) call it themselves.
  */
 void qemu_notify_event(void);
 
@@ -172,52 +171,6 @@ typedef void IOReadHandler(void *opaque, const uint8_t *buf, int size);
 typedef int IOCanReadHandler(void *opaque);
 
 /**
- * qemu_set_fd_handler2: Register a file descriptor with the main loop
- *
- * This function tells the main loop to wake up whenever one of the
- * following conditions is true:
- *
- * 1) if @fd_write is not %NULL, when the file descriptor is writable;
- *
- * 2) if @fd_read is not %NULL, when the file descriptor is readable.
- *
- * @fd_read_poll can be used to disable the @fd_read callback temporarily.
- * This is useful to avoid calling qemu_set_fd_handler2 every time the
- * client becomes interested in reading (or dually, stops being interested).
- * A typical example is when @fd is a listening socket and you want to bound
- * the number of active clients.  Remember to call qemu_notify_event whenever
- * the condition may change from %false to %true.
- *
- * The callbacks that are set up by qemu_set_fd_handler2 are level-triggered.
- * If @fd_read does not read from @fd, or @fd_write does not write to @fd
- * until its buffers are full, they will be called again on the next
- * iteration.
- *
- * @fd: The file descriptor to be observed.  Under Windows it must be
- * a #SOCKET.
- *
- * @fd_read_poll: A function that returns 1 if the @fd_read callback
- * should be fired.  If the function returns 0, the main loop will not
- * end its iteration even if @fd becomes readable.
- *
- * @fd_read: A level-triggered callback that is fired if @fd is readable
- * at the beginning of a main loop iteration, or if it becomes readable
- * during one.
- *
- * @fd_write: A level-triggered callback that is fired when @fd is writable
- * at the beginning of a main loop iteration, or if it becomes writable
- * during one.
- *
- * @opaque: A pointer-sized value that is passed to @fd_read_poll,
- * @fd_read and @fd_write.
- */
-int qemu_set_fd_handler2(int fd,
-                         IOCanReadHandler *fd_read_poll,
-                         IOHandler *fd_read,
-                         IOHandler *fd_write,
-                         void *opaque);
-
-/**
  * qemu_set_fd_handler: Register a file descriptor with the main loop
  *
  * This function tells the main loop to wake up whenever one of the
diff --git a/iohandler.c b/iohandler.c
index cca614f..d361cf2 100644
--- a/iohandler.c
+++ b/iohandler.c
@@ -33,7 +33,6 @@
 #endif
 
 typedef struct IOHandlerRecord {
-    IOCanReadHandler *fd_read_poll;
     IOHandler *fd_read;
     IOHandler *fd_write;
     void *opaque;
@@ -46,14 +45,10 @@ typedef struct IOHandlerRecord {
 static QLIST_HEAD(, IOHandlerRecord) io_handlers =
     QLIST_HEAD_INITIALIZER(io_handlers);
 
-
-/* XXX: fd_read_poll should be suppressed, but an API change is
-   necessary in the character devices to suppress fd_can_read(). */
-int qemu_set_fd_handler2(int fd,
-                         IOCanReadHandler *fd_read_poll,
-                         IOHandler *fd_read,
-                         IOHandler *fd_write,
-                         void *opaque)
+int qemu_set_fd_handler(int fd,
+                        IOHandler *fd_read,
+                        IOHandler *fd_write,
+                        void *opaque)
 {
     IOHandlerRecord *ioh;
 
@@ -75,7 +70,6 @@ int qemu_set_fd_handler2(int fd,
         QLIST_INSERT_HEAD(&io_handlers, ioh, next);
     found:
         ioh->fd = fd;
-        ioh->fd_read_poll = fd_read_poll;
         ioh->fd_read = fd_read;
         ioh->fd_write = fd_write;
         ioh->opaque = opaque;
@@ -86,14 +80,6 @@ int qemu_set_fd_handler2(int fd,
     return 0;
 }
 
-int qemu_set_fd_handler(int fd,
-                        IOHandler *fd_read,
-                        IOHandler *fd_write,
-                        void *opaque)
-{
-    return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
-}
-
 void qemu_iohandler_fill(GArray *pollfds)
 {
     IOHandlerRecord *ioh;
@@ -103,9 +89,7 @@ void qemu_iohandler_fill(GArray *pollfds)
 
         if (ioh->deleted)
             continue;
-        if (ioh->fd_read &&
-            (!ioh->fd_read_poll ||
-             ioh->fd_read_poll(ioh->opaque) != 0)) {
+        if (ioh->fd_read) {
             events |= G_IO_IN | G_IO_HUP | G_IO_ERR;
         }
         if (ioh->fd_write) {
diff --git a/stubs/set-fd-handler.c b/stubs/set-fd-handler.c
index 25cca8c..a895e62 100644
--- a/stubs/set-fd-handler.c
+++ b/stubs/set-fd-handler.c
@@ -8,12 +8,3 @@ int qemu_set_fd_handler(int fd,
 {
     abort();
 }
-
-int qemu_set_fd_handler2(int fd,
-                         IOCanReadHandler *fd_read_poll,
-                         IOHandler *fd_read,
-                         IOHandler *fd_write,
-                         void *opaque)
-{
-    abort();
-}
-- 
2.4.1

[Qemu-devel] [PATCH v3 09/13] alsaaudio: Remove unused error handling of qemu_set_fd_handler

[Fam Zheng]

The function cannot fail, so the check is superfluous.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 audio/alsaaudio.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/audio/alsaaudio.c b/audio/alsaaudio.c
index 74ead97..ed7655d 100644
--- a/audio/alsaaudio.c
+++ b/audio/alsaaudio.c
@@ -266,31 +266,19 @@ static int alsa_poll_helper (snd_pcm_t *handle, struct pollhlp *hlp, int mask)
 
     for (i = 0; i < count; ++i) {
         if (pfds[i].events & POLLIN) {
-            err = qemu_set_fd_handler (pfds[i].fd, alsa_poll_handler,
-                                       NULL, hlp);
+            qemu_set_fd_handler (pfds[i].fd, alsa_poll_handler, NULL, hlp);
         }
         if (pfds[i].events & POLLOUT) {
             if (conf.verbose) {
                 dolog ("POLLOUT %d %d\n", i, pfds[i].fd);
             }
-            err = qemu_set_fd_handler (pfds[i].fd, NULL,
-                                       alsa_poll_handler, hlp);
+            qemu_set_fd_handler (pfds[i].fd, NULL, alsa_poll_handler, hlp);
         }
         if (conf.verbose) {
             dolog ("Set handler events=%#x index=%d fd=%d err=%d\n",
                    pfds[i].events, i, pfds[i].fd, err);
         }
 
-        if (err) {
-            dolog ("Failed to set handler events=%#x index=%d fd=%d err=%d\n",
-                   pfds[i].events, i, pfds[i].fd, err);
-
-            while (i--) {
-                qemu_set_fd_handler (pfds[i].fd, NULL, NULL, NULL);
-            }
-            g_free (pfds);
-            return -1;
-        }
     }
     hlp->pfds = pfds;
     hlp->count = count;
-- 
2.4.1

[Qemu-devel] [PATCH v3 10/13] oss: Remove unused error handling of qemu_set_fd_handler

[Fam Zheng]

The function cannot fail, so the check is superfluous.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 audio/ossaudio.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/audio/ossaudio.c b/audio/ossaudio.c
index 4db2ca6..b9c6b30 100644
--- a/audio/ossaudio.c
+++ b/audio/ossaudio.c
@@ -138,18 +138,18 @@ static void oss_helper_poll_in (void *opaque)
     audio_run ("oss_poll_in");
 }
 
-static int oss_poll_out (HWVoiceOut *hw)
+static void oss_poll_out (HWVoiceOut *hw)
 {
     OSSVoiceOut *oss = (OSSVoiceOut *) hw;
 
-    return qemu_set_fd_handler (oss->fd, NULL, oss_helper_poll_out, NULL);
+    qemu_set_fd_handler (oss->fd, NULL, oss_helper_poll_out, NULL);
 }
 
-static int oss_poll_in (HWVoiceIn *hw)
+static void oss_poll_in (HWVoiceIn *hw)
 {
     OSSVoiceIn *oss = (OSSVoiceIn *) hw;
 
-    return qemu_set_fd_handler (oss->fd, oss_helper_poll_in, NULL, NULL);
+    qemu_set_fd_handler (oss->fd, oss_helper_poll_in, NULL, NULL);
 }
 
 static int oss_write (SWVoiceOut *sw, void *buf, int len)
@@ -634,7 +634,8 @@ static int oss_ctl_out (HWVoiceOut *hw, int cmd, ...)
             va_end (ap);
 
             ldebug ("enabling voice\n");
-            if (poll_mode && oss_poll_out (hw)) {
+            if (poll_mode) {
+                oss_poll_out (hw);
                 poll_mode = 0;
             }
             hw->poll_mode = poll_mode;
@@ -828,7 +829,8 @@ static int oss_ctl_in (HWVoiceIn *hw, int cmd, ...)
             poll_mode = va_arg (ap, int);
             va_end (ap);
 
-            if (poll_mode && oss_poll_in (hw)) {
+            if (poll_mode) {
+                oss_poll_in (hw);
                 poll_mode = 0;
             }
             hw->poll_mode = poll_mode;
-- 
2.4.1

[Qemu-devel] [PATCH v3 11/13] xen_backend: Remove unused error handling of qemu_set_fd_handler

[Fam Zheng]

The function cannot fail, so the check is superfluous.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/xen/xen_backend.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index b2cb22b..2510e2e 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -714,9 +714,7 @@ int xen_be_init(void)
         return -1;
     }
 
-    if (qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL) < 0) {
-        goto err;
-    }
+    qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL);
 
     if (xen_xc == XC_HANDLER_INITIAL_VALUE) {
         /* Check if xen_init() have been called */
-- 
2.4.1

[Qemu-devel] [PATCH v3 12/13] event-notifier: Always return 0 for posix implementation

[Fam Zheng]

qemu_set_fd_handler cannot fail, let's always return 0.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 util/event_notifier-posix.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/event_notifier-posix.c b/util/event_notifier-posix.c
index 8442c6e..ed4ca2b 100644
--- a/util/event_notifier-posix.c
+++ b/util/event_notifier-posix.c
@@ -85,7 +85,8 @@ int event_notifier_get_fd(EventNotifier *e)
 int event_notifier_set_handler(EventNotifier *e,
                                EventNotifierHandler *handler)
 {
-    return qemu_set_fd_handler(e->rfd, (IOHandler *)handler, NULL, e);
+    qemu_set_fd_handler(e->rfd, (IOHandler *)handler, NULL, e);
+    return 0;
 }
 
 int event_notifier_set(EventNotifier *e)
-- 
2.4.1

[Qemu-devel] [PATCH v3 13/13] iohandler: Change return type of qemu_set_fd_handler to "void"

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/qemu/main-loop.h | 8 ++++----
 iohandler.c              | 9 ++++-----
 stubs/set-fd-handler.c   | 8 ++++----
 3 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 7da1d63..0f4a0fd 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -198,10 +198,10 @@ typedef int IOCanReadHandler(void *opaque);
  *
  * @opaque: A pointer-sized value that is passed to @fd_read and @fd_write.
  */
-int qemu_set_fd_handler(int fd,
-                        IOHandler *fd_read,
-                        IOHandler *fd_write,
-                        void *opaque);
+void qemu_set_fd_handler(int fd,
+                         IOHandler *fd_read,
+                         IOHandler *fd_write,
+                         void *opaque);
 
 #ifdef CONFIG_POSIX
 /**
diff --git a/iohandler.c b/iohandler.c
index d361cf2..826f713 100644
--- a/iohandler.c
+++ b/iohandler.c
@@ -45,10 +45,10 @@ typedef struct IOHandlerRecord {
 static QLIST_HEAD(, IOHandlerRecord) io_handlers =
     QLIST_HEAD_INITIALIZER(io_handlers);
 
-int qemu_set_fd_handler(int fd,
-                        IOHandler *fd_read,
-                        IOHandler *fd_write,
-                        void *opaque)
+void qemu_set_fd_handler(int fd,
+                         IOHandler *fd_read,
+                         IOHandler *fd_write,
+                         void *opaque)
 {
     IOHandlerRecord *ioh;
 
@@ -77,7 +77,6 @@ int qemu_set_fd_handler(int fd,
         ioh->deleted = 0;
         qemu_notify_event();
     }
-    return 0;
 }
 
 void qemu_iohandler_fill(GArray *pollfds)
diff --git a/stubs/set-fd-handler.c b/stubs/set-fd-handler.c
index a895e62..a8481bc 100644
--- a/stubs/set-fd-handler.c
+++ b/stubs/set-fd-handler.c
@@ -1,10 +1,10 @@
 #include "qemu-common.h"
 #include "qemu/main-loop.h"
 
-int qemu_set_fd_handler(int fd,
-                        IOHandler *fd_read,
-                        IOHandler *fd_write,
-                        void *opaque)
+void qemu_set_fd_handler(int fd,
+                         IOHandler *fd_read,
+                         IOHandler *fd_write,
+                         void *opaque)
 {
     abort();
 }
-- 
2.4.1

Re: [Qemu-devel] [Qemu-block] [PATCH v3 02/13] qemu-nbd: Switch to qemu_set_fd_handler

[Stefan Hajnoczi]

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

On Tue, May 19, 2015 at 10:50:59AM +0000, Fam Zheng wrote:
> Achieved by:
> 
> - Remembering the server fd with a global variable, in order to access
>   it from nbd_client_closed.
> 
> - Checking nbd_can_accept() and updating server_fd handler whenever
>   client connects or disconnects.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-nbd.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 03/13] l2tpv3: Drop l2tpv3_can_send

[Stefan Hajnoczi]

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

On Tue, May 19, 2015 at 10:51:00AM +0000, Fam Zheng wrote:
> This callback is called by main loop before polling s->fd, if it returns
> false, the fd will not be polled in this iteration.
> 
> This is redundant with checks inside read callback. After this patch,
> the data will be copied from s->fd to s->msgvec when it arrives. If the
> device can't receive, it will be queued to incoming_queue, and when the
> device status changes, this queue will be flushed.

This doesn't work because s->msgvec can fill up when
qemu_can_send_packet() returns false.  At that point we burn 100% CPU
because the file descriptor is still being monitored.

It really is necessary to stop monitoring the file descriptor until we
can send again.  Or the code needs to be changed to drop packets in QEMU
(currently it's the host kernel where packets may be dropped).

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 04/13] netmap: Drop netmap_can_send

[Stefan Hajnoczi]

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

On Tue, May 19, 2015 at 10:51:01AM +0000, Fam Zheng wrote:
> This callback is called by main loop before polling s->fd, if it returns
> false, the fd will not be polled in this iteration.
> 
> This is redundant with checks inside read callback. After this patch,
> the data will be copied from s->fd to s->iov when it arrives. If the
> device can't receive, it will be queued to incoming_queue, and when the
> device status changes, this queue will be flushed.
> 
> Also remove the qemu_can_send_packet() check in netmap_send. If it's
> true, we are good; if it's false, the qemu_sendv_packet_async would
> return 0 and read poll will be disabled until netmap_send_completed is
> called.

This causes unbounded memory usage in QEMU because
qemu_net_queue_append_iov() does not drop packets when sent_cb != NULL.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 00/13] main-loop: Get rid of fd_read_poll and qemu_set_fd_handler2

[Stefan Hajnoczi]

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

On Tue, May 19, 2015 at 10:50:57AM +0000, Fam Zheng wrote:
> v3: Replace previous 13 with a simple return type conversion patch.
>     Drop RFC.
> 
> This carries out the mandate in the comment of qemu_set_fd_handler2 and removes
> fd_read_poll from the code base, because it will make the work easier to
> convert ppoll to epoll in main loop.  Also, the aio interface doesn't have a
> read poll callback, which means this conversion woule be necessary if we want
> to move iohandler fds from main loop to AioContext.
> 
> There are five users of the read poll callback now: qemu-nbd, l2tpv3, netmap,
> socket and tap, which are all covered.
> 
> Patch 1 adds a stub for qemu_set_fd_handler which will be referenced in coming
> patches.
> 
> Patch 2 converts qemu-nbd which compares two global numbers in the fd_read_poll
> callback.
> 
> Patches 3~6 converts the four net devices, all of which checks
> qemu_can_send_packet() in the callback.
> 
> Patch 7 and 8 finally removes the function.
> 
> The rest patches are cleanup for the unuseful return value of
> qemu_set_fd_handler.
> 
> Please review!

I looked at the first few patches.

The qemu-nbd.c conversion is good.  It finds locations where read_poll
state transitions happen and updates file descriptor monitoring there.

The l2tpv3 and netmap conversion throws away read_poll and introduces
problems instead.

Please look at the patches again and either:

1. Convert everything like you converted qemu-nbd.c.  This is a
   conservative approach and we can be confident that behavior is
   unchanged.

2. Convert more carefully, making sure that packets are actually
   dropped and QEMU does not spin on a readable fd.

I prefer #1 because it's easier to check that behavior is still correct.
It also minimizes queuing inside QEMU.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 00/13] main-loop: Get rid of fd_read_poll and qemu_set_fd_handler2

[Fam Zheng]

On Tue, 05/19 16:02, Stefan Hajnoczi wrote:
> On Tue, May 19, 2015 at 10:50:57AM +0000, Fam Zheng wrote:
> > v3: Replace previous 13 with a simple return type conversion patch.
> >     Drop RFC.
> > 
> > This carries out the mandate in the comment of qemu_set_fd_handler2 and removes
> > fd_read_poll from the code base, because it will make the work easier to
> > convert ppoll to epoll in main loop.  Also, the aio interface doesn't have a
> > read poll callback, which means this conversion woule be necessary if we want
> > to move iohandler fds from main loop to AioContext.
> > 
> > There are five users of the read poll callback now: qemu-nbd, l2tpv3, netmap,
> > socket and tap, which are all covered.
> > 
> > Patch 1 adds a stub for qemu_set_fd_handler which will be referenced in coming
> > patches.
> > 
> > Patch 2 converts qemu-nbd which compares two global numbers in the fd_read_poll
> > callback.
> > 
> > Patches 3~6 converts the four net devices, all of which checks
> > qemu_can_send_packet() in the callback.
> > 
> > Patch 7 and 8 finally removes the function.
> > 
> > The rest patches are cleanup for the unuseful return value of
> > qemu_set_fd_handler.
> > 
> > Please review!
> 
> I looked at the first few patches.
> 
> The qemu-nbd.c conversion is good.  It finds locations where read_poll
> state transitions happen and updates file descriptor monitoring there.

Paolo, will you apply the qemu-nbd.c patch? I'll split net patches into a
separate series.

> 
> The l2tpv3 and netmap conversion throws away read_poll and introduces
> problems instead.
> 
> Please look at the patches again and either:
> 
> 1. Convert everything like you converted qemu-nbd.c.  This is a
>    conservative approach and we can be confident that behavior is
>    unchanged.

OK, I'll try this.

Fam

> 
> 2. Convert more carefully, making sure that packets are actually
>    dropped and QEMU does not spin on a readable fd.
> 
> I prefer #1 because it's easier to check that behavior is still correct.
> It also minimizes queuing inside QEMU.
> 
> Stefan

Re: [Qemu-devel] [Qemu-block] [PATCH v3 00/13] main-loop: Get rid of fd_read_poll and qemu_set_fd_handler2

[Paolo Bonzini]

On 19/05/2015 17:02, Stefan Hajnoczi wrote:
> 1. Convert everything like you converted qemu-nbd.c.  This is a 
> conservative approach and we can be confident that behavior is 
> unchanged.

So, that means whenever you change receive_disabled you call a new
callback on the peer?  In addition, whenever the count of
receive-disabled ports switches from zero to non-zero or vice versa,
hubs need to inform all its ports.

There are just two places that set/clear receive_disabled,
qemu_deliver_packet and qemu_flush_or_purge_queued_packets, but I
think a new API is needed to implement the callback for hubs
(qemu_send_enable/qemu_send_disable).

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v3 00/13] main-loop: Get rid of fd_read_poll and qemu_set_fd_handler2

[Fam Zheng]

On Wed, 05/20 08:26, Paolo Bonzini wrote:
> 
> 
> On 19/05/2015 17:02, Stefan Hajnoczi wrote:
> > 1. Convert everything like you converted qemu-nbd.c.  This is a 
> > conservative approach and we can be confident that behavior is 
> > unchanged.
> 
> So, that means whenever you change receive_disabled you call a new
> callback on the peer?  In addition, whenever the count of
> receive-disabled ports switches from zero to non-zero or vice versa,
> hubs need to inform all its ports.
> 
> There are just two places that set/clear receive_disabled,
> qemu_deliver_packet and qemu_flush_or_purge_queued_packets, but I
> think a new API is needed to implement the callback for hubs
> (qemu_send_enable/qemu_send_disable).
> 

I think .can_receive is the harder one, I'm not sure it's feasible - each
device has its own set of conditions, so it will be a huge change.

Fam

Re: [Qemu-devel] [PATCH v3 00/13] main-loop: Get rid of fd_read_poll and qemu_set_fd_handler2

[Paolo Bonzini]

On 20/05/2015 08:38, Fam Zheng wrote:
> On Wed, 05/20 08:26, Paolo Bonzini wrote:
>>
>>
>> On 19/05/2015 17:02, Stefan Hajnoczi wrote:
>>> 1. Convert everything like you converted qemu-nbd.c.  This is a 
>>> conservative approach and we can be confident that behavior is 
>>> unchanged.
>>
>> So, that means whenever you change receive_disabled you call a new
>> callback on the peer?  In addition, whenever the count of
>> receive-disabled ports switches from zero to non-zero or vice versa,
>> hubs need to inform all its ports.
>>
>> There are just two places that set/clear receive_disabled,
>> qemu_deliver_packet and qemu_flush_or_purge_queued_packets, but I
>> think a new API is needed to implement the callback for hubs
>> (qemu_send_enable/qemu_send_disable).
> 
> I think .can_receive is the harder one, I'm not sure it's feasible - each
> device has its own set of conditions, so it will be a huge change.

The 1->0 transition is easy because it happens when message delivery
fails.  I know the network code very little, but I think queuing exactly
one packet in this case should be acceptable.  If this is true, the
network code can detect the 1->0 transition automatically.

The 0->1 transition should be easy in principle, because NICs are
supposed to call qemu_flush_queued_packets when it happens.  Not that
they do, but you can find some very old and partial work in branch
rx-flush of my github repo.

Paolo

Re: [Qemu-devel] [PATCH v3 00/13] main-loop: Get rid of fd_read_poll and qemu_set_fd_handler2

[Stefan Hajnoczi]

On Wed, May 20, 2015 at 8:39 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 20/05/2015 08:38, Fam Zheng wrote:
>> On Wed, 05/20 08:26, Paolo Bonzini wrote:
>>>
>>>
>>> On 19/05/2015 17:02, Stefan Hajnoczi wrote:
>>>> 1. Convert everything like you converted qemu-nbd.c.  This is a
>>>> conservative approach and we can be confident that behavior is
>>>> unchanged.
>>>
>>> So, that means whenever you change receive_disabled you call a new
>>> callback on the peer?  In addition, whenever the count of
>>> receive-disabled ports switches from zero to non-zero or vice versa,
>>> hubs need to inform all its ports.
>>>
>>> There are just two places that set/clear receive_disabled,
>>> qemu_deliver_packet and qemu_flush_or_purge_queued_packets, but I
>>> think a new API is needed to implement the callback for hubs
>>> (qemu_send_enable/qemu_send_disable).
>>
>> I think .can_receive is the harder one, I'm not sure it's feasible - each
>> device has its own set of conditions, so it will be a huge change.
>
> The 1->0 transition is easy because it happens when message delivery
> fails.  I know the network code very little, but I think queuing exactly
> one packet in this case should be acceptable.  If this is true, the
> network code can detect the 1->0 transition automatically.

That's correct.  One packet is queued the first time
qemu_net_queue_send_iov() goes false.

> The 0->1 transition should be easy in principle, because NICs are
> supposed to call qemu_flush_queued_packets when it happens.  Not that
> they do, but you can find some very old and partial work in branch
> rx-flush of my github repo.

That sounds good.  Failure to call qemu_flush_queued_packets is
already a problem today so this change would benefit some of the
existing NICs.

Stefan

Re: [Qemu-devel] [PATCH v3 04/13] netmap: Drop netmap_can_send

[Fam Zheng]

On Tue, 05/19 15:54, Stefan Hajnoczi wrote:
> On Tue, May 19, 2015 at 10:51:01AM +0000, Fam Zheng wrote:
> > This callback is called by main loop before polling s->fd, if it returns
> > false, the fd will not be polled in this iteration.
> > 
> > This is redundant with checks inside read callback. After this patch,
> > the data will be copied from s->fd to s->iov when it arrives. If the
> > device can't receive, it will be queued to incoming_queue, and when the
> > device status changes, this queue will be flushed.
> > 
> > Also remove the qemu_can_send_packet() check in netmap_send. If it's
> > true, we are good; if it's false, the qemu_sendv_packet_async would
> > return 0 and read poll will be disabled until netmap_send_completed is
> > called.
> 
> This causes unbounded memory usage in QEMU because
> qemu_net_queue_append_iov() does not drop packets when sent_cb != NULL.

I think netmap_send will use "netmap_read_poll(s, false)" to stop reading, only
the first packet will be queued. Why is it unbounded?

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v3 03/13] l2tpv3: Drop l2tpv3_can_send

[Fam Zheng]

On Tue, 05/19 15:48, Stefan Hajnoczi wrote:
> On Tue, May 19, 2015 at 10:51:00AM +0000, Fam Zheng wrote:
> > This callback is called by main loop before polling s->fd, if it returns
> > false, the fd will not be polled in this iteration.
> > 
> > This is redundant with checks inside read callback. After this patch,
> > the data will be copied from s->fd to s->msgvec when it arrives. If the
> > device can't receive, it will be queued to incoming_queue, and when the
> > device status changes, this queue will be flushed.
> 
> This doesn't work because s->msgvec can fill up when
> qemu_can_send_packet() returns false.  At that point we burn 100% CPU
> because the file descriptor is still being monitored.

If qemu_can_send_packet returns false, we do stop monitoring the fd. In
net_l2tpv3_process_queue:

    size = qemu_send_packet_async(
            &s->nc,
            vec->iov_base,
            data_size,
            l2tpv3_send_completed
        );
    if (size == 0) {
        l2tpv3_read_poll(s, false);
    }

The packet is queued and size is 0, so the read poll will be disabled until
it's flushed.

What am I missing?

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v3 03/13] l2tpv3: Drop l2tpv3_can_send

[Stefan Hajnoczi]

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

On Tue, May 26, 2015 at 02:52:48PM +0800, Fam Zheng wrote:
> On Tue, 05/19 15:48, Stefan Hajnoczi wrote:
> > On Tue, May 19, 2015 at 10:51:00AM +0000, Fam Zheng wrote:
> > > This callback is called by main loop before polling s->fd, if it returns
> > > false, the fd will not be polled in this iteration.
> > > 
> > > This is redundant with checks inside read callback. After this patch,
> > > the data will be copied from s->fd to s->msgvec when it arrives. If the
> > > device can't receive, it will be queued to incoming_queue, and when the
> > > device status changes, this queue will be flushed.
> > 
> > This doesn't work because s->msgvec can fill up when
> > qemu_can_send_packet() returns false.  At that point we burn 100% CPU
> > because the file descriptor is still being monitored.
> 
> If qemu_can_send_packet returns false, we do stop monitoring the fd. In
> net_l2tpv3_process_queue:
> 
>     size = qemu_send_packet_async(
>             &s->nc,
>             vec->iov_base,
>             data_size,
>             l2tpv3_send_completed
>         );
>     if (size == 0) {
>         l2tpv3_read_poll(s, false);
>     }
> 
> The packet is queued and size is 0, so the read poll will be disabled until
> it's flushed.
> 
> What am I missing?

I think you are right.

I was looking at the while loop qemu_can_send_packet():

 } while (
          (s->queue_depth > 0) &&
           qemu_can_send_packet(&s->nc) &&
          ((size > 0) || bad_read)
      );

and missed the portion you quoted.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 04/13] netmap: Drop netmap_can_send

[Stefan Hajnoczi]

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

On Mon, May 25, 2015 at 11:51:23AM +0800, Fam Zheng wrote:
> On Tue, 05/19 15:54, Stefan Hajnoczi wrote:
> > On Tue, May 19, 2015 at 10:51:01AM +0000, Fam Zheng wrote:
> > > This callback is called by main loop before polling s->fd, if it returns
> > > false, the fd will not be polled in this iteration.
> > > 
> > > This is redundant with checks inside read callback. After this patch,
> > > the data will be copied from s->fd to s->iov when it arrives. If the
> > > device can't receive, it will be queued to incoming_queue, and when the
> > > device status changes, this queue will be flushed.
> > > 
> > > Also remove the qemu_can_send_packet() check in netmap_send. If it's
> > > true, we are good; if it's false, the qemu_sendv_packet_async would
> > > return 0 and read poll will be disabled until netmap_send_completed is
> > > called.
> > 
> > This causes unbounded memory usage in QEMU because
> > qemu_net_queue_append_iov() does not drop packets when sent_cb != NULL.
> 
> I think netmap_send will use "netmap_read_poll(s, false)" to stop reading, only
> the first packet will be queued. Why is it unbounded?

I looked again and I agree with you.  It should stop after the first
packet and resume when the peer flushes the queue.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 04/13] netmap: Drop netmap_can_send

[Fam Zheng]

On Tue, 05/26 10:18, Stefan Hajnoczi wrote:
> On Mon, May 25, 2015 at 11:51:23AM +0800, Fam Zheng wrote:
> > On Tue, 05/19 15:54, Stefan Hajnoczi wrote:
> > > On Tue, May 19, 2015 at 10:51:01AM +0000, Fam Zheng wrote:
> > > > This callback is called by main loop before polling s->fd, if it returns
> > > > false, the fd will not be polled in this iteration.
> > > > 
> > > > This is redundant with checks inside read callback. After this patch,
> > > > the data will be copied from s->fd to s->iov when it arrives. If the
> > > > device can't receive, it will be queued to incoming_queue, and when the
> > > > device status changes, this queue will be flushed.
> > > > 
> > > > Also remove the qemu_can_send_packet() check in netmap_send. If it's
> > > > true, we are good; if it's false, the qemu_sendv_packet_async would
> > > > return 0 and read poll will be disabled until netmap_send_completed is
> > > > called.
> > > 
> > > This causes unbounded memory usage in QEMU because
> > > qemu_net_queue_append_iov() does not drop packets when sent_cb != NULL.
> > 
> > I think netmap_send will use "netmap_read_poll(s, false)" to stop reading, only
> > the first packet will be queued. Why is it unbounded?
> 
> I looked again and I agree with you.  It should stop after the first
> packet and resume when the peer flushes the queue.
> 

The other patches (socket and tap) have the same rationale. Are you happy with
this appraoch?

Fam

Re: [Qemu-devel] [PATCH v3 06/13] tap: Drop tap_can_send

[Stefan Hajnoczi]

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

On Tue, May 19, 2015 at 10:51:03AM +0000, Fam Zheng wrote:
> -    while (qemu_can_send_packet(&s->nc)) {
> +    while (true) {
> +        bool can_send;
>          uint8_t *buf = s->buf;
>  
> +        can_send = qemu_can_send_packet(&s->nc);
> +
>          size = tap_read_packet(s->fd, s->buf, sizeof(s->buf));
>          if (size <= 0) {
>              break;
> @@ -204,8 +198,12 @@ static void tap_send(void *opaque)
>              size -= s->host_vnet_hdr_len;
>          }
>  
> +        /* If !can_send, we will want to disable the read poll, but we still
> +         * need the send completion callback to enable it again, which is a
> +         * sign of peer becoming ready.  So call the send function anyway.
> +         */
>          size = qemu_send_packet_async(&s->nc, buf, size, tap_send_completed);
> -        if (size == 0) {
> +        if (size == 0 || !can_send) {
>              tap_read_poll(s, false);
>              break;
>          } else if (size < 0) {

Why is can_send necessary here but not for the other netdevs that you
converted?  I thought relying on size == 0 is enough.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 04/13] netmap: Drop netmap_can_send

[Stefan Hajnoczi]

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

On Wed, May 27, 2015 at 03:24:38PM +0800, Fam Zheng wrote:
> On Tue, 05/26 10:18, Stefan Hajnoczi wrote:
> > On Mon, May 25, 2015 at 11:51:23AM +0800, Fam Zheng wrote:
> > > On Tue, 05/19 15:54, Stefan Hajnoczi wrote:
> > > > On Tue, May 19, 2015 at 10:51:01AM +0000, Fam Zheng wrote:
> > > > > This callback is called by main loop before polling s->fd, if it returns
> > > > > false, the fd will not be polled in this iteration.
> > > > > 
> > > > > This is redundant with checks inside read callback. After this patch,
> > > > > the data will be copied from s->fd to s->iov when it arrives. If the
> > > > > device can't receive, it will be queued to incoming_queue, and when the
> > > > > device status changes, this queue will be flushed.
> > > > > 
> > > > > Also remove the qemu_can_send_packet() check in netmap_send. If it's
> > > > > true, we are good; if it's false, the qemu_sendv_packet_async would
> > > > > return 0 and read poll will be disabled until netmap_send_completed is
> > > > > called.
> > > > 
> > > > This causes unbounded memory usage in QEMU because
> > > > qemu_net_queue_append_iov() does not drop packets when sent_cb != NULL.
> > > 
> > > I think netmap_send will use "netmap_read_poll(s, false)" to stop reading, only
> > > the first packet will be queued. Why is it unbounded?
> > 
> > I looked again and I agree with you.  It should stop after the first
> > packet and resume when the peer flushes the queue.
> > 
> 
> The other patches (socket and tap) have the same rationale. Are you happy with
> this appraoch?

Yes, but I have a question on the tap patch.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 06/13] tap: Drop tap_can_send

[Fam Zheng]

On Tue, 06/02 17:21, Stefan Hajnoczi wrote:
> On Tue, May 19, 2015 at 10:51:03AM +0000, Fam Zheng wrote:
> > -    while (qemu_can_send_packet(&s->nc)) {
> > +    while (true) {
> > +        bool can_send;
> >          uint8_t *buf = s->buf;
> >  
> > +        can_send = qemu_can_send_packet(&s->nc);
> > +
> >          size = tap_read_packet(s->fd, s->buf, sizeof(s->buf));
> >          if (size <= 0) {
> >              break;
> > @@ -204,8 +198,12 @@ static void tap_send(void *opaque)
> >              size -= s->host_vnet_hdr_len;
> >          }
> >  
> > +        /* If !can_send, we will want to disable the read poll, but we still
> > +         * need the send completion callback to enable it again, which is a
> > +         * sign of peer becoming ready.  So call the send function anyway.
> > +         */
> >          size = qemu_send_packet_async(&s->nc, buf, size, tap_send_completed);
> > -        if (size == 0) {
> > +        if (size == 0 || !can_send) {
> >              tap_read_poll(s, false);
> >              break;
> >          } else if (size < 0) {
> 
> Why is can_send necessary here but not for the other netdevs that you
> converted?  I thought relying on size == 0 is enough.

I think so. Jason pointed out this offline as well. I'll drop it.

Fam