Message ID | 1425986326-28797-4-git-send-email-kraxel@redhat.com |
---|---|
State | New |
Headers | show |
On 10/03/2015 12:18, Gerd Hoffmann wrote: > Use inet_listen_opts instead of inet_listen. Allows us to drop some > pointless indirection: Format strings just to parse them again later on. Maybe use socket_listen then? Paolo
On 2015/3/10 19:18, Gerd Hoffmann wrote: > Use inet_listen_opts instead of inet_listen. Allows us to drop some > pointless indirection: Format strings just to parse them again later on. > Nice improvement. :) > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > ui/vnc.c | 75 +++++++++++++++++++++++++++++++++++----------------------------- > 1 file changed, 41 insertions(+), 34 deletions(-) > > diff --git a/ui/vnc.c b/ui/vnc.c > index bac44ce..2510c15 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -3322,13 +3322,14 @@ void vnc_display_open(const char *id, Error **errp) > { > VncDisplay *vs = vnc_display_find(id); > QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id); > + QemuOpts *sopts, *wsopts; > const char *share, *device_id; > QemuConsole *con; > bool password = false; > bool reverse = false; > const char *vnc; > const char *has_to; > - char *display, *ws_display = NULL, *to = NULL; > + char *h; > bool has_ipv4 = false; > bool has_ipv6 = false; > #ifdef CONFIG_VNC_WS > @@ -3361,16 +3362,36 @@ void vnc_display_open(const char *id, Error **errp) > return; > } > > - has_to = qemu_opt_get(opts, "to"); > - if (has_to) { > - to = g_strdup_printf(",to=%s", has_to); > + sopts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort); > + wsopts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort); > + > + h = strrchr(vnc, ':'); > + if (h) { > + char *host = g_strndup(vnc, h - vnc); > + qemu_opt_set(sopts, "host", host); > + qemu_opt_set(wsopts, "host", host); > + qemu_opt_set(sopts, "port", h+1); Maybe you missed passing '&error_abort' or something like that: ui/vnc.c:3371: error: too few arguments to function ‘qemu_opt_set’ ui/vnc.c:3372: error: too few arguments to function ‘qemu_opt_set’ ui/vnc.c:3373: error: too few arguments to function ‘qemu_opt_set’ ui/vnc.c:3384: error: too few arguments to function ‘qemu_opt_set’ as the same as below several places of calling qemu_opt_set(). Regards, -Gonglei > + g_free(host); > + } else { > + error_setg(errp, "no vnc port specified"); > + goto fail; > } > + > + has_to = qemu_opt_get(opts, "to"); > has_ipv4 = qemu_opt_get_bool(opts, "ipv4", false); > has_ipv6 = qemu_opt_get_bool(opts, "ipv6", false); > - display = g_strdup_printf("%s%s%s%s", vnc, > - has_to ? to : "", > - has_ipv4 ? ",ipv4" : "", > - has_ipv6 ? ",ipv6" : ""); > + if (has_to) { > + qemu_opt_set(sopts, "to", has_to); > + qemu_opt_set(wsopts, "to", has_to); > + } > + if (has_ipv4) { > + qemu_opt_set(sopts, "ipv4", "on"); > + qemu_opt_set(wsopts, "ipv4", "on"); > + } > + if (has_ipv6) { > + qemu_opt_set(sopts, "ipv6", "on"); > + qemu_opt_set(wsopts, "ipv6", "on"); > + } > > password = qemu_opt_get_bool(opts, "password", false); > if (password && fips_get_state()) { > @@ -3423,19 +3444,9 @@ void vnc_display_open(const char *id, Error **errp) > #ifdef CONFIG_VNC_WS > websocket = qemu_opt_get(opts, "websocket"); > if (websocket) { > - /* extract the host specification from display */ > - char *host = NULL, *host_end = NULL; > vs->ws_enabled = true; > + qemu_opt_set(wsopts, "port", websocket); > > - /* ipv6 hosts have colons */ > - host_end = strrchr(display, ':'); > - if (host_end) { > - host = g_strndup(display, host_end - display + 1); > - } else { > - host = g_strdup(":"); > - } > - ws_display = g_strconcat(host, websocket, NULL); > - g_free(host); > } > #endif /* CONFIG_VNC_WS */ > > @@ -3605,10 +3616,10 @@ void vnc_display_open(const char *id, Error **errp) > #ifdef CONFIG_VNC_WS > vs->lwebsock = -1; > #endif > - if (strncmp(display, "unix:", 5) == 0) { > - csock = unix_connect(display+5, errp); > + if (strncmp(vnc, "unix:", 5) == 0) { > + csock = unix_connect(vnc+5, errp); > } else { > - csock = inet_connect(display, errp); > + csock = inet_connect(vnc, errp); > } > if (csock < 0) { > goto fail; > @@ -3616,19 +3627,17 @@ void vnc_display_open(const char *id, Error **errp) > vnc_connect(vs, csock, false, false); > } else { > /* listen for connects */ > - if (strncmp(display, "unix:", 5) == 0) { > - vs->lsock = unix_listen(display+5, NULL, 0, errp); > + if (strncmp(vnc, "unix:", 5) == 0) { > + vs->lsock = unix_listen(vnc+5, NULL, 0, errp); > vs->is_unix = true; > } else { > - vs->lsock = inet_listen(display, NULL, 0, > - SOCK_STREAM, 5900, errp); > + vs->lsock = inet_listen_opts(sopts, 5900, errp); > if (vs->lsock < 0) { > goto fail; > } > #ifdef CONFIG_VNC_WS > if (vs->ws_enabled) { > - vs->lwebsock = inet_listen(ws_display, NULL, 0, > - SOCK_STREAM, 0, errp); > + vs->lwebsock = inet_listen_opts(wsopts, 0, errp); > if (vs->lwebsock < 0) { > if (vs->lsock) { > close(vs->lsock); > @@ -3639,27 +3648,25 @@ void vnc_display_open(const char *id, Error **errp) > } > #endif /* CONFIG_VNC_WS */ > } > - g_free(to); > - g_free(display); > vs->enabled = true; > qemu_set_fd_handler2(vs->lsock, NULL, > vnc_listen_regular_read, NULL, vs); > #ifdef CONFIG_VNC_WS > - g_free(ws_display); > if (vs->ws_enabled) { > qemu_set_fd_handler2(vs->lwebsock, NULL, > vnc_listen_websocket_read, NULL, vs); > } > #endif /* CONFIG_VNC_WS */ > } > + qemu_opts_del(sopts); > + qemu_opts_del(wsopts); > return; > > fail: > - g_free(to); > - g_free(display); > + qemu_opts_del(sopts); > + qemu_opts_del(wsopts); > vs->enabled = false; > #ifdef CONFIG_VNC_WS > - g_free(ws_display); > vs->ws_enabled = false; > #endif /* CONFIG_VNC_WS */ > } >
On Di, 2015-03-10 at 16:50 +0100, Paolo Bonzini wrote: > > On 10/03/2015 12:18, Gerd Hoffmann wrote: > > Use inet_listen_opts instead of inet_listen. Allows us to drop some > > pointless indirection: Format strings just to parse them again later on. > > Maybe use socket_listen then? Hmm, mapping "vnc qemuopts" -> "socket qemuopts" looks a bit easier than mapping "vnc qemuopts" -> SocketAddress, which will then be translated into "socket qemuopts" anyway ... cheers, Gerd
Hi, > > + qemu_opt_set(sopts, "port", h+1); > > Maybe you missed passing '&error_abort' or something like that: > > ui/vnc.c:3371: error: too few arguments to function ‘qemu_opt_set’ > ui/vnc.c:3372: error: too few arguments to function ‘qemu_opt_set’ > ui/vnc.c:3373: error: too few arguments to function ‘qemu_opt_set’ > ui/vnc.c:3384: error: too few arguments to function ‘qemu_opt_set’ > > as the same as below several places of calling qemu_opt_set(). Looks like a a error handling series from markus was merged meanwhile, need to rebase & adapt patch. cheers, Gerd
diff --git a/ui/vnc.c b/ui/vnc.c index bac44ce..2510c15 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -3322,13 +3322,14 @@ void vnc_display_open(const char *id, Error **errp) { VncDisplay *vs = vnc_display_find(id); QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id); + QemuOpts *sopts, *wsopts; const char *share, *device_id; QemuConsole *con; bool password = false; bool reverse = false; const char *vnc; const char *has_to; - char *display, *ws_display = NULL, *to = NULL; + char *h; bool has_ipv4 = false; bool has_ipv6 = false; #ifdef CONFIG_VNC_WS @@ -3361,16 +3362,36 @@ void vnc_display_open(const char *id, Error **errp) return; } - has_to = qemu_opt_get(opts, "to"); - if (has_to) { - to = g_strdup_printf(",to=%s", has_to); + sopts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort); + wsopts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort); + + h = strrchr(vnc, ':'); + if (h) { + char *host = g_strndup(vnc, h - vnc); + qemu_opt_set(sopts, "host", host); + qemu_opt_set(wsopts, "host", host); + qemu_opt_set(sopts, "port", h+1); + g_free(host); + } else { + error_setg(errp, "no vnc port specified"); + goto fail; } + + has_to = qemu_opt_get(opts, "to"); has_ipv4 = qemu_opt_get_bool(opts, "ipv4", false); has_ipv6 = qemu_opt_get_bool(opts, "ipv6", false); - display = g_strdup_printf("%s%s%s%s", vnc, - has_to ? to : "", - has_ipv4 ? ",ipv4" : "", - has_ipv6 ? ",ipv6" : ""); + if (has_to) { + qemu_opt_set(sopts, "to", has_to); + qemu_opt_set(wsopts, "to", has_to); + } + if (has_ipv4) { + qemu_opt_set(sopts, "ipv4", "on"); + qemu_opt_set(wsopts, "ipv4", "on"); + } + if (has_ipv6) { + qemu_opt_set(sopts, "ipv6", "on"); + qemu_opt_set(wsopts, "ipv6", "on"); + } password = qemu_opt_get_bool(opts, "password", false); if (password && fips_get_state()) { @@ -3423,19 +3444,9 @@ void vnc_display_open(const char *id, Error **errp) #ifdef CONFIG_VNC_WS websocket = qemu_opt_get(opts, "websocket"); if (websocket) { - /* extract the host specification from display */ - char *host = NULL, *host_end = NULL; vs->ws_enabled = true; + qemu_opt_set(wsopts, "port", websocket); - /* ipv6 hosts have colons */ - host_end = strrchr(display, ':'); - if (host_end) { - host = g_strndup(display, host_end - display + 1); - } else { - host = g_strdup(":"); - } - ws_display = g_strconcat(host, websocket, NULL); - g_free(host); } #endif /* CONFIG_VNC_WS */ @@ -3605,10 +3616,10 @@ void vnc_display_open(const char *id, Error **errp) #ifdef CONFIG_VNC_WS vs->lwebsock = -1; #endif - if (strncmp(display, "unix:", 5) == 0) { - csock = unix_connect(display+5, errp); + if (strncmp(vnc, "unix:", 5) == 0) { + csock = unix_connect(vnc+5, errp); } else { - csock = inet_connect(display, errp); + csock = inet_connect(vnc, errp); } if (csock < 0) { goto fail; @@ -3616,19 +3627,17 @@ void vnc_display_open(const char *id, Error **errp) vnc_connect(vs, csock, false, false); } else { /* listen for connects */ - if (strncmp(display, "unix:", 5) == 0) { - vs->lsock = unix_listen(display+5, NULL, 0, errp); + if (strncmp(vnc, "unix:", 5) == 0) { + vs->lsock = unix_listen(vnc+5, NULL, 0, errp); vs->is_unix = true; } else { - vs->lsock = inet_listen(display, NULL, 0, - SOCK_STREAM, 5900, errp); + vs->lsock = inet_listen_opts(sopts, 5900, errp); if (vs->lsock < 0) { goto fail; } #ifdef CONFIG_VNC_WS if (vs->ws_enabled) { - vs->lwebsock = inet_listen(ws_display, NULL, 0, - SOCK_STREAM, 0, errp); + vs->lwebsock = inet_listen_opts(wsopts, 0, errp); if (vs->lwebsock < 0) { if (vs->lsock) { close(vs->lsock); @@ -3639,27 +3648,25 @@ void vnc_display_open(const char *id, Error **errp) } #endif /* CONFIG_VNC_WS */ } - g_free(to); - g_free(display); vs->enabled = true; qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_regular_read, NULL, vs); #ifdef CONFIG_VNC_WS - g_free(ws_display); if (vs->ws_enabled) { qemu_set_fd_handler2(vs->lwebsock, NULL, vnc_listen_websocket_read, NULL, vs); } #endif /* CONFIG_VNC_WS */ } + qemu_opts_del(sopts); + qemu_opts_del(wsopts); return; fail: - g_free(to); - g_free(display); + qemu_opts_del(sopts); + qemu_opts_del(wsopts); vs->enabled = false; #ifdef CONFIG_VNC_WS - g_free(ws_display); vs->ws_enabled = false; #endif /* CONFIG_VNC_WS */ }
Use inet_listen_opts instead of inet_listen. Allows us to drop some pointless indirection: Format strings just to parse them again later on. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- ui/vnc.c | 75 +++++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 41 insertions(+), 34 deletions(-)