On Tue, Aug 28, 2018 at 01:18:19PM +0300, Julia Suvorova wrote:
> 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 <jus...@mail.ru>
> > > > > ---
> > > > > 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.
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
