* [Qemu-devel] [PATCH v2] chardev/char-socket: add POLLHUP handler @ 2018-01-18 14:33 Klim Kireev 2018-01-18 15:49 ` Marc-André Lureau 0 siblings, 1 reply; 4+ messages in thread From: Klim Kireev @ 2018-01-18 14:33 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, den, marcandre.lureau The following behavior was observed for QEMU configured by libvirt to use guest agent as usual for the guests without virtio-serial driver (Windows or the guest remaining in BIOS stage). In QEMU on first connect to listen character device socket the listen socket is removed from poll just after the accept(). virtio_serial_guest_ready() returns 0 and the descriptor of the connected Unix socket is removed from poll and it will not be present in poll() until the guest will initialize the driver and change the state of the serial to "guest connected". In libvirt connect() to guest agent is performed on restart and is run under VM state lock. Connect() is blocking and can wait forever. In this case libvirt can not perform ANY operation on that VM. The bug can be easily reproduced this way: Terminal 1: qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev socket,id=serial1,path=/tmp/console.sock,server,nowait (virtio-serial and isa-serial also fit) Terminal 2: minicom -D unix\#/tmp/console.sock (type something and press enter) C-a x (to exit) Do 3 times: minicom -D unix\#/tmp/console.sock C-a x It needs 4 connections, because the first one is accepted by QEMU, then two are queued by the kernel, and the 4th blocks. The problem is that QEMU doesn't add a read watcher after succesful read until the guest device wants to acquire recieved data, so I propose to install a separate pullhup watcher regardless of whether the device waits for data or not. Signed-off-by: Klim Kireev <klim.kireev@virtuozzo.com> --- Changelog: v2: Remove timer as a redundant feature chardev/char-socket.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 77cdf487eb..d3fe903ab6 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -42,6 +42,7 @@ typedef struct { QIOChannel *ioc; /* Client I/O channel */ QIOChannelSocket *sioc; /* Client master channel */ QIONetListener *listener; + guint hup_tag; QCryptoTLSCreds *tls_creds; int connected; int max_size; @@ -352,6 +353,11 @@ static void tcp_chr_free_connection(Chardev *chr) s->read_msgfds_num = 0; } + if (s->hup_tag != 0) { + g_source_remove(s->hup_tag); + s->hup_tag = 0; + } + tcp_set_msgfds(chr, NULL, 0); remove_fd_in_watch(chr); object_unref(OBJECT(s->sioc)); @@ -455,6 +461,19 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque) return TRUE; } +static gboolean tcp_chr_hup(QIOChannel *channel, + GIOCondition cond, + void *opaque) +{ + Chardev *chr = CHARDEV(opaque); + SocketChardev *s = SOCKET_CHARDEV(chr); + tcp_chr_read(channel, cond, opaque); + if (s->connected != 0) { + tcp_chr_disconnect(chr); + } + return TRUE; +} + static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len) { SocketChardev *s = SOCKET_CHARDEV(chr); @@ -528,6 +547,10 @@ static void tcp_chr_connect(void *opaque) tcp_chr_read, chr, chr->gcontext); } + if (s->hup_tag == 0) { + s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP, + tcp_chr_hup, chr, NULL); + } qemu_chr_be_event(chr, CHR_EVENT_OPENED); } @@ -546,7 +569,11 @@ static void tcp_chr_update_read_handler(Chardev *chr) tcp_chr_read, chr, chr->gcontext); } -} + if (s->hup_tag == 0) { + s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP, + tcp_chr_hup, chr, NULL); + } + } typedef struct { Chardev *chr; -- 2.14.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] chardev/char-socket: add POLLHUP handler 2018-01-18 14:33 [Qemu-devel] [PATCH v2] chardev/char-socket: add POLLHUP handler Klim Kireev @ 2018-01-18 15:49 ` Marc-André Lureau 2018-01-18 16:18 ` klim 0 siblings, 1 reply; 4+ messages in thread From: Marc-André Lureau @ 2018-01-18 15:49 UTC (permalink / raw) To: Klim Kireev; +Cc: QEMU, Paolo Bonzini, den Hi On Thu, Jan 18, 2018 at 3:33 PM, Klim Kireev <klim.kireev@virtuozzo.com> wrote: > The following behavior was observed for QEMU configured by libvirt > to use guest agent as usual for the guests without virtio-serial > driver (Windows or the guest remaining in BIOS stage). > > In QEMU on first connect to listen character device socket > the listen socket is removed from poll just after the accept(). > virtio_serial_guest_ready() returns 0 and the descriptor > of the connected Unix socket is removed from poll and it will > not be present in poll() until the guest will initialize the driver > and change the state of the serial to "guest connected". > > In libvirt connect() to guest agent is performed on restart and > is run under VM state lock. Connect() is blocking and can > wait forever. > In this case libvirt can not perform ANY operation on that VM. > > The bug can be easily reproduced this way: > > Terminal 1: > qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev socket,id=serial1,path=/tmp/console.sock,server,nowait > (virtio-serial and isa-serial also fit) > > Terminal 2: > minicom -D unix\#/tmp/console.sock > (type something and press enter) > C-a x (to exit) > > Do 3 times: > minicom -D unix\#/tmp/console.sock > C-a x > > It needs 4 connections, because the first one is accepted by QEMU, then two are queued by > the kernel, and the 4th blocks. > > The problem is that QEMU doesn't add a read watcher after succesful read > until the guest device wants to acquire recieved data, so > I propose to install a separate pullhup watcher regardless of > whether the device waits for data or not. > > Signed-off-by: Klim Kireev <klim.kireev@virtuozzo.com> > --- > Changelog: > v2: Remove timer as a redundant feature > > chardev/char-socket.c | 29 ++++++++++++++++++++++++++++- > 1 file changed, 28 insertions(+), 1 deletion(-) > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > index 77cdf487eb..d3fe903ab6 100644 > --- a/chardev/char-socket.c > +++ b/chardev/char-socket.c > @@ -42,6 +42,7 @@ typedef struct { > QIOChannel *ioc; /* Client I/O channel */ > QIOChannelSocket *sioc; /* Client master channel */ > QIONetListener *listener; > + guint hup_tag; > QCryptoTLSCreds *tls_creds; > int connected; > int max_size; > @@ -352,6 +353,11 @@ static void tcp_chr_free_connection(Chardev *chr) > s->read_msgfds_num = 0; > } > > + if (s->hup_tag != 0) { > + g_source_remove(s->hup_tag); > + s->hup_tag = 0; > + } > + > tcp_set_msgfds(chr, NULL, 0); > remove_fd_in_watch(chr); > object_unref(OBJECT(s->sioc)); > @@ -455,6 +461,19 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque) > return TRUE; > } > > +static gboolean tcp_chr_hup(QIOChannel *channel, > + GIOCondition cond, > + void *opaque) > +{ > + Chardev *chr = CHARDEV(opaque); > + SocketChardev *s = SOCKET_CHARDEV(chr); > + tcp_chr_read(channel, cond, opaque); > + if (s->connected != 0) { tcp_chr_read() shouldn't be called unless frontend is ready to read. qemu_chr_be_can_write() is regularly updated with tcp_chr_read_poll() but this may create some race here (if it read all it could read previously for example) If frontend can't read, s->connected won't be updated, so you'll busy loop in the source callback, not good. I think it needs further rework of how s->connected is updated. Why call tcp_chr_read() if you received HUP event ? could it call tcp_chr_free_connection()? > + tcp_chr_disconnect(chr); > + } > + return TRUE; please use G_SOURCE_CONTINUE/REMOVE (I know it's not being used widely, but we have define now, and it is much clearer) > +} > + > static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len) > { > SocketChardev *s = SOCKET_CHARDEV(chr); > @@ -528,6 +547,10 @@ static void tcp_chr_connect(void *opaque) > tcp_chr_read, > chr, chr->gcontext); > } > + if (s->hup_tag == 0) { > + s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP, > + tcp_chr_hup, chr, NULL); > + } > qemu_chr_be_event(chr, CHR_EVENT_OPENED); > } > > @@ -546,7 +569,11 @@ static void tcp_chr_update_read_handler(Chardev *chr) > tcp_chr_read, chr, > chr->gcontext); > } > -} > + if (s->hup_tag == 0) { > + s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP, > + tcp_chr_hup, chr, NULL); > + } > + } > > typedef struct { > Chardev *chr; > -- > 2.14.3 > > -- Marc-André Lureau ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] chardev/char-socket: add POLLHUP handler 2018-01-18 15:49 ` Marc-André Lureau @ 2018-01-18 16:18 ` klim 2018-01-18 16:29 ` Marc-André Lureau 0 siblings, 1 reply; 4+ messages in thread From: klim @ 2018-01-18 16:18 UTC (permalink / raw) To: Marc-André Lureau; +Cc: QEMU, Paolo Bonzini, den On 01/18/2018 06:49 PM, Marc-André Lureau wrote: > Hi > > On Thu, Jan 18, 2018 at 3:33 PM, Klim Kireev <klim.kireev@virtuozzo.com> wrote: >> The following behavior was observed for QEMU configured by libvirt >> to use guest agent as usual for the guests without virtio-serial >> driver (Windows or the guest remaining in BIOS stage). >> >> In QEMU on first connect to listen character device socket >> the listen socket is removed from poll just after the accept(). >> virtio_serial_guest_ready() returns 0 and the descriptor >> of the connected Unix socket is removed from poll and it will >> not be present in poll() until the guest will initialize the driver >> and change the state of the serial to "guest connected". >> >> In libvirt connect() to guest agent is performed on restart and >> is run under VM state lock. Connect() is blocking and can >> wait forever. >> In this case libvirt can not perform ANY operation on that VM. >> >> The bug can be easily reproduced this way: >> >> Terminal 1: >> qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev socket,id=serial1,path=/tmp/console.sock,server,nowait >> (virtio-serial and isa-serial also fit) >> >> Terminal 2: >> minicom -D unix\#/tmp/console.sock >> (type something and press enter) >> C-a x (to exit) >> >> Do 3 times: >> minicom -D unix\#/tmp/console.sock >> C-a x >> >> It needs 4 connections, because the first one is accepted by QEMU, then two are queued by >> the kernel, and the 4th blocks. >> >> The problem is that QEMU doesn't add a read watcher after succesful read >> until the guest device wants to acquire recieved data, so >> I propose to install a separate pullhup watcher regardless of >> whether the device waits for data or not. >> >> Signed-off-by: Klim Kireev <klim.kireev@virtuozzo.com> >> --- >> Changelog: >> v2: Remove timer as a redundant feature >> >> chardev/char-socket.c | 29 ++++++++++++++++++++++++++++- >> 1 file changed, 28 insertions(+), 1 deletion(-) >> >> diff --git a/chardev/char-socket.c b/chardev/char-socket.c >> index 77cdf487eb..d3fe903ab6 100644 >> --- a/chardev/char-socket.c >> +++ b/chardev/char-socket.c >> @@ -42,6 +42,7 @@ typedef struct { >> QIOChannel *ioc; /* Client I/O channel */ >> QIOChannelSocket *sioc; /* Client master channel */ >> QIONetListener *listener; >> + guint hup_tag; >> QCryptoTLSCreds *tls_creds; >> int connected; >> int max_size; >> @@ -352,6 +353,11 @@ static void tcp_chr_free_connection(Chardev *chr) >> s->read_msgfds_num = 0; >> } >> >> + if (s->hup_tag != 0) { >> + g_source_remove(s->hup_tag); >> + s->hup_tag = 0; >> + } >> + >> tcp_set_msgfds(chr, NULL, 0); >> remove_fd_in_watch(chr); >> object_unref(OBJECT(s->sioc)); >> @@ -455,6 +461,19 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque) >> return TRUE; >> } >> >> +static gboolean tcp_chr_hup(QIOChannel *channel, >> + GIOCondition cond, >> + void *opaque) >> +{ >> + Chardev *chr = CHARDEV(opaque); >> + SocketChardev *s = SOCKET_CHARDEV(chr); >> + tcp_chr_read(channel, cond, opaque); >> + if (s->connected != 0) { > tcp_chr_read() shouldn't be called unless frontend is ready to read. > qemu_chr_be_can_write() is regularly updated with tcp_chr_read_poll() > but this may create some race here (if it read all it could read > previously for example) > > If frontend can't read, s->connected won't be updated, so you'll busy > loop in the source callback, not good. > > I think it needs further rework of how s->connected is updated. > > Why call tcp_chr_read() if you received HUP event ? could it call > tcp_chr_free_connection()? The reason is that: if client sends data and closes the socket between two ppoll(), POLLHUP handler is called and data in channel is lost, so read is used to pass it to guest. if there is no data in channel, tcp_chr_recv() returns 0 and tcp_chr_read() calls tcp_chr_disconnect() which calls tcp_chr_free_connection(). If there is some data in channel it calls qemu_chr_be_write() and then in tcp_chr_disconnect() tcp_free_connection() will be called. In any case connection will be closed, so where is busy loop? >> + tcp_chr_disconnect(chr); >> + } >> + return TRUE; > please use G_SOURCE_CONTINUE/REMOVE (I know it's not being used > widely, but we have define now, and it is much clearer) > >> +} >> + >> static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len) >> { >> SocketChardev *s = SOCKET_CHARDEV(chr); >> @@ -528,6 +547,10 @@ static void tcp_chr_connect(void *opaque) >> tcp_chr_read, >> chr, chr->gcontext); >> } >> + if (s->hup_tag == 0) { >> + s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP, >> + tcp_chr_hup, chr, NULL); >> + } >> qemu_chr_be_event(chr, CHR_EVENT_OPENED); >> } >> >> @@ -546,7 +569,11 @@ static void tcp_chr_update_read_handler(Chardev *chr) >> tcp_chr_read, chr, >> chr->gcontext); >> } >> -} >> + if (s->hup_tag == 0) { >> + s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP, >> + tcp_chr_hup, chr, NULL); >> + } >> + } >> >> typedef struct { >> Chardev *chr; >> -- >> 2.14.3 >> >> > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] chardev/char-socket: add POLLHUP handler 2018-01-18 16:18 ` klim @ 2018-01-18 16:29 ` Marc-André Lureau 0 siblings, 0 replies; 4+ messages in thread From: Marc-André Lureau @ 2018-01-18 16:29 UTC (permalink / raw) To: klim; +Cc: QEMU, Paolo Bonzini, den Hi On Thu, Jan 18, 2018 at 5:18 PM, klim <klim.kireev@virtuozzo.com> wrote: > On 01/18/2018 06:49 PM, Marc-André Lureau wrote: >> >> Hi >> >> On Thu, Jan 18, 2018 at 3:33 PM, Klim Kireev <klim.kireev@virtuozzo.com> >> wrote: >>> >>> The following behavior was observed for QEMU configured by libvirt >>> to use guest agent as usual for the guests without virtio-serial >>> driver (Windows or the guest remaining in BIOS stage). >>> >>> In QEMU on first connect to listen character device socket >>> the listen socket is removed from poll just after the accept(). >>> virtio_serial_guest_ready() returns 0 and the descriptor >>> of the connected Unix socket is removed from poll and it will >>> not be present in poll() until the guest will initialize the driver >>> and change the state of the serial to "guest connected". >>> >>> In libvirt connect() to guest agent is performed on restart and >>> is run under VM state lock. Connect() is blocking and can >>> wait forever. >>> In this case libvirt can not perform ANY operation on that VM. >>> >>> The bug can be easily reproduced this way: >>> >>> Terminal 1: >>> qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev >>> socket,id=serial1,path=/tmp/console.sock,server,nowait >>> (virtio-serial and isa-serial also fit) >>> >>> Terminal 2: >>> minicom -D unix\#/tmp/console.sock >>> (type something and press enter) >>> C-a x (to exit) >>> >>> Do 3 times: >>> minicom -D unix\#/tmp/console.sock >>> C-a x >>> >>> It needs 4 connections, because the first one is accepted by QEMU, then >>> two are queued by >>> the kernel, and the 4th blocks. >>> >>> The problem is that QEMU doesn't add a read watcher after succesful read >>> until the guest device wants to acquire recieved data, so >>> I propose to install a separate pullhup watcher regardless of >>> whether the device waits for data or not. >>> >>> Signed-off-by: Klim Kireev <klim.kireev@virtuozzo.com> >>> --- >>> Changelog: >>> v2: Remove timer as a redundant feature >>> >>> chardev/char-socket.c | 29 ++++++++++++++++++++++++++++- >>> 1 file changed, 28 insertions(+), 1 deletion(-) >>> >>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c >>> index 77cdf487eb..d3fe903ab6 100644 >>> --- a/chardev/char-socket.c >>> +++ b/chardev/char-socket.c >>> @@ -42,6 +42,7 @@ typedef struct { >>> QIOChannel *ioc; /* Client I/O channel */ >>> QIOChannelSocket *sioc; /* Client master channel */ >>> QIONetListener *listener; >>> + guint hup_tag; >>> QCryptoTLSCreds *tls_creds; >>> int connected; >>> int max_size; >>> @@ -352,6 +353,11 @@ static void tcp_chr_free_connection(Chardev *chr) >>> s->read_msgfds_num = 0; >>> } >>> >>> + if (s->hup_tag != 0) { >>> + g_source_remove(s->hup_tag); >>> + s->hup_tag = 0; >>> + } >>> + >>> tcp_set_msgfds(chr, NULL, 0); >>> remove_fd_in_watch(chr); >>> object_unref(OBJECT(s->sioc)); >>> @@ -455,6 +461,19 @@ static gboolean tcp_chr_read(QIOChannel *chan, >>> GIOCondition cond, void *opaque) >>> return TRUE; >>> } >>> >>> +static gboolean tcp_chr_hup(QIOChannel *channel, >>> + GIOCondition cond, >>> + void *opaque) >>> +{ >>> + Chardev *chr = CHARDEV(opaque); >>> + SocketChardev *s = SOCKET_CHARDEV(chr); >>> + tcp_chr_read(channel, cond, opaque); >>> + if (s->connected != 0) { >> >> tcp_chr_read() shouldn't be called unless frontend is ready to read. >> qemu_chr_be_can_write() is regularly updated with tcp_chr_read_poll() >> but this may create some race here (if it read all it could read >> previously for example) >> >> If frontend can't read, s->connected won't be updated, so you'll busy >> loop in the source callback, not good. >> >> I think it needs further rework of how s->connected is updated. >> >> Why call tcp_chr_read() if you received HUP event ? could it call >> tcp_chr_free_connection()? > > > The reason is that: > > if client sends data and closes the socket between two ppoll(), POLLHUP > handler is called > and data in channel is lost, so read is used to pass it to guest. I thought we agree we don't need to handle reliability of client data if it disconnects. > if there is no data in channel, tcp_chr_recv() returns 0 > and tcp_chr_read() calls tcp_chr_disconnect() which calls > tcp_chr_free_connection(). tcp_chr_read() won't try to read if max_size = 0 (set when !qemu_chr_be_can_write) This is where it will busy loop I believe > If there is some data in channel it calls qemu_chr_be_write() and then in > tcp_chr_disconnect() > tcp_free_connection() will be called. > In any case connection will be closed, so where is busy loop? > >>> + tcp_chr_disconnect(chr); >>> + } >>> + return TRUE; >> >> please use G_SOURCE_CONTINUE/REMOVE (I know it's not being used >> widely, but we have define now, and it is much clearer) >> >>> +} >>> + >>> static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len) >>> { >>> SocketChardev *s = SOCKET_CHARDEV(chr); >>> @@ -528,6 +547,10 @@ static void tcp_chr_connect(void *opaque) >>> tcp_chr_read, >>> chr, chr->gcontext); >>> } >>> + if (s->hup_tag == 0) { >>> + s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP, >>> + tcp_chr_hup, chr, NULL); >>> + } >>> qemu_chr_be_event(chr, CHR_EVENT_OPENED); >>> } >>> >>> @@ -546,7 +569,11 @@ static void tcp_chr_update_read_handler(Chardev >>> *chr) >>> tcp_chr_read, chr, >>> chr->gcontext); >>> } >>> -} >>> + if (s->hup_tag == 0) { >>> + s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP, >>> + tcp_chr_hup, chr, NULL); >>> + } >>> + } >>> >>> typedef struct { >>> Chardev *chr; >>> -- >>> 2.14.3 >>> >>> >> >> > -- Marc-André Lureau ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-01-18 16:29 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-01-18 14:33 [Qemu-devel] [PATCH v2] chardev/char-socket: add POLLHUP handler Klim Kireev 2018-01-18 15:49 ` Marc-André Lureau 2018-01-18 16:18 ` klim 2018-01-18 16:29 ` Marc-André Lureau
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).