From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58849) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zofmj-0003Ao-1w for qemu-devel@nongnu.org; Tue, 20 Oct 2015 18:53:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zofmf-0001QJ-Qp for qemu-devel@nongnu.org; Tue, 20 Oct 2015 18:53:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53386) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zofmf-0001QD-In for qemu-devel@nongnu.org; Tue, 20 Oct 2015 18:53:33 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 0BF4CC0AF78B for ; Tue, 20 Oct 2015 22:53:32 +0000 (UTC) References: <1444968943-11254-1-git-send-email-eblake@redhat.com> <1444968943-11254-5-git-send-email-eblake@redhat.com> <87h9lmat1b.fsf@blackfin.pond.sub.org> <1445331261.13733.65.camel@redhat.com> <87io614myv.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <5626C5E4.1010308@redhat.com> Date: Tue, 20 Oct 2015 16:53:24 -0600 MIME-Version: 1.0 In-Reply-To: <87io614myv.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="JOikr4XfIJW1w1rkT1taRrWKjAhpmdoJR" Subject: Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , Gerd Hoffmann Cc: qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --JOikr4XfIJW1w1rkT1taRrWKjAhpmdoJR Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/20/2015 08:46 AM, Markus Armbruster wrote: > Gerd Hoffmann writes: >=20 >> Hi, >> >>>> -static VncBasicInfo *vnc_basic_info_get(struct sockaddr_storage *sa= , >>>> - socklen_t salen) >>>> +static void vnc_basic_info_get(struct sockaddr_storage *sa, >>>> + socklen_t salen, >>>> + VncBasicInfo *info, >>>> + Error **errp) >> >>> The function no longer "gets info", it merely initializes it. Rename= it >>> accordingly? Gerd? >> >> vnc_fill_basic_info maybe? >=20 > Fine with me. Could also call it _init_ instead of _fill_. I like init a bit better than fill. >=20 >>> Outside this patch's scope, but since I'm looking at the code already= =2E.. I'm guessing that also means that fixing it is outside this series' scope= =2E >>> When vnc_server_info_get() fails, the event is dropped. Why is that >>> okay? Failure seems unlikely, though. >> >> Suggestions what else to do? I don't wanna crash qemu by calling >> qapi_event_send_vnc_* with a NULL pointer. And, yes, it should be >> highly unlikely so trying some more sophisticated error handling would= >> probably be dead code ... >=20 > These events signal a state change. Dropping them make me feel uneasy,= > because if a client uses them to track state, it gets out of sync. Events are already best-effort; clients have to be prepared to miss an event - but that's mainly when reconnecting (such as across libvirtd restarts), and not while the monitor is reliably connected. > The events need to identify the server to be of any use for state > tracking. Currently, this is members host, service, family of data > member server. >=20 > We could avoid failures in vnc_qmp_event() as follows: >=20 > 1. When we create a server, we obtain its info with getsockname() and > getnameinfo(). If they fail, we fail server creation. Else, we > store the info for vnc_qmp_event(). >=20 > 2. When a client connects, we obtain its info with getpeername() and > getnameinfo(). If they fail, we refuse the connection. Else, we > store the infor for vnc_qmp_event(). Seems reasonable to me, but starts to be out of scope for what I'm currently tackling, so is this something I can hand to you, Gerd? >=20 > Alternatively, we can embrace the failures and send the events without > the information we failed to get. Requires another way to identify the= > server. VncDisplay's id, perhaps? Might be nice to have anyway. Yes, including the id as a new field to the event sounds like a reasonable addition, whether or not my next patch reworking things to drop info->base makes it impossible to pass qmp_event_send_vnc_* a NULL pointer, even if we couldn't resolve useful information to display. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --JOikr4XfIJW1w1rkT1taRrWKjAhpmdoJR Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWJsXkAAoJEKeha0olJ0Nq+r4H/1jc85t+j+Px9VsTB21VK/S5 yAK6G57SWdZzurVNE2Sxikubw3bnPu9YiBXfNgBy7zLi18oQ1+cLLw6bpWdHyf78 wQPhBZjf/86PKB+eGyV1QmzXE/GklDYBBmCWGEQbC5Y7u3F4CNM8ULsxCCT7WsHb YXuiMndgmhLFFmuriq0NYLAoQeGSZU3s/sblcP/EQ8dyYRLICv1Y9I/jzAji9xfN YK7UXPe1vrLG5S7Nv5/Y6ulxiUF7UPmKRtvOd4C792D0rPCBLO981Ni1U6JFjuwY BZITM+zUeZoS3SeeI0wwAeQzT/ea1mnw2E6yhcpnqPifbxzgkcEUbtHD4+hbJ3A= =SZ5G -----END PGP SIGNATURE----- --JOikr4XfIJW1w1rkT1taRrWKjAhpmdoJR--