| Message ID | 20190115145256.9593-10-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | chardev: refactoring & many bugfixes related tcp_chr_wait_connected | expand |
Hi On Tue, Jan 15, 2019 at 6:54 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > The socket connection state is indicated via the 'bool connected' field > in the SocketChardev struct. This variable is somewhat misleading > though, as it is only set to true once the connection has completed all > required handshakes (eg for TLS, telnet or websockets). IOW there is a > period of time in which the socket is connected, but the "connected" > flag is still false. > > The socket chardev really has three states that it can be in, > disconnected, connecting and connected and those should be tracked > explicitly. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> You could split that patch, to introduce CONNECTING in a separate step. But lgtm so, Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > chardev/char-socket.c | 63 +++++++++++++++++++++++++++++++++---------- > 1 file changed, 49 insertions(+), 14 deletions(-) > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > index 385504b021..96a60eb105 100644 > --- a/chardev/char-socket.c > +++ b/chardev/char-socket.c > @@ -46,6 +46,12 @@ typedef struct { > size_t buflen; > } TCPChardevTelnetInit; > > +typedef enum { > + TCP_CHARDEV_STATE_DISCONNECTED, > + TCP_CHARDEV_STATE_CONNECTING, > + TCP_CHARDEV_STATE_CONNECTED, > +} TCPChardevState; > + > typedef struct { > Chardev parent; > QIOChannel *ioc; /* Client I/O channel */ > @@ -53,7 +59,7 @@ typedef struct { > QIONetListener *listener; > GSource *hup_source; > QCryptoTLSCreds *tls_creds; > - int connected; > + TCPChardevState state; > int max_size; > int do_telnetopt; > int do_nodelay; > @@ -82,6 +88,21 @@ typedef struct { > static gboolean socket_reconnect_timeout(gpointer opaque); > static void tcp_chr_telnet_init(Chardev *chr); > > +static void tcp_chr_change_state(SocketChardev *s, TCPChardevState state) > +{ > + switch (state) { > + case TCP_CHARDEV_STATE_DISCONNECTED: > + break; > + case TCP_CHARDEV_STATE_CONNECTING: > + assert(s->state == TCP_CHARDEV_STATE_DISCONNECTED); > + break; > + case TCP_CHARDEV_STATE_CONNECTED: > + assert(s->state == TCP_CHARDEV_STATE_CONNECTING); > + break; > + } > + s->state = state; > +} > + > static void tcp_chr_reconn_timer_cancel(SocketChardev *s) > { > if (s->reconnect_timer) { > @@ -96,7 +117,7 @@ static void qemu_chr_socket_restart_timer(Chardev *chr) > SocketChardev *s = SOCKET_CHARDEV(chr); > char *name; > > - assert(s->connected == 0); > + assert(s->state == TCP_CHARDEV_STATE_DISCONNECTED); > name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label); > s->reconnect_timer = qemu_chr_timeout_add_ms(chr, > s->reconnect_time * 1000, > @@ -131,7 +152,7 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len) > { > SocketChardev *s = SOCKET_CHARDEV(chr); > > - if (s->connected) { > + if (s->state == TCP_CHARDEV_STATE_CONNECTED) { > int ret = io_channel_send_full(s->ioc, buf, len, > s->write_msgfds, > s->write_msgfds_num); > @@ -164,7 +185,7 @@ static int tcp_chr_read_poll(void *opaque) > { > Chardev *chr = CHARDEV(opaque); > SocketChardev *s = SOCKET_CHARDEV(opaque); > - if (!s->connected) { > + if (s->state != TCP_CHARDEV_STATE_CONNECTED) { > return 0; > } > s->max_size = qemu_chr_be_can_write(chr); > @@ -277,7 +298,7 @@ static int tcp_set_msgfds(Chardev *chr, int *fds, int num) > s->write_msgfds = NULL; > s->write_msgfds_num = 0; > > - if (!s->connected || > + if ((s->state != TCP_CHARDEV_STATE_CONNECTED) || > !qio_channel_has_feature(s->ioc, > QIO_CHANNEL_FEATURE_FD_PASS)) { > return -1; > @@ -389,7 +410,7 @@ static void tcp_chr_free_connection(Chardev *chr) > s->ioc = NULL; > g_free(chr->filename); > chr->filename = NULL; > - s->connected = 0; > + tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED); > } > > static const char *qemu_chr_socket_protocol(SocketChardev *s) > @@ -442,12 +463,12 @@ static void update_disconnected_filename(SocketChardev *s) > > /* NB may be called even if tcp_chr_connect has not been > * reached, due to TLS or telnet initialization failure, > - * so can *not* assume s->connected == true > + * so can *not* assume s->state == TCP_CHARDEV_STATE_CONNECTED > */ > static void tcp_chr_disconnect(Chardev *chr) > { > SocketChardev *s = SOCKET_CHARDEV(chr); > - bool emit_close = s->connected; > + bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED; > > tcp_chr_free_connection(chr); > > @@ -471,7 +492,8 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque) > uint8_t buf[CHR_READ_BUF_LEN]; > int len, size; > > - if (!s->connected || s->max_size <= 0) { > + if ((s->state != TCP_CHARDEV_STATE_CONNECTED) || > + s->max_size <= 0) { > return TRUE; > } > len = sizeof(buf); > @@ -508,7 +530,7 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len) > SocketChardev *s = SOCKET_CHARDEV(chr); > int size; > > - if (!s->connected) { > + if (s->state != TCP_CHARDEV_STATE_CONNECTED) { > return 0; > } > > @@ -564,7 +586,7 @@ static void update_ioc_handlers(SocketChardev *s) > { > Chardev *chr = CHARDEV(s); > > - if (!s->connected) { > + if (s->state != TCP_CHARDEV_STATE_CONNECTED) { > return; > } > > @@ -589,7 +611,7 @@ static void tcp_chr_connect(void *opaque) > g_free(chr->filename); > chr->filename = qemu_chr_compute_filename(s); > > - s->connected = 1; > + tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTED); > update_ioc_handlers(s); > qemu_chr_be_event(chr, CHR_EVENT_OPENED); > } > @@ -828,7 +850,7 @@ static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc) > { > SocketChardev *s = SOCKET_CHARDEV(chr); > > - if (s->ioc != NULL) { > + if (s->state != TCP_CHARDEV_STATE_CONNECTING) { > return -1; > } > > @@ -865,11 +887,17 @@ static int tcp_chr_add_client(Chardev *chr, int fd) > { > int ret; > QIOChannelSocket *sioc; > + SocketChardev *s = SOCKET_CHARDEV(chr); > + > + if (s->state != TCP_CHARDEV_STATE_DISCONNECTED) { > + return -1; > + } > > sioc = qio_channel_socket_new_fd(fd, NULL); > if (!sioc) { > return -1; > } > + tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING); > tcp_chr_set_client_ioc_name(chr, sioc); > ret = tcp_chr_new_client(chr, sioc); > object_unref(OBJECT(sioc)); > @@ -881,7 +909,9 @@ static void tcp_chr_accept(QIONetListener *listener, > void *opaque) > { > Chardev *chr = CHARDEV(opaque); > + SocketChardev *s = SOCKET_CHARDEV(chr); > > + tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING); > tcp_chr_set_client_ioc_name(chr, cioc); > tcp_chr_new_client(chr, cioc); > } > @@ -891,8 +921,10 @@ static int tcp_chr_connect_client_sync(Chardev *chr, Error **errp) > { > SocketChardev *s = SOCKET_CHARDEV(chr); > QIOChannelSocket *sioc = qio_channel_socket_new(); > + tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING); > tcp_chr_set_client_ioc_name(chr, sioc); > if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) { > + tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED); > object_unref(OBJECT(sioc)); > return -1; > } > @@ -908,6 +940,7 @@ static void tcp_chr_accept_server_sync(Chardev *chr) > QIOChannelSocket *sioc; > info_report("QEMU waiting for connection on: %s", > chr->filename); > + tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING); > sioc = qio_net_listener_wait_client(s->listener); > tcp_chr_set_client_ioc_name(chr, sioc); > tcp_chr_new_client(chr, sioc); > @@ -963,6 +996,7 @@ static void qemu_chr_socket_connected(QIOTask *task, void *opaque) > Error *err = NULL; > > if (qio_task_propagate_error(task, &err)) { > + tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED); > check_report_connect_error(chr, err); > error_free(err); > goto cleanup; > @@ -980,6 +1014,7 @@ static void tcp_chr_connect_client_async(Chardev *chr) > SocketChardev *s = SOCKET_CHARDEV(chr); > QIOChannelSocket *sioc; > > + tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING); > sioc = qio_channel_socket_new(); > tcp_chr_set_client_ioc_name(chr, sioc); > qio_channel_socket_connect_async(sioc, s->addr, > @@ -1307,7 +1342,7 @@ char_socket_get_connected(Object *obj, Error **errp) > { > SocketChardev *s = SOCKET_CHARDEV(obj); > > - return s->connected; > + return s->state == TCP_CHARDEV_STATE_CONNECTED; > } > > static void char_socket_class_init(ObjectClass *oc, void *data) > -- > 2.20.1 >
diff --git a/chardev/char-socket.c b/chardev/char-socket.c index 385504b021..96a60eb105 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -46,6 +46,12 @@ typedef struct { size_t buflen; } TCPChardevTelnetInit; +typedef enum { + TCP_CHARDEV_STATE_DISCONNECTED, + TCP_CHARDEV_STATE_CONNECTING, + TCP_CHARDEV_STATE_CONNECTED, +} TCPChardevState; + typedef struct { Chardev parent; QIOChannel *ioc; /* Client I/O channel */ @@ -53,7 +59,7 @@ typedef struct { QIONetListener *listener; GSource *hup_source; QCryptoTLSCreds *tls_creds; - int connected; + TCPChardevState state; int max_size; int do_telnetopt; int do_nodelay; @@ -82,6 +88,21 @@ typedef struct { static gboolean socket_reconnect_timeout(gpointer opaque); static void tcp_chr_telnet_init(Chardev *chr); +static void tcp_chr_change_state(SocketChardev *s, TCPChardevState state) +{ + switch (state) { + case TCP_CHARDEV_STATE_DISCONNECTED: + break; + case TCP_CHARDEV_STATE_CONNECTING: + assert(s->state == TCP_CHARDEV_STATE_DISCONNECTED); + break; + case TCP_CHARDEV_STATE_CONNECTED: + assert(s->state == TCP_CHARDEV_STATE_CONNECTING); + break; + } + s->state = state; +} + static void tcp_chr_reconn_timer_cancel(SocketChardev *s) { if (s->reconnect_timer) { @@ -96,7 +117,7 @@ static void qemu_chr_socket_restart_timer(Chardev *chr) SocketChardev *s = SOCKET_CHARDEV(chr); char *name; - assert(s->connected == 0); + assert(s->state == TCP_CHARDEV_STATE_DISCONNECTED); name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label); s->reconnect_timer = qemu_chr_timeout_add_ms(chr, s->reconnect_time * 1000, @@ -131,7 +152,7 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len) { SocketChardev *s = SOCKET_CHARDEV(chr); - if (s->connected) { + if (s->state == TCP_CHARDEV_STATE_CONNECTED) { int ret = io_channel_send_full(s->ioc, buf, len, s->write_msgfds, s->write_msgfds_num); @@ -164,7 +185,7 @@ static int tcp_chr_read_poll(void *opaque) { Chardev *chr = CHARDEV(opaque); SocketChardev *s = SOCKET_CHARDEV(opaque); - if (!s->connected) { + if (s->state != TCP_CHARDEV_STATE_CONNECTED) { return 0; } s->max_size = qemu_chr_be_can_write(chr); @@ -277,7 +298,7 @@ static int tcp_set_msgfds(Chardev *chr, int *fds, int num) s->write_msgfds = NULL; s->write_msgfds_num = 0; - if (!s->connected || + if ((s->state != TCP_CHARDEV_STATE_CONNECTED) || !qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PASS)) { return -1; @@ -389,7 +410,7 @@ static void tcp_chr_free_connection(Chardev *chr) s->ioc = NULL; g_free(chr->filename); chr->filename = NULL; - s->connected = 0; + tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED); } static const char *qemu_chr_socket_protocol(SocketChardev *s) @@ -442,12 +463,12 @@ static void update_disconnected_filename(SocketChardev *s) /* NB may be called even if tcp_chr_connect has not been * reached, due to TLS or telnet initialization failure, - * so can *not* assume s->connected == true + * so can *not* assume s->state == TCP_CHARDEV_STATE_CONNECTED */ static void tcp_chr_disconnect(Chardev *chr) { SocketChardev *s = SOCKET_CHARDEV(chr); - bool emit_close = s->connected; + bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED; tcp_chr_free_connection(chr); @@ -471,7 +492,8 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque) uint8_t buf[CHR_READ_BUF_LEN]; int len, size; - if (!s->connected || s->max_size <= 0) { + if ((s->state != TCP_CHARDEV_STATE_CONNECTED) || + s->max_size <= 0) { return TRUE; } len = sizeof(buf); @@ -508,7 +530,7 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len) SocketChardev *s = SOCKET_CHARDEV(chr); int size; - if (!s->connected) { + if (s->state != TCP_CHARDEV_STATE_CONNECTED) { return 0; } @@ -564,7 +586,7 @@ static void update_ioc_handlers(SocketChardev *s) { Chardev *chr = CHARDEV(s); - if (!s->connected) { + if (s->state != TCP_CHARDEV_STATE_CONNECTED) { return; } @@ -589,7 +611,7 @@ static void tcp_chr_connect(void *opaque) g_free(chr->filename); chr->filename = qemu_chr_compute_filename(s); - s->connected = 1; + tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTED); update_ioc_handlers(s); qemu_chr_be_event(chr, CHR_EVENT_OPENED); } @@ -828,7 +850,7 @@ static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc) { SocketChardev *s = SOCKET_CHARDEV(chr); - if (s->ioc != NULL) { + if (s->state != TCP_CHARDEV_STATE_CONNECTING) { return -1; } @@ -865,11 +887,17 @@ static int tcp_chr_add_client(Chardev *chr, int fd) { int ret; QIOChannelSocket *sioc; + SocketChardev *s = SOCKET_CHARDEV(chr); + + if (s->state != TCP_CHARDEV_STATE_DISCONNECTED) { + return -1; + } sioc = qio_channel_socket_new_fd(fd, NULL); if (!sioc) { return -1; } + tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING); tcp_chr_set_client_ioc_name(chr, sioc); ret = tcp_chr_new_client(chr, sioc); object_unref(OBJECT(sioc)); @@ -881,7 +909,9 @@ static void tcp_chr_accept(QIONetListener *listener, void *opaque) { Chardev *chr = CHARDEV(opaque); + SocketChardev *s = SOCKET_CHARDEV(chr); + tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING); tcp_chr_set_client_ioc_name(chr, cioc); tcp_chr_new_client(chr, cioc); } @@ -891,8 +921,10 @@ static int tcp_chr_connect_client_sync(Chardev *chr, Error **errp) { SocketChardev *s = SOCKET_CHARDEV(chr); QIOChannelSocket *sioc = qio_channel_socket_new(); + tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING); tcp_chr_set_client_ioc_name(chr, sioc); if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) { + tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED); object_unref(OBJECT(sioc)); return -1; } @@ -908,6 +940,7 @@ static void tcp_chr_accept_server_sync(Chardev *chr) QIOChannelSocket *sioc; info_report("QEMU waiting for connection on: %s", chr->filename); + tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING); sioc = qio_net_listener_wait_client(s->listener); tcp_chr_set_client_ioc_name(chr, sioc); tcp_chr_new_client(chr, sioc); @@ -963,6 +996,7 @@ static void qemu_chr_socket_connected(QIOTask *task, void *opaque) Error *err = NULL; if (qio_task_propagate_error(task, &err)) { + tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED); check_report_connect_error(chr, err); error_free(err); goto cleanup; @@ -980,6 +1014,7 @@ static void tcp_chr_connect_client_async(Chardev *chr) SocketChardev *s = SOCKET_CHARDEV(chr); QIOChannelSocket *sioc; + tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING); sioc = qio_channel_socket_new(); tcp_chr_set_client_ioc_name(chr, sioc); qio_channel_socket_connect_async(sioc, s->addr, @@ -1307,7 +1342,7 @@ char_socket_get_connected(Object *obj, Error **errp) { SocketChardev *s = SOCKET_CHARDEV(obj); - return s->connected; + return s->state == TCP_CHARDEV_STATE_CONNECTED; } static void char_socket_class_init(ObjectClass *oc, void *data)
The socket connection state is indicated via the 'bool connected' field in the SocketChardev struct. This variable is somewhat misleading though, as it is only set to true once the connection has completed all required handshakes (eg for TLS, telnet or websockets). IOW there is a period of time in which the socket is connected, but the "connected" flag is still false. The socket chardev really has three states that it can be in, disconnected, connecting and connected and those should be tracked explicitly. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- chardev/char-socket.c | 63 +++++++++++++++++++++++++++++++++---------- 1 file changed, 49 insertions(+), 14 deletions(-)