From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39290) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fubT7-0001vj-DP for qemu-devel@nongnu.org; Tue, 28 Aug 2018 06:43:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fubNv-0000Tv-TJ for qemu-devel@nongnu.org; Tue, 28 Aug 2018 06:38:12 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:39722 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fubNv-0000Ta-LK for qemu-devel@nongnu.org; Tue, 28 Aug 2018 06:38:07 -0400 Date: Tue, 28 Aug 2018 11:37:57 +0100 From: Daniel =?utf-8?B?UC4gQmVycmFuZ8Op?= Message-ID: <20180828103757.GI25114@redhat.com> Reply-To: Daniel =?utf-8?B?UC4gQmVycmFuZ8Op?= References: <20180827184103.32190-1-jusual@mail.ru> <20180827184103.32190-2-jusual@mail.ru> <20180828090249.GB25114@redhat.com> <20180828100914.GE25114@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable 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: Julia Suvorova Cc: qemu-devel@nongnu.org, Eric Blake , marcandre.lureau@redhat.com, Paolo Bonzini , Stefan Hajnoczi , Stefan Hajnoczi , Joel Stanley , Jim Mussared , Steffen =?utf-8?B?R8O2cnR6?= On Tue, Aug 28, 2018 at 01:18:19PM +0300, Julia Suvorova wrote: > On 28.08.2018 13:09, Daniel P. Berrang=C3=A9 wrote: > > On Tue, Aug 28, 2018 at 01:04:41PM +0300, Julia Suvorova wrote: > > > On 28.08.2018 12:02, Daniel P. Berrang=C3=A9 wrote: > > > > On Mon, Aug 27, 2018 at 09:41:02PM +0300, Julia Suvorova wrote: > > > > > New option "websock" added to allow using websocket protocol fo= r > > > > > chardev socket backend. > > > > > Example: > > > > > -chardev socket,websock,id=3D... > > > > >=20 > > > > > 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(-) > > > >=20 > > > > 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. > > > >=20 > > > > > 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(Chard= ev *chr) > > > > > s->connected =3D 0; > > > > > } > > > > > -static char *SocketAddress_to_str(const char *prefix, SocketAd= dress *addr, > > > > > - bool is_listen, bool is_teln= et) > > > > > +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 c= har *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(So= cketChardev *s) > > > > > Chardev *chr =3D CHARDEV(s); > > > > > g_free(chr->filename); > > > > > - chr->filename =3D SocketAddress_to_str("disconnected:", s-= >addr, > > > > > - s->is_listen, s->is_t= elnet); > > > > > + chr->filename =3D 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, sock= len_t ss_len, > > > > > - struct sockaddr_storage *ps, sock= len_t ps_len, > > > > > - bool is_listen, bool is_telnet) > > > > > +static char *qemu_chr_compute_filename(SocketChardev *s) > > > > > { > > > > > + struct sockaddr_storage *ss =3D &s->sioc->localAddr; > > > > > + struct sockaddr_storage *ps =3D &s->sioc->remoteAddr; > > > > > + socklen_t ss_len =3D s->sioc->localAddrLen; > > > > > + socklen_t ps_len =3D s->sioc->remoteAddrLen; > > > > > char shost[NI_MAXHOST], sserv[NI_MAXSERV]; > > > > > char phost[NI_MAXHOST], pserv[NI_MAXSERV]; > > > > > const char *left =3D "", *right =3D ""; > > > > > @@ -519,7 +530,7 @@ static char *sockaddr_to_str(struct sockadd= r_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 =3D "["; > > > > > @@ -531,9 +542,9 @@ static char *sockaddr_to_str(struct sockadd= r_storage *ss, socklen_t ss_len, > > > > > getnameinfo((struct sockaddr *) ps, ps_len, phost, s= izeof(phost), > > > > > pserv, sizeof(pserv), NI_NUMERICHOST | N= I_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 =3D SOCKET_CHARDEV(opaque); > > > > > g_free(chr->filename); > > > > > - chr->filename =3D sockaddr_to_str( > > > > > - &s->sioc->localAddr, s->sioc->localAddrLen, > > > > > - &s->sioc->remoteAddr, s->sioc->remoteAddrLen, > > > > > - s->is_listen, s->is_telnet); > > > > > + chr->filename =3D qemu_chr_compute_filename(s); > > > > > s->connected =3D 1; > > > > > chr->gsource =3D io_add_watch_poll(chr, s->ioc, > > > >=20 > > > > Everything above this point is refactoring of existing code, mixe= d in with > > > > a few websock additions. > > > >=20 > > > > Everything below this is implementing the websocket feature. > > > >=20 > > > > These two distinct tasks should be separated, so that refactoring= is in > > > > the first patch, and websocks impl is in a second patch. > > >=20 > > > 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 wi= ll > > > divide the patch. > >=20 > > Don't really know why you're saying the first patch code won't be use= d. > > The code you're refactoring here is already used today, and you are j= ust > > renaming methods & changing what parameters are passed to it. >=20 > 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. That is to be expected and isn't really considered unnecessary code. It is best practice when separating work into a series of commits to ensure each commit only covers one distinct task. Regards, Daniel --=20 |: https://berrange.com -o- https://www.flickr.com/photos/dberran= ge :| |: https://libvirt.org -o- https://fstop138.berrange.c= om :| |: https://entangle-photo.org -o- https://www.instagram.com/dberran= ge :|