From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57210) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoX8q-0004GA-9I for qemu-devel@nongnu.org; Tue, 20 Oct 2015 09:39:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZoX8l-0004Dr-Dz for qemu-devel@nongnu.org; Tue, 20 Oct 2015 09:39:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50740) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoX8l-0004Dj-7Y for qemu-devel@nongnu.org; Tue, 20 Oct 2015 09:39:47 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id D5100C0AF785 for ; Tue, 20 Oct 2015 13:39:46 +0000 (UTC) Date: Tue, 20 Oct 2015 14:39:43 +0100 From: "Daniel P. Berrange" Message-ID: <20151020133943.GM14616@redhat.com> References: <1444648509-29179-1-git-send-email-berrange@redhat.com> <1444648509-29179-5-git-send-email-berrange@redhat.com> <56256C99.1000100@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <56256C99.1000100@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 04/16] ui: convert VNC startup code to use SocketAddress Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Paolo Bonzini , qemu-devel@nongnu.org, "Dr. David Alan Gilbert" On Mon, Oct 19, 2015 at 04:20:09PM -0600, Eric Blake wrote: > On 10/12/2015 05:14 AM, Daniel P. Berrange wrote: > > The VNC code is currently using QemuOpts to configure the > > sockets connections / listeners it needs. Convert it to > > use SocketAddress to bring it in line with modern QAPI > > based code elsewhere in QEMU. > > > > Signed-off-by: Daniel P. Berrange > > --- > > ui/vnc.c | 161 ++++++++++++++++++++++++++++++++++++--------------------------- > > 1 file changed, 91 insertions(+), 70 deletions(-) > > > > @@ -3539,44 +3535,83 @@ void vnc_display_open(const char *id, Error **errp) > > return; > > } > > > > - 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; > > size_t hlen = h - vnc; > > > > - if (vnc[0] == '[' && vnc[hlen - 1] == ']') { > > - host = g_strndup(vnc + 1, hlen - 2); > > + const char *websocket = qemu_opt_get(opts, "websocket"); > > + int to = qemu_opt_get_number(opts, "to", 0); > > We really ought to improve the qapi notion of InetSocketAddress to allow > { 'alternate':'StrOrInt', 'data': { 'i':'int', 's':'str' } } rather than > only 'str' for port. But that's a project for another day. I guess > things get a bit weird if "to" is present but doesn't parse as a number, > but I don't think your patch is making things any worse. > > > + > > + if (strncmp(vnc, "unix:", 5) == 0) { > > + saddr->kind = SOCKET_ADDRESS_KIND_UNIX; > > + saddr->q_unix = g_new0(UnixSocketAddress, 1); > > More clashes with my pending qapi patches; rebase fun for one of us :) > > > > + if (to) { > > + saddr->inet->has_to = true; > > + saddr->inet->to = to; > > + } > > + saddr->inet->ipv4 = saddr->inet->has_ipv4 = has_ipv4; > > + saddr->inet->ipv6 = saddr->inet->has_ipv6 = has_ipv6; > > Do we want to specify has_ipvX as true even when setting inet->ipvX to > false? This saddr instance is passed into 'socket_connect' which then converts it back into a QemuOpts object using bool ipv4 = addr->has_ipv4 && addr->ipv4; So, we don't need to set has_ipvX to true, when ipvX is false. I'll be so happy to finally kill QemuOpts from the sockets code and use SocketAddress everywhere.... > > > > @@ -3770,37 +3795,32 @@ void vnc_display_open(const char *id, Error **errp) > > int csock; > > vs->lsock = -1; > > vs->lwebsock = -1; > > - if (strncmp(vnc, "unix:", 5) == 0) { > > - csock = unix_connect(vnc+5, errp); > > - } else { > > - csock = inet_connect(vnc, errp); > > + if (vs->ws_enabled) { > > + error_setg(errp, "Cannot use websockets in reverse mode"); > > + goto fail; > > } > > + csock = socket_connect(saddr, errp, > > + NULL, NULL); > > Looks like the line-wrapping isn't needed here. > > Overall, looks like a sane conversion. > > Reviewed-by: Eric Blake Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|