* [Qemu-devel] [PATCH v2 0/2] chardev: Add websocket support
@ 2018-08-27 18:41 Julia Suvorova
2018-08-27 18:41 ` [Qemu-devel] [PATCH v2 1/2] " Julia Suvorova
2018-08-27 18:41 ` [Qemu-devel] [PATCH v2 2/2] tests/test-char: Check websocket chardev functionality Julia Suvorova
0 siblings, 2 replies; 11+ messages in thread
From: Julia Suvorova @ 2018-08-27 18:41 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P . Berrangé,
Eric Blake, marcandre.lureau, Paolo Bonzini, Stefan Hajnoczi,
Stefan Hajnoczi, Joel Stanley, Jim Mussared, Steffen Görtz,
Julia Suvorova
v2:
* Fixed initialization order [Daniel]
* Function arguments refactoring [Paolo]
* Added test [Stefan]
* Added meaningful error message [Stefan]
* Added "websock:" URI prefix support
Julia Suvorova (2):
chardev: Add websocket support
tests/test-char: Check websocket chardev functionality
chardev/char-socket.c | 124 ++++++++++++++++++++++++++++++-----------
chardev/char.c | 8 ++-
qapi/char.json | 3 +
tests/test-char.c | 125 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 228 insertions(+), 32 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] chardev: Add websocket support
2018-08-27 18:41 [Qemu-devel] [PATCH v2 0/2] chardev: Add websocket support Julia Suvorova
@ 2018-08-27 18:41 ` Julia Suvorova
2018-08-28 9:02 ` Daniel P. Berrangé
2018-10-03 14:34 ` Stefan Hajnoczi
2018-08-27 18:41 ` [Qemu-devel] [PATCH v2 2/2] tests/test-char: Check websocket chardev functionality Julia Suvorova
1 sibling, 2 replies; 11+ messages in thread
From: Julia Suvorova @ 2018-08-27 18:41 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P . Berrangé,
Eric Blake, marcandre.lureau, Paolo Bonzini, Stefan Hajnoczi,
Stefan Hajnoczi, Joel Stanley, Jim Mussared, Steffen Görtz,
Julia Suvorova
New option "websock" added to allow using websocket protocol for
chardev socket backend.
Example:
-chardev socket,websock,id=...
Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
chardev/char-socket.c | 124 +++++++++++++++++++++++++++++++-----------
chardev/char.c | 8 ++-
qapi/char.json | 3 +
3 files changed, 103 insertions(+), 32 deletions(-)
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,
@@ -699,6 +707,48 @@ cont:
}
+static void tcp_chr_websock_handshake(QIOTask *task, gpointer user_data)
+{
+ Chardev *chr = user_data;
+ SocketChardev *s = user_data;
+
+ if (qio_task_propagate_error(task, NULL)) {
+ tcp_chr_disconnect(chr);
+ } else {
+ if (s->do_telnetopt) {
+ tcp_chr_telnet_init(chr);
+ } else {
+ tcp_chr_connect(chr);
+ }
+ }
+}
+
+
+static void tcp_chr_websock_init(Chardev *chr)
+{
+ SocketChardev *s = SOCKET_CHARDEV(chr);
+ QIOChannelWebsock *wioc = NULL;
+ gchar *name;
+
+ if (s->is_listen) {
+ wioc = qio_channel_websock_new_server(s->ioc);
+ };
+
+ if (wioc == NULL) {
+ tcp_chr_disconnect(chr);
+ return;
+ }
+
+ name = g_strdup_printf("chardev-websock-server-%s", chr->label);
+ qio_channel_set_name(QIO_CHANNEL(wioc), name);
+ g_free(name);
+ object_unref(OBJECT(s->ioc));
+ s->ioc = QIO_CHANNEL(wioc);
+
+ qio_channel_websock_handshake(wioc, tcp_chr_websock_handshake, chr, NULL);
+}
+
+
static void tcp_chr_tls_handshake(QIOTask *task,
gpointer user_data)
{
@@ -708,7 +758,9 @@ static void tcp_chr_tls_handshake(QIOTask *task,
if (qio_task_propagate_error(task, NULL)) {
tcp_chr_disconnect(chr);
} else {
- if (s->do_telnetopt) {
+ if (s->is_websock) {
+ tcp_chr_websock_init(chr);
+ } else if (s->do_telnetopt) {
tcp_chr_telnet_init(chr);
} else {
tcp_chr_connect(chr);
@@ -799,12 +851,12 @@ static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc)
if (s->tls_creds) {
tcp_chr_tls_init(chr);
+ } else if (s->is_websock) {
+ tcp_chr_websock_init(chr);
+ } else if (s->do_telnetopt) {
+ tcp_chr_telnet_init(chr);
} else {
- if (s->do_telnetopt) {
- tcp_chr_telnet_init(chr);
- } else {
- tcp_chr_connect(chr);
- }
+ tcp_chr_connect(chr);
}
return 0;
@@ -948,14 +1000,21 @@ static void qmp_chardev_open_socket(Chardev *chr,
bool is_listen = sock->has_server ? sock->server : true;
bool is_telnet = sock->has_telnet ? sock->telnet : false;
bool is_tn3270 = sock->has_tn3270 ? sock->tn3270 : false;
+ bool is_websock = sock->has_websock ? sock->websock : false;
bool is_waitconnect = sock->has_wait ? sock->wait : false;
int64_t reconnect = sock->has_reconnect ? sock->reconnect : 0;
QIOChannelSocket *sioc = NULL;
SocketAddress *addr;
+ if (!is_listen && is_websock) {
+ error_setg(errp, "%s", "Websocket client is not implemented");
+ goto error;
+ }
+
s->is_listen = is_listen;
s->is_telnet = is_telnet;
s->is_tn3270 = is_tn3270;
+ s->is_websock = is_websock;
s->do_nodelay = do_nodelay;
if (sock->tls_creds) {
Object *creds;
@@ -1061,6 +1120,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true);
bool is_telnet = qemu_opt_get_bool(opts, "telnet", false);
bool is_tn3270 = qemu_opt_get_bool(opts, "tn3270", false);
+ bool is_websock = qemu_opt_get_bool(opts, "websock", false);
bool do_nodelay = !qemu_opt_get_bool(opts, "delay", true);
int64_t reconnect = qemu_opt_get_number(opts, "reconnect", 0);
const char *path = qemu_opt_get(opts, "path");
@@ -1109,6 +1169,8 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
sock->telnet = is_telnet;
sock->has_tn3270 = true;
sock->tn3270 = is_tn3270;
+ sock->has_websock = true;
+ sock->websock = is_websock;
sock->has_wait = true;
sock->wait = is_waitconnect;
sock->has_reconnect = true;
diff --git a/chardev/char.c b/chardev/char.c
index 1ab80e0f68..c045b8183b 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -404,7 +404,8 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
}
if (strstart(filename, "tcp:", &p) ||
strstart(filename, "telnet:", &p) ||
- strstart(filename, "tn3270:", &p)) {
+ strstart(filename, "tn3270:", &p) ||
+ strstart(filename, "websock:", &p)) {
if (sscanf(p, "%64[^:]:%32[^,]%n", host, port, &pos) < 2) {
host[0] = 0;
if (sscanf(p, ":%32[^,]%n", port, &pos) < 1)
@@ -424,6 +425,8 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
qemu_opt_set(opts, "telnet", "on", &error_abort);
} else if (strstart(filename, "tn3270:", &p)) {
qemu_opt_set(opts, "tn3270", "on", &error_abort);
+ } else if (strstart(filename, "websock:", &p)) {
+ qemu_opt_set(opts, "websock", "on", &error_abort);
}
return opts;
}
@@ -837,6 +840,9 @@ QemuOptsList qemu_chardev_opts = {
},{
.name = "tls-creds",
.type = QEMU_OPT_STRING,
+ },{
+ .name = "websock",
+ .type = QEMU_OPT_BOOL,
},{
.name = "width",
.type = QEMU_OPT_NUMBER,
diff --git a/qapi/char.json b/qapi/char.json
index b7b2a05766..aa48a150a8 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -251,6 +251,8 @@
# sockets (default: false)
# @tn3270: enable tn3270 protocol on server
# sockets (default: false) (Since: 2.10)
+# @websock: enable websocket protocol on server
+# sockets (default: false) (Since: 3.1)
# @reconnect: For a client socket, if a socket is disconnected,
# then attempt a reconnect after the given number of seconds.
# Setting this to zero disables this function. (default: 0)
@@ -265,6 +267,7 @@
'*nodelay' : 'bool',
'*telnet' : 'bool',
'*tn3270' : 'bool',
+ '*websock' : 'bool',
'*reconnect' : 'int' },
'base': 'ChardevCommon' }
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] tests/test-char: Check websocket chardev functionality
2018-08-27 18:41 [Qemu-devel] [PATCH v2 0/2] chardev: Add websocket support Julia Suvorova
2018-08-27 18:41 ` [Qemu-devel] [PATCH v2 1/2] " Julia Suvorova
@ 2018-08-27 18:41 ` Julia Suvorova
2018-08-28 9:07 ` Daniel P. Berrangé
2018-10-03 14:37 ` Stefan Hajnoczi
1 sibling, 2 replies; 11+ messages in thread
From: Julia Suvorova @ 2018-08-27 18:41 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P . Berrangé,
Eric Blake, marcandre.lureau, Paolo Bonzini, Stefan Hajnoczi,
Stefan Hajnoczi, Joel Stanley, Jim Mussared, Steffen Görtz,
Julia Suvorova
Test order:
Creating server websocket chardev
Creating usual tcp chardev client
Sending handshake message from client
Receiving handshake reply
Sending ping frame with "hello" payload
Receiving pong reply
Sending binary data "world"
Checking the received data on server side
Checking of closing handshake
Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
tests/test-char.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 125 insertions(+)
diff --git a/tests/test-char.c b/tests/test-char.c
index 5905d31441..deba699b25 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -406,6 +406,130 @@ static void char_socket_fdpass_test(void)
}
+static void websock_server_read(void *opaque, const uint8_t *buf, int size)
+{
+ g_assert(size == 5);
+ g_assert(strncmp((char *) buf, "world", size) == 0);
+ quit = true;
+}
+
+
+static int websock_server_can_read(void *opaque)
+{
+ return 10;
+}
+
+
+static bool websock_check_http_headers(char *buf, int size)
+{
+ int i;
+ const char *ans[] = { "HTTP/1.1 101 Switching Protocols\r\n",
+ "Server: QEMU VNC\r\n",
+ "Upgrade: websocket\r\n",
+ "Connection: Upgrade\r\n",
+ "Sec-WebSocket-Accept:",
+ "Sec-WebSocket-Protocol: binary\r\n" };
+
+ for (i = 0; i < 6; i++) {
+ if (g_strstr_len(buf, size, ans[i]) == NULL) {
+ return false;
+ }
+ }
+
+ return true;
+}
+
+
+static void websock_client_read(void *opaque, const uint8_t *buf, int size)
+{
+ const uint8_t ping[] = { 0x89, 0x85, /* Ping header */
+ 0x07, 0x77, 0x9e, 0xf9, /* Masking key */
+ 0x6f, 0x12, 0xf2, 0x95, 0x68 /* "hello" */ };
+
+ const uint8_t binary[] = { 0x82, 0x85, /* Binary header */
+ 0x74, 0x90, 0xb9, 0xdf, /* Masking key */
+ 0x03, 0xff, 0xcb, 0xb3, 0x10 /* "world" */ };
+ Chardev *chr_client = opaque;
+
+ if (websock_check_http_headers((char *) buf, size)) {
+ qemu_chr_fe_write(chr_client->be, ping, sizeof(ping));
+ } else if (buf[0] == 0x8a && buf[1] == 0x05) {
+ g_assert(strncmp((char *) buf + 2, "hello", 5) == 0);
+ qemu_chr_fe_write(chr_client->be, binary, sizeof(binary));
+ } else {
+ g_assert(buf[0] == 0x88 && buf[1] == 0x16);
+ g_assert(strncmp((char *) buf + 4, "peer requested close", 10) == 0);
+ quit = true;
+ }
+}
+
+
+static int websock_client_can_read(void *opaque)
+{
+ return 4096;
+}
+
+
+static void char_websock_test(void)
+{
+ QObject *addr;
+ QDict *qdict;
+ const char *port;
+ char *tmp;
+ char *handshake_port;
+ CharBackend be;
+ CharBackend client_be;
+ Chardev *chr_client;
+ Chardev *chr = qemu_chr_new("server",
+ "websock:127.0.0.1:0,server,nowait");
+ const char handshake[] = "GET / HTTP/1.1\r\n"
+ "Upgrade: websocket\r\n"
+ "Connection: Upgrade\r\n"
+ "Host: localhost:%s\r\n"
+ "Origin: http://localhost:%s\r\n"
+ "Sec-WebSocket-Key: o9JHNiS3/0/0zYE1wa3yIw==\r\n"
+ "Sec-WebSocket-Version: 13\r\n"
+ "Sec-WebSocket-Protocol: binary\r\n\r\n";
+ const uint8_t close[] = { 0x88, 0x82, /* Close header */
+ 0xef, 0xaa, 0xc5, 0x97, /* Masking key */
+ 0xec, 0x42 /* Status code */ };
+
+ addr = object_property_get_qobject(OBJECT(chr), "addr", &error_abort);
+ qdict = qobject_to(QDict, addr);
+ port = qdict_get_str(qdict, "port");
+ tmp = g_strdup_printf("tcp:127.0.0.1:%s", port);
+ handshake_port = g_strdup_printf(handshake, port, port);
+ qobject_unref(qdict);
+
+ qemu_chr_fe_init(&be, chr, &error_abort);
+ qemu_chr_fe_set_handlers(&be, websock_server_can_read, websock_server_read,
+ NULL, NULL, chr, NULL, true);
+
+ chr_client = qemu_chr_new("client", tmp);
+ qemu_chr_fe_init(&client_be, chr_client, &error_abort);
+ qemu_chr_fe_set_handlers(&client_be, websock_client_can_read,
+ websock_client_read,
+ NULL, NULL, chr_client, NULL, true);
+ g_free(tmp);
+
+ qemu_chr_write_all(chr_client,
+ (uint8_t *) handshake_port,
+ strlen(handshake_port));
+ g_free(handshake_port);
+ main_loop();
+
+ g_assert(object_property_get_bool(OBJECT(chr), "connected", &error_abort));
+ g_assert(object_property_get_bool(OBJECT(chr_client),
+ "connected", &error_abort));
+
+ qemu_chr_write_all(chr_client, close, sizeof(close));
+ main_loop();
+
+ object_unparent(OBJECT(chr_client));
+ object_unparent(OBJECT(chr));
+}
+
+
#ifndef _WIN32
static void char_pipe_test(void)
{
@@ -829,6 +953,7 @@ int main(int argc, char **argv)
g_test_add_func("/char/serial", char_serial_test);
#endif
g_test_add_func("/char/hotswap", char_hotswap_test);
+ g_test_add_func("/char/websocket", char_websock_test);
return g_test_run();
}
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] chardev: Add websocket support
2018-08-27 18:41 ` [Qemu-devel] [PATCH v2 1/2] " Julia Suvorova
@ 2018-08-28 9:02 ` Daniel P. Berrangé
2018-08-28 10:04 ` Julia Suvorova
2018-10-03 14:34 ` Stefan Hajnoczi
1 sibling, 1 reply; 11+ messages in thread
From: Daniel P. Berrangé @ 2018-08-28 9:02 UTC (permalink / raw)
To: Julia Suvorova
Cc: qemu-devel, Eric Blake, marcandre.lureau, Paolo Bonzini,
Stefan Hajnoczi, Stefan Hajnoczi, Joel Stanley, Jim Mussared,
Steffen Görtz
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 <jusual@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.
> @@ -699,6 +707,48 @@ cont:
> }
>
>
> +static void tcp_chr_websock_handshake(QIOTask *task, gpointer user_data)
> +{
> + Chardev *chr = user_data;
> + SocketChardev *s = user_data;
> +
> + if (qio_task_propagate_error(task, NULL)) {
> + tcp_chr_disconnect(chr);
> + } else {
> + if (s->do_telnetopt) {
> + tcp_chr_telnet_init(chr);
> + } else {
> + tcp_chr_connect(chr);
> + }
> + }
> +}
> +
> +
> +static void tcp_chr_websock_init(Chardev *chr)
> +{
> + SocketChardev *s = SOCKET_CHARDEV(chr);
> + QIOChannelWebsock *wioc = NULL;
> + gchar *name;
> +
> + if (s->is_listen) {
> + wioc = qio_channel_websock_new_server(s->ioc);
> + };
You've already rejected the !is_listen codepath later on, so
this if () is redundant and can be removed.
> +
> + if (wioc == NULL) {
> + tcp_chr_disconnect(chr);
> + return;
> + }
qio_channel_websock_new_server() cannot fail, so wioc will never
be NULL.
> + name = g_strdup_printf("chardev-websock-server-%s", chr->label);
> + qio_channel_set_name(QIO_CHANNEL(wioc), name);
> + g_free(name);
> + object_unref(OBJECT(s->ioc));
> + s->ioc = QIO_CHANNEL(wioc);
> +
> + qio_channel_websock_handshake(wioc, tcp_chr_websock_handshake, chr, NULL);
> +}
> +
> +
> static void tcp_chr_tls_handshake(QIOTask *task,
> gpointer user_data)
> {
> @@ -708,7 +758,9 @@ static void tcp_chr_tls_handshake(QIOTask *task,
> if (qio_task_propagate_error(task, NULL)) {
> tcp_chr_disconnect(chr);
> } else {
> - if (s->do_telnetopt) {
> + if (s->is_websock) {
> + tcp_chr_websock_init(chr);
> + } else if (s->do_telnetopt) {
> tcp_chr_telnet_init(chr);
> } else {
> tcp_chr_connect(chr);
> @@ -799,12 +851,12 @@ static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc)
>
> if (s->tls_creds) {
> tcp_chr_tls_init(chr);
> + } else if (s->is_websock) {
> + tcp_chr_websock_init(chr);
> + } else if (s->do_telnetopt) {
> + tcp_chr_telnet_init(chr);
> } else {
> - if (s->do_telnetopt) {
> - tcp_chr_telnet_init(chr);
> - } else {
> - tcp_chr_connect(chr);
> - }
> + tcp_chr_connect(chr);
> }
>
> return 0;
> @@ -948,14 +1000,21 @@ static void qmp_chardev_open_socket(Chardev *chr,
> bool is_listen = sock->has_server ? sock->server : true;
> bool is_telnet = sock->has_telnet ? sock->telnet : false;
> bool is_tn3270 = sock->has_tn3270 ? sock->tn3270 : false;
> + bool is_websock = sock->has_websock ? sock->websock : false;
> bool is_waitconnect = sock->has_wait ? sock->wait : false;
> int64_t reconnect = sock->has_reconnect ? sock->reconnect : 0;
> QIOChannelSocket *sioc = NULL;
> SocketAddress *addr;
>
> + if (!is_listen && is_websock) {
> + error_setg(errp, "%s", "Websocket client is not implemented");
> + goto error;
> + }
> +
> s->is_listen = is_listen;
> s->is_telnet = is_telnet;
> s->is_tn3270 = is_tn3270;
> + s->is_websock = is_websock;
> s->do_nodelay = do_nodelay;
> if (sock->tls_creds) {
> Object *creds;
> @@ -1061,6 +1120,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
> bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true);
> bool is_telnet = qemu_opt_get_bool(opts, "telnet", false);
> bool is_tn3270 = qemu_opt_get_bool(opts, "tn3270", false);
> + bool is_websock = qemu_opt_get_bool(opts, "websock", false);
> bool do_nodelay = !qemu_opt_get_bool(opts, "delay", true);
> int64_t reconnect = qemu_opt_get_number(opts, "reconnect", 0);
> const char *path = qemu_opt_get(opts, "path");
> @@ -1109,6 +1169,8 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
> sock->telnet = is_telnet;
> sock->has_tn3270 = true;
> sock->tn3270 = is_tn3270;
> + sock->has_websock = true;
> + sock->websock = is_websock;
> sock->has_wait = true;
> sock->wait = is_waitconnect;
> sock->has_reconnect = true;
> diff --git a/chardev/char.c b/chardev/char.c
> index 1ab80e0f68..c045b8183b 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -404,7 +404,8 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
> }
> if (strstart(filename, "tcp:", &p) ||
> strstart(filename, "telnet:", &p) ||
> - strstart(filename, "tn3270:", &p)) {
> + strstart(filename, "tn3270:", &p) ||
> + strstart(filename, "websock:", &p)) {
> if (sscanf(p, "%64[^:]:%32[^,]%n", host, port, &pos) < 2) {
> host[0] = 0;
> if (sscanf(p, ":%32[^,]%n", port, &pos) < 1)
> @@ -424,6 +425,8 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
> qemu_opt_set(opts, "telnet", "on", &error_abort);
> } else if (strstart(filename, "tn3270:", &p)) {
> qemu_opt_set(opts, "tn3270", "on", &error_abort);
> + } else if (strstart(filename, "websock:", &p)) {
> + qemu_opt_set(opts, "websock", "on", &error_abort);
> }
> return opts;
> }
> @@ -837,6 +840,9 @@ QemuOptsList qemu_chardev_opts = {
> },{
> .name = "tls-creds",
> .type = QEMU_OPT_STRING,
> + },{
> + .name = "websock",
> + .type = QEMU_OPT_BOOL,
> },{
> .name = "width",
> .type = QEMU_OPT_NUMBER,
> diff --git a/qapi/char.json b/qapi/char.json
> index b7b2a05766..aa48a150a8 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -251,6 +251,8 @@
> # sockets (default: false)
> # @tn3270: enable tn3270 protocol on server
> # sockets (default: false) (Since: 2.10)
> +# @websock: enable websocket protocol on server
> +# sockets (default: false) (Since: 3.1)
> # @reconnect: For a client socket, if a socket is disconnected,
> # then attempt a reconnect after the given number of seconds.
> # Setting this to zero disables this function. (default: 0)
> @@ -265,6 +267,7 @@
> '*nodelay' : 'bool',
> '*telnet' : 'bool',
> '*tn3270' : 'bool',
> + '*websock' : 'bool',
> '*reconnect' : 'int' },
> 'base': 'ChardevCommon' }
>
> --
> 2.17.1
>
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 :|
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] tests/test-char: Check websocket chardev functionality
2018-08-27 18:41 ` [Qemu-devel] [PATCH v2 2/2] tests/test-char: Check websocket chardev functionality Julia Suvorova
@ 2018-08-28 9:07 ` Daniel P. Berrangé
2018-10-03 14:37 ` Stefan Hajnoczi
1 sibling, 0 replies; 11+ messages in thread
From: Daniel P. Berrangé @ 2018-08-28 9:07 UTC (permalink / raw)
To: Julia Suvorova
Cc: qemu-devel, Eric Blake, marcandre.lureau, Paolo Bonzini,
Stefan Hajnoczi, Stefan Hajnoczi, Joel Stanley, Jim Mussared,
Steffen Görtz
On Mon, Aug 27, 2018 at 09:41:03PM +0300, Julia Suvorova wrote:
> Test order:
> Creating server websocket chardev
> Creating usual tcp chardev client
> Sending handshake message from client
> Receiving handshake reply
> Sending ping frame with "hello" payload
> Receiving pong reply
> Sending binary data "world"
> Checking the received data on server side
> Checking of closing handshake
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
> tests/test-char.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 125 insertions(+)
>
> diff --git a/tests/test-char.c b/tests/test-char.c
> index 5905d31441..deba699b25 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -406,6 +406,130 @@ static void char_socket_fdpass_test(void)
> }
>
>
> +static void websock_server_read(void *opaque, const uint8_t *buf, int size)
> +{
> + g_assert(size == 5);
g_assert_cmpint
> + g_assert(strncmp((char *) buf, "world", size) == 0);
memcmp() feels better since we're not actually sending NUL
terminated strings over the wire - just the "world" without
a trailing NUL. Likewise for places below which is strncmp
> + quit = true;
> +}
> +
> +
> +static int websock_server_can_read(void *opaque)
> +{
> + return 10;
> +}
> +
> +
> +static bool websock_check_http_headers(char *buf, int size)
> +{
> + int i;
> + const char *ans[] = { "HTTP/1.1 101 Switching Protocols\r\n",
> + "Server: QEMU VNC\r\n",
> + "Upgrade: websocket\r\n",
> + "Connection: Upgrade\r\n",
> + "Sec-WebSocket-Accept:",
> + "Sec-WebSocket-Protocol: binary\r\n" };
> +
> + for (i = 0; i < 6; i++) {
> + if (g_strstr_len(buf, size, ans[i]) == NULL) {
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
> +
> +static void websock_client_read(void *opaque, const uint8_t *buf, int size)
> +{
> + const uint8_t ping[] = { 0x89, 0x85, /* Ping header */
> + 0x07, 0x77, 0x9e, 0xf9, /* Masking key */
> + 0x6f, 0x12, 0xf2, 0x95, 0x68 /* "hello" */ };
> +
> + const uint8_t binary[] = { 0x82, 0x85, /* Binary header */
> + 0x74, 0x90, 0xb9, 0xdf, /* Masking key */
> + 0x03, 0xff, 0xcb, 0xb3, 0x10 /* "world" */ };
> + Chardev *chr_client = opaque;
> +
> + if (websock_check_http_headers((char *) buf, size)) {
> + qemu_chr_fe_write(chr_client->be, ping, sizeof(ping));
> + } else if (buf[0] == 0x8a && buf[1] == 0x05) {
> + g_assert(strncmp((char *) buf + 2, "hello", 5) == 0);
> + qemu_chr_fe_write(chr_client->be, binary, sizeof(binary));
> + } else {
> + g_assert(buf[0] == 0x88 && buf[1] == 0x16);
> + g_assert(strncmp((char *) buf + 4, "peer requested close", 10) == 0);
> + quit = true;
> + }
> +}
> +
> +
> +static int websock_client_can_read(void *opaque)
> +{
> + return 4096;
> +}
> +
> +
> +static void char_websock_test(void)
> +{
> + QObject *addr;
> + QDict *qdict;
> + const char *port;
> + char *tmp;
> + char *handshake_port;
> + CharBackend be;
> + CharBackend client_be;
> + Chardev *chr_client;
> + Chardev *chr = qemu_chr_new("server",
> + "websock:127.0.0.1:0,server,nowait");
> + const char handshake[] = "GET / HTTP/1.1\r\n"
> + "Upgrade: websocket\r\n"
> + "Connection: Upgrade\r\n"
> + "Host: localhost:%s\r\n"
> + "Origin: http://localhost:%s\r\n"
> + "Sec-WebSocket-Key: o9JHNiS3/0/0zYE1wa3yIw==\r\n"
> + "Sec-WebSocket-Version: 13\r\n"
> + "Sec-WebSocket-Protocol: binary\r\n\r\n";
> + const uint8_t close[] = { 0x88, 0x82, /* Close header */
> + 0xef, 0xaa, 0xc5, 0x97, /* Masking key */
> + 0xec, 0x42 /* Status code */ };
> +
> + addr = object_property_get_qobject(OBJECT(chr), "addr", &error_abort);
> + qdict = qobject_to(QDict, addr);
> + port = qdict_get_str(qdict, "port");
> + tmp = g_strdup_printf("tcp:127.0.0.1:%s", port);
> + handshake_port = g_strdup_printf(handshake, port, port);
> + qobject_unref(qdict);
> +
> + qemu_chr_fe_init(&be, chr, &error_abort);
> + qemu_chr_fe_set_handlers(&be, websock_server_can_read, websock_server_read,
> + NULL, NULL, chr, NULL, true);
> +
> + chr_client = qemu_chr_new("client", tmp);
> + qemu_chr_fe_init(&client_be, chr_client, &error_abort);
> + qemu_chr_fe_set_handlers(&client_be, websock_client_can_read,
> + websock_client_read,
> + NULL, NULL, chr_client, NULL, true);
> + g_free(tmp);
> +
> + qemu_chr_write_all(chr_client,
> + (uint8_t *) handshake_port,
> + strlen(handshake_port));
> + g_free(handshake_port);
> + main_loop();
> +
> + g_assert(object_property_get_bool(OBJECT(chr), "connected", &error_abort));
> + g_assert(object_property_get_bool(OBJECT(chr_client),
> + "connected", &error_abort));
> +
> + qemu_chr_write_all(chr_client, close, sizeof(close));
> + main_loop();
> +
> + object_unparent(OBJECT(chr_client));
> + object_unparent(OBJECT(chr));
> +}
> +
> +
> #ifndef _WIN32
> static void char_pipe_test(void)
> {
> @@ -829,6 +953,7 @@ int main(int argc, char **argv)
> g_test_add_func("/char/serial", char_serial_test);
> #endif
> g_test_add_func("/char/hotswap", char_hotswap_test);
> + g_test_add_func("/char/websocket", char_websock_test);
>
> return g_test_run();
> }
> --
> 2.17.1
>
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 :|
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] chardev: Add websocket support
2018-08-28 9:02 ` Daniel P. Berrangé
@ 2018-08-28 10:04 ` Julia Suvorova
2018-08-28 10:09 ` Daniel P. Berrangé
0 siblings, 1 reply; 11+ messages in thread
From: Julia Suvorova @ 2018-08-28 10:04 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Eric Blake, marcandre.lureau, Paolo Bonzini,
Stefan Hajnoczi, Stefan Hajnoczi, Joel Stanley, Jim Mussared,
Steffen Görtz
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 <jusual@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.
Best regards, Julia Suvorova.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] chardev: Add websocket support
2018-08-28 10:04 ` Julia Suvorova
@ 2018-08-28 10:09 ` Daniel P. Berrangé
2018-08-28 10:18 ` Julia Suvorova
0 siblings, 1 reply; 11+ messages in thread
From: Daniel P. Berrangé @ 2018-08-28 10:09 UTC (permalink / raw)
To: Julia Suvorova
Cc: qemu-devel, Eric Blake, marcandre.lureau, Paolo Bonzini,
Stefan Hajnoczi, Stefan Hajnoczi, Joel Stanley, Jim Mussared,
Steffen Görtz
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 <jusual@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.
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 :|
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] chardev: Add websocket support
2018-08-28 10:09 ` Daniel P. Berrangé
@ 2018-08-28 10:18 ` Julia Suvorova
2018-08-28 10:37 ` Daniel P. Berrangé
0 siblings, 1 reply; 11+ messages in thread
From: Julia Suvorova @ 2018-08-28 10:18 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Eric Blake, marcandre.lureau, Paolo Bonzini,
Stefan Hajnoczi, Stefan Hajnoczi, Joel Stanley, Jim Mussared,
Steffen Görtz
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 <jusual@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.
Best regards, Julia Suvorova.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] chardev: Add websocket support
2018-08-28 10:18 ` Julia Suvorova
@ 2018-08-28 10:37 ` Daniel P. Berrangé
0 siblings, 0 replies; 11+ messages in thread
From: Daniel P. Berrangé @ 2018-08-28 10:37 UTC (permalink / raw)
To: Julia Suvorova
Cc: qemu-devel, Eric Blake, marcandre.lureau, Paolo Bonzini,
Stefan Hajnoczi, Stefan Hajnoczi, Joel Stanley, Jim Mussared,
Steffen Görtz
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 <jusual@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 :|
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] chardev: Add websocket support
2018-08-27 18:41 ` [Qemu-devel] [PATCH v2 1/2] " Julia Suvorova
2018-08-28 9:02 ` Daniel P. Berrangé
@ 2018-10-03 14:34 ` Stefan Hajnoczi
1 sibling, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2018-10-03 14:34 UTC (permalink / raw)
To: Julia Suvorova
Cc: qemu-devel, Daniel P . Berrangé,
Eric Blake, marcandre.lureau, Paolo Bonzini, Stefan Hajnoczi,
Joel Stanley, Jim Mussared, Steffen Görtz
[-- Attachment #1: Type: text/plain, Size: 445 bytes --]
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=...
Please make user-facing interfaces use the name "websocket" instead of
"websock" for consistency. The official protocol name is WebSocket and
QEMU already exposes "websocket" to the user in qapi/ui.json and
qemu-options.hx documentation.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] tests/test-char: Check websocket chardev functionality
2018-08-27 18:41 ` [Qemu-devel] [PATCH v2 2/2] tests/test-char: Check websocket chardev functionality Julia Suvorova
2018-08-28 9:07 ` Daniel P. Berrangé
@ 2018-10-03 14:37 ` Stefan Hajnoczi
1 sibling, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2018-10-03 14:37 UTC (permalink / raw)
To: Julia Suvorova
Cc: qemu-devel, Daniel P . Berrangé,
Eric Blake, marcandre.lureau, Paolo Bonzini, Stefan Hajnoczi,
Joel Stanley, Jim Mussared, Steffen Görtz
[-- Attachment #1: Type: text/plain, Size: 664 bytes --]
On Mon, Aug 27, 2018 at 09:41:03PM +0300, Julia Suvorova wrote:
> Test order:
> Creating server websocket chardev
> Creating usual tcp chardev client
> Sending handshake message from client
> Receiving handshake reply
> Sending ping frame with "hello" payload
> Receiving pong reply
> Sending binary data "world"
> Checking the received data on server side
> Checking of closing handshake
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
> tests/test-char.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 125 insertions(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-10-03 14:37 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-27 18:41 [Qemu-devel] [PATCH v2 0/2] chardev: Add websocket support Julia Suvorova
2018-08-27 18:41 ` [Qemu-devel] [PATCH v2 1/2] " Julia Suvorova
2018-08-28 9:02 ` Daniel P. Berrangé
2018-08-28 10:04 ` Julia Suvorova
2018-08-28 10:09 ` Daniel P. Berrangé
2018-08-28 10:18 ` Julia Suvorova
2018-08-28 10:37 ` Daniel P. Berrangé
2018-10-03 14:34 ` Stefan Hajnoczi
2018-08-27 18:41 ` [Qemu-devel] [PATCH v2 2/2] tests/test-char: Check websocket chardev functionality Julia Suvorova
2018-08-28 9:07 ` Daniel P. Berrangé
2018-10-03 14:37 ` Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.