Message ID | 0f9330ee99fb9d11639a98d5fb9c01625a15822e.1294743490.git.amit.shah@redhat.com |
---|---|
State | New |
Headers | show |
On 01/11/11 12:10, Amit Shah wrote: > Introduce a char-specific wrapper to qemu_set_fd_handler functions. > This wrapper is useful to add / remove a write handler easily. Write > handlers are only used when the backend is blocked and cannot receive > any more input. I'd suggest to add flags to enable/disable handlers to IOHandlerRecord instead. And helper functions to set/clear them of course. With that in place you also can move the handlers to a separate struct simliar to the new QemuChrHandlers struct from patch #1. cheers, Gerd
On (Tue) Jan 11 2011 [15:39:46], Gerd Hoffmann wrote: > On 01/11/11 12:10, Amit Shah wrote: > >Introduce a char-specific wrapper to qemu_set_fd_handler functions. > >This wrapper is useful to add / remove a write handler easily. Write > >handlers are only used when the backend is blocked and cannot receive > >any more input. > > I'd suggest to add flags to enable/disable handlers to > IOHandlerRecord instead. And helper functions to set/clear them of > course. > > With that in place you also can move the handlers to a separate > struct simliar to the new QemuChrHandlers struct from patch #1. I'm planning to do that later -- when more backends get involved, which have multiple fds (one for in, one for out). Are you OK with this for now (to solve the immediate bugs of guests freezing if host can't flush data) and doing this cleanup later as we progress? Amit
On 01/11/11 16:38, Amit Shah wrote: > On (Tue) Jan 11 2011 [15:39:46], Gerd Hoffmann wrote: >> On 01/11/11 12:10, Amit Shah wrote: >>> Introduce a char-specific wrapper to qemu_set_fd_handler functions. >>> This wrapper is useful to add / remove a write handler easily. Write >>> handlers are only used when the backend is blocked and cannot receive >>> any more input. >> >> I'd suggest to add flags to enable/disable handlers to >> IOHandlerRecord instead. And helper functions to set/clear them of >> course. >> >> With that in place you also can move the handlers to a separate >> struct simliar to the new QemuChrHandlers struct from patch #1. > > I'm planning to do that later -- when more backends get involved, which > have multiple fds (one for in, one for out). Moving the handlers to a separate struct is clearly a incremental cleanup which can follow later. Using enable/disable flags will probably simplify the interfaces for the non-blocking mode and thus simplify the whole patch series so I think this should be done now. cheers, Gerd
On (Tue) Jan 11 2011 [16:54:48], Gerd Hoffmann wrote: > On 01/11/11 16:38, Amit Shah wrote: > >On (Tue) Jan 11 2011 [15:39:46], Gerd Hoffmann wrote: > >>On 01/11/11 12:10, Amit Shah wrote: > >>>Introduce a char-specific wrapper to qemu_set_fd_handler functions. > >>>This wrapper is useful to add / remove a write handler easily. Write > >>>handlers are only used when the backend is blocked and cannot receive > >>>any more input. > >> > >>I'd suggest to add flags to enable/disable handlers to > >>IOHandlerRecord instead. And helper functions to set/clear them of > >>course. > >> > >>With that in place you also can move the handlers to a separate > >>struct simliar to the new QemuChrHandlers struct from patch #1. > > > >I'm planning to do that later -- when more backends get involved, which > >have multiple fds (one for in, one for out). > > Moving the handlers to a separate struct is clearly a incremental > cleanup which can follow later. Using enable/disable flags will > probably simplify the interfaces for the non-blocking mode and thus > simplify the whole patch series so I think this should be done now. Agree -- but it looks to be a big patch. I have some initial work done, and hence am not converting anything other than unix/tcp backends. The proposed interface here is local to this file, and just a couple of callers. No big deal to change it once this is in. (The struct for that will look like: struct fd_handler { int fd; IOHandler *read; IOHandler *write; IOCanReadHandler *read_poll; bool read_enabled, write_enabled, read_poll_enabled; void (*set_read_handler)(IOHandler *read_handler); void (*set_write_handler)(IOHandler *write_handler); void (*set_readpoll_handler)(IOCanReadHandler *read_poll_handler); } This has to be embedded in the CharDriverState for each fd for each backend. Also: we also want to be able to select() on all fds so that we can detect disconnection events as they happen. So we also need an array somewhere.) Amit
Hi, >> Moving the handlers to a separate struct is clearly a incremental >> cleanup which can follow later. Using enable/disable flags will >> probably simplify the interfaces for the non-blocking mode and thus >> simplify the whole patch series so I think this should be done now. > > Agree -- but it looks to be a big patch. I have some initial work done, > and hence am not converting anything other than unix/tcp backends. The > proposed interface here is local to this file, and just a couple of > callers. No big deal to change it once this is in. > > (The struct for that will look like: > > struct fd_handler { > int fd; > > IOHandler *read; > IOHandler *write; > IOCanReadHandler *read_poll; > > bool read_enabled, write_enabled, read_poll_enabled; > > void (*set_read_handler)(IOHandler *read_handler); > void (*set_write_handler)(IOHandler *write_handler); > void (*set_readpoll_handler)(IOCanReadHandler *read_poll_handler); > } No, that isn't what I have in mind ... I'm thinking more about this: (1) IOHandlerRecord gets variables to enable/disable the handlers (a bunch of bools, a bitmask, whatever). They default to enabled to maintain backward compatibility. (2) One or more functions are added to enable/disable the handlers, something like qemu_fd_check_read(int fd, bool enable); (3) main_loop_wait() will check whenever the handler is actually enabled before adding it to the file handle set for the select() call. This shouldn't be that big. And with this initial stuff in place you can go forward with the non-blocking stuff. No need to pass around the write handler or have callbacks just to enable/disable the checking for a writable fd, the non-blocking code can just call qemu_fd_check_write(fd, true/false) to do it. Additional cleanups possible: (1) switch everybody who registers/unregisters handlers to the new enable/disable interface. (2) move the function pointers from IOHandlerRecord to a separate struct (say IOHandlerOps). > Also: we also want to be able to select() on all fds so that we can > detect disconnection events as they happen. So we also need an array > somewhere.) I think you can't do that without switching from select() to poll(). cheers, Gerd
diff --git a/qemu-char.c b/qemu-char.c index d9a1df3..2420b6b 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -214,6 +214,24 @@ void qemu_chr_add_handlers(CharDriverState *s, } } +static int char_set_fd_handlers(int fd, + IOCanReadHandler *fd_read_poll, + IOHandler *fd_read, IOHandler *fd_write, + void *opaque, bool install_write_handler) +{ + if (install_write_handler) { + assert(fd_write); + } else { + fd_write = NULL; + } + return qemu_set_fd_handler2(fd, fd_read_poll, fd_read, fd_write, opaque); +} + +static int char_remove_fd_handlers(int fd) +{ + return qemu_set_fd_handler2(fd, NULL, NULL, NULL, NULL); +} + static int null_chr_write(CharDriverState *chr, const uint8_t *buf, int len) { return len; @@ -579,7 +597,7 @@ static void fd_chr_read(void *opaque) size = read(s->fd_in, buf, len); if (size == 0) { /* FD has been closed. Remove it from the active list. */ - qemu_set_fd_handler2(s->fd_in, NULL, NULL, NULL, NULL); + char_remove_fd_handlers(s->fd_in); qemu_chr_event(chr, CHR_EVENT_CLOSED); return; } @@ -595,8 +613,8 @@ static void fd_chr_update_read_handler(CharDriverState *chr) if (s->fd_in >= 0) { if (display_type == DT_NOGRAPHIC && s->fd_in == 0) { } else { - qemu_set_fd_handler2(s->fd_in, fd_chr_read_poll, - fd_chr_read, NULL, chr); + char_set_fd_handlers(s->fd_in, fd_chr_read_poll, fd_chr_read, + NULL, chr, false); } } } @@ -608,7 +626,7 @@ static void fd_chr_close(struct CharDriverState *chr) if (s->fd_in >= 0) { if (display_type == DT_NOGRAPHIC && s->fd_in == 0) { } else { - qemu_set_fd_handler2(s->fd_in, NULL, NULL, NULL, NULL); + char_remove_fd_handlers(s->fd_in); } } @@ -708,7 +726,7 @@ static void stdio_read(void *opaque) size = read(0, buf, 1); if (size == 0) { /* stdin has been closed. Remove it from the active list. */ - qemu_set_fd_handler2(0, NULL, NULL, NULL, NULL); + char_remove_fd_handlers(0); qemu_chr_event(chr, CHR_EVENT_CLOSED); return; } @@ -764,7 +782,7 @@ static void qemu_chr_close_stdio(struct CharDriverState *chr) { term_exit(); stdio_nb_clients--; - qemu_set_fd_handler2(0, NULL, NULL, NULL, NULL); + char_remove_fd_handlers(0); fd_chr_close(chr); } @@ -776,7 +794,7 @@ static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts) return NULL; chr = qemu_chr_open_fd(0, 1); chr->chr_close = qemu_chr_close_stdio; - qemu_set_fd_handler2(0, stdio_read_poll, stdio_read, NULL, chr); + char_set_fd_handlers(0, stdio_read_poll, stdio_read, NULL, chr, false); stdio_nb_clients++; term_init(opts); @@ -904,8 +922,8 @@ static void pty_chr_update_read_handler(CharDriverState *chr) { PtyCharDriver *s = chr->opaque; - qemu_set_fd_handler2(s->fd, pty_chr_read_poll, - pty_chr_read, NULL, chr); + char_set_fd_handlers(s->fd, pty_chr_read_poll, pty_chr_read, NULL, + chr, false); s->polling = 1; /* * Short timeout here: just need wait long enougth that qemu makes @@ -923,7 +941,7 @@ static void pty_chr_state(CharDriverState *chr, int connected) PtyCharDriver *s = chr->opaque; if (!connected) { - qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL); + char_remove_fd_handlers(s->fd); s->connected = 0; s->polling = 0; /* (re-)connect poll interval for idle guests: once per second. @@ -959,7 +977,7 @@ static void pty_chr_close(struct CharDriverState *chr) { PtyCharDriver *s = chr->opaque; - qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL); + char_remove_fd_handlers(s->fd); close(s->fd); qemu_del_timer(s->timer); qemu_free_timer(s->timer); @@ -1860,8 +1878,8 @@ static void udp_chr_update_read_handler(CharDriverState *chr) NetCharDriver *s = chr->opaque; if (s->fd >= 0) { - qemu_set_fd_handler2(s->fd, udp_chr_read_poll, - udp_chr_read, NULL, chr); + char_set_fd_handlers(s->fd, udp_chr_read_poll, udp_chr_read, NULL, + chr, false); } } @@ -1869,7 +1887,7 @@ static void udp_chr_close(CharDriverState *chr) { NetCharDriver *s = chr->opaque; if (s->fd >= 0) { - qemu_set_fd_handler(s->fd, NULL, NULL, NULL); + char_remove_fd_handlers(s->fd); closesocket(s->fd); } qemu_free(s); @@ -2078,9 +2096,10 @@ static void tcp_chr_read(void *opaque) /* connection closed */ s->connected = 0; if (s->listen_fd >= 0) { - qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr); + char_set_fd_handlers(s->listen_fd, NULL, tcp_chr_accept, NULL, + chr, false); } - qemu_set_fd_handler(s->fd, NULL, NULL, NULL); + char_remove_fd_handlers(s->fd); closesocket(s->fd); s->fd = -1; qemu_chr_event(chr, CHR_EVENT_CLOSED); @@ -2105,8 +2124,8 @@ static void tcp_chr_connect(void *opaque) TCPCharDriver *s = chr->opaque; s->connected = 1; - qemu_set_fd_handler2(s->fd, tcp_chr_read_poll, - tcp_chr_read, NULL, chr); + char_set_fd_handlers(s->fd, tcp_chr_read_poll, tcp_chr_read, NULL, + chr, false); qemu_chr_generic_open(chr); } @@ -2167,7 +2186,7 @@ static void tcp_chr_accept(void *opaque) if (s->do_nodelay) socket_set_nodelay(fd); s->fd = fd; - qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL); + char_remove_fd_handlers(s->listen_fd); tcp_chr_connect(chr); } @@ -2175,11 +2194,11 @@ static void tcp_chr_close(CharDriverState *chr) { TCPCharDriver *s = chr->opaque; if (s->fd >= 0) { - qemu_set_fd_handler(s->fd, NULL, NULL, NULL); + char_remove_fd_handlers(s->fd); closesocket(s->fd); } if (s->listen_fd >= 0) { - qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL); + char_remove_fd_handlers(s->listen_fd); closesocket(s->listen_fd); } qemu_free(s); @@ -2241,7 +2260,8 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) if (is_listen) { s->listen_fd = fd; - qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr); + char_set_fd_handlers(s->listen_fd, NULL, tcp_chr_accept, NULL, + chr, false); if (is_telnet) s->do_telnetopt = 1;
Introduce a char-specific wrapper to qemu_set_fd_handler functions. This wrapper is useful to add / remove a write handler easily. Write handlers are only used when the backend is blocked and cannot receive any more input. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- qemu-char.c | 64 ++++++++++++++++++++++++++++++++++++++-------------------- 1 files changed, 42 insertions(+), 22 deletions(-)