From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60680) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fub4w-00086H-LW for qemu-devel@nongnu.org; Tue, 28 Aug 2018 06:18:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fub4s-0008EY-GL for qemu-devel@nongnu.org; Tue, 28 Aug 2018 06:18:30 -0400 Received: from smtp57.i.mail.ru ([217.69.128.37]:35514) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fub4r-0008DG-Uu for qemu-devel@nongnu.org; Tue, 28 Aug 2018 06:18:26 -0400 References: <20180827184103.32190-1-jusual@mail.ru> <20180827184103.32190-2-jusual@mail.ru> <20180828090249.GB25114@redhat.com> <20180828100914.GE25114@redhat.com> From: Julia Suvorova Message-ID: Date: Tue, 28 Aug 2018 13:18:19 +0300 MIME-Version: 1.0 In-Reply-To: <20180828100914.GE25114@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 1/2] chardev: Add websocket support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "=?UTF-8?Q?Daniel_P._Berrang=c3=a9?=" Cc: qemu-devel@nongnu.org, Eric Blake , marcandre.lureau@redhat.com, Paolo Bonzini , Stefan Hajnoczi , Stefan Hajnoczi , Joel Stanley , Jim Mussared , =?UTF-8?Q?Steffen_G=c3=b6rtz?= On 28.08.2018 13:09, Daniel P. Berrangé wrote: > On Tue, Aug 28, 2018 at 01:04:41PM +0300, Julia Suvorova wrote: >> On 28.08.2018 12:02, Daniel P. Berrangé wrote: >>> On Mon, Aug 27, 2018 at 09:41:02PM +0300, Julia Suvorova wrote: >>>> New option "websock" added to allow using websocket protocol for >>>> chardev socket backend. >>>> Example: >>>> -chardev socket,websock,id=... >>>> >>>> Signed-off-by: Julia Suvorova >>>> --- >>>> chardev/char-socket.c | 124 +++++++++++++++++++++++++++++++----------- >>>> chardev/char.c | 8 ++- >>>> qapi/char.json | 3 + >>>> 3 files changed, 103 insertions(+), 32 deletions(-) >>> >>> Also needs changes to qemu-options.hx to document the new >>> websock option. This doc ends up included in the main >>> qemu-doc.html published on the website, and also forms the >>> man page for qemu. >>> >>>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c >>>> index efbad6ee7c..1d3c14d85d 100644 >>>> --- a/chardev/char-socket.c >>>> +++ b/chardev/char-socket.c >>>> @@ -26,6 +26,7 @@ >>>> #include "chardev/char.h" >>>> #include "io/channel-socket.h" >>>> #include "io/channel-tls.h" >>>> +#include "io/channel-websock.h" >>>> #include "io/net-listener.h" >>>> #include "qemu/error-report.h" >>>> #include "qemu/option.h" >>>> @@ -69,6 +70,8 @@ typedef struct { >>>> GSource *telnet_source; >>>> TCPChardevTelnetInit *telnet_init; >>>> + bool is_websock; >>>> + >>>> GSource *reconnect_timer; >>>> int64_t reconnect_time; >>>> bool connect_err_reported; >>>> @@ -385,30 +388,37 @@ static void tcp_chr_free_connection(Chardev *chr) >>>> s->connected = 0; >>>> } >>>> -static char *SocketAddress_to_str(const char *prefix, SocketAddress *addr, >>>> - bool is_listen, bool is_telnet) >>>> +static const char *qemu_chr_socket_protocol(SocketChardev *s) >>>> +{ >>>> + if (s->is_telnet) { >>>> + return "telnet"; >>>> + } >>>> + return s->is_websock ? "websock" : "tcp"; >>>> +} >>>> + >>>> +static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix) >>>> { >>>> - switch (addr->type) { >>>> + switch (s->addr->type) { >>>> case SOCKET_ADDRESS_TYPE_INET: >>>> return g_strdup_printf("%s%s:%s:%s%s", prefix, >>>> - is_telnet ? "telnet" : "tcp", >>>> - addr->u.inet.host, >>>> - addr->u.inet.port, >>>> - is_listen ? ",server" : ""); >>>> + qemu_chr_socket_protocol(s), >>>> + s->addr->u.inet.host, >>>> + s->addr->u.inet.port, >>>> + s->is_listen ? ",server" : ""); >>>> break; >>>> case SOCKET_ADDRESS_TYPE_UNIX: >>>> return g_strdup_printf("%sunix:%s%s", prefix, >>>> - addr->u.q_unix.path, >>>> - is_listen ? ",server" : ""); >>>> + s->addr->u.q_unix.path, >>>> + s->is_listen ? ",server" : ""); >>>> break; >>>> case SOCKET_ADDRESS_TYPE_FD: >>>> - return g_strdup_printf("%sfd:%s%s", prefix, addr->u.fd.str, >>>> - is_listen ? ",server" : ""); >>>> + return g_strdup_printf("%sfd:%s%s", prefix, s->addr->u.fd.str, >>>> + s->is_listen ? ",server" : ""); >>>> break; >>>> case SOCKET_ADDRESS_TYPE_VSOCK: >>>> return g_strdup_printf("%svsock:%s:%s", prefix, >>>> - addr->u.vsock.cid, >>>> - addr->u.vsock.port); >>>> + s->addr->u.vsock.cid, >>>> + s->addr->u.vsock.port); >>>> default: >>>> abort(); >>>> } >>>> @@ -419,8 +429,7 @@ static void update_disconnected_filename(SocketChardev *s) >>>> Chardev *chr = CHARDEV(s); >>>> g_free(chr->filename); >>>> - chr->filename = SocketAddress_to_str("disconnected:", s->addr, >>>> - s->is_listen, s->is_telnet); >>>> + chr->filename = qemu_chr_socket_address(s, "disconnected:"); >>>> } >>>> /* NB may be called even if tcp_chr_connect has not been >>>> @@ -506,10 +515,12 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len) >>>> return size; >>>> } >>>> -static char *sockaddr_to_str(struct sockaddr_storage *ss, socklen_t ss_len, >>>> - struct sockaddr_storage *ps, socklen_t ps_len, >>>> - bool is_listen, bool is_telnet) >>>> +static char *qemu_chr_compute_filename(SocketChardev *s) >>>> { >>>> + struct sockaddr_storage *ss = &s->sioc->localAddr; >>>> + struct sockaddr_storage *ps = &s->sioc->remoteAddr; >>>> + socklen_t ss_len = s->sioc->localAddrLen; >>>> + socklen_t ps_len = s->sioc->remoteAddrLen; >>>> char shost[NI_MAXHOST], sserv[NI_MAXSERV]; >>>> char phost[NI_MAXHOST], pserv[NI_MAXSERV]; >>>> const char *left = "", *right = ""; >>>> @@ -519,7 +530,7 @@ static char *sockaddr_to_str(struct sockaddr_storage *ss, socklen_t ss_len, >>>> case AF_UNIX: >>>> return g_strdup_printf("unix:%s%s", >>>> ((struct sockaddr_un *)(ss))->sun_path, >>>> - is_listen ? ",server" : ""); >>>> + s->is_listen ? ",server" : ""); >>>> #endif >>>> case AF_INET6: >>>> left = "["; >>>> @@ -531,9 +542,9 @@ static char *sockaddr_to_str(struct sockaddr_storage *ss, socklen_t ss_len, >>>> getnameinfo((struct sockaddr *) ps, ps_len, phost, sizeof(phost), >>>> pserv, sizeof(pserv), NI_NUMERICHOST | NI_NUMERICSERV); >>>> return g_strdup_printf("%s:%s%s%s:%s%s <-> %s%s%s:%s", >>>> - is_telnet ? "telnet" : "tcp", >>>> + qemu_chr_socket_protocol(s), >>>> left, shost, right, sserv, >>>> - is_listen ? ",server" : "", >>>> + s->is_listen ? ",server" : "", >>>> left, phost, right, pserv); >>>> default: >>>> @@ -547,10 +558,7 @@ static void tcp_chr_connect(void *opaque) >>>> SocketChardev *s = SOCKET_CHARDEV(opaque); >>>> g_free(chr->filename); >>>> - chr->filename = sockaddr_to_str( >>>> - &s->sioc->localAddr, s->sioc->localAddrLen, >>>> - &s->sioc->remoteAddr, s->sioc->remoteAddrLen, >>>> - s->is_listen, s->is_telnet); >>>> + chr->filename = qemu_chr_compute_filename(s); >>>> s->connected = 1; >>>> chr->gsource = io_add_watch_poll(chr, s->ioc, >>> >>> Everything above this point is refactoring of existing code, mixed in with >>> a few websock additions. >>> >>> Everything below this is implementing the websocket feature. >>> >>> These two distinct tasks should be separated, so that refactoring is in >>> the first patch, and websocks impl is in a second patch. >> >> Thus, I will have second patch based on the first patch, and the >> first will contain code that will never be used. If it's okay, I will >> divide the patch. > > Don't really know why you're saying the first patch code won't be used. > The code you're refactoring here is already used today, and you are just > renaming methods & changing what parameters are passed to it. I meant that some code will be replaced with a second patch. For instance, in the first patch I will add this line: + return "tcp"; And in the second it will be replaced by: + return s->is_websock ? "websock" : "tcp"; So this is extra unnecessary code. Best regards, Julia Suvorova.