"Daniel P. Berrange" <berra...@redhat.com> writes: > On Tue, Oct 20, 2015 at 04:53:24PM -0600, Eric Blake wrote: >> On 10/20/2015 08:46 AM, Markus Armbruster wrote: >> > Gerd Hoffmann <kra...@redhat.com> writes: >> > >> >> 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? >> > >> > Fine with me. Could also call it _init_ instead of _fill_. >> >> I like init a bit better than fill. >> >> > >> >>> Outside this patch's scope, but since I'm looking at the code already... >> >> I'm guessing that also means that fixing it is outside this series' scope. >> >> >>> 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 ... >> > >> > 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. >> > >> > We could avoid failures in vnc_qmp_event() as follows: >> > >> > 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(). >> > >> > 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? > > My pending IO channel patches do exactly this > > Take a look at the QIOChannelSocket impl > > https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg03440.html > > This caches the results of getpeername & getsockname in the > QOIChannelSocket object. > > So my patches which convert VNC to use QIOChannelSOcket should solve > this particular failure scenario you're discussing.
Great! No need to add a FIXME comment then.
