On Thu, May 24, 2018 at 05:06:19AM -0400, Frediano Ziglio wrote: > > > > On Wed, May 23, 2018 at 01:36:35PM -0400, Frediano Ziglio wrote: > > > Testing this, so far is working correctly: > > > > > > Subject: [PATCH spice-server] rcc: Make red_channel_client_is_connected > > > return > > > correct information during initialization > > > > > > Use stream->watch as flag to check if connected or not initializing > > > it during construction, not during init (after all hierarchy is built). > > > > > > If there are any failures during red_channel_client_initable_init(), > > red_channel_client_is_connected() will still return TRUE with this > > patch. > > > > In general, we should consider RedChannelClient and its derivative to be > > non-functional until red_channel_client_initable_init() successfully > > returns. This change makes RedChannelClient::is_connected become TRUE > > much earlier in the instantiation process, imo we should not make the > > object "look ready" until red_channel_client_initable_init() has > > returned, I'm worried we'd end up doing some eg network operations > > because the socket looks ready, while some other important things for > > the channel haven't been initialized yet. SndChannelClient is the only > > class trying to do network operations, or 'complex' stuff as part of its > > instantiation btw, maybe I should move more of it to _initable_init. > > > > With that said, and apart from that "early init" issue, the patch > > makes sense. > > > > Christophe > > > > Surely the "red_channel_client_is_connected" name and current behaviour > are wrong. If the check is for readiness, then should be renamed to > red_channel_client_is_ready, the channel in its initial state is already > "connected" as there is a client with a socket connected even before > g_object_new is even called. I'm not exactly saying the check shoud be for readiness. I agree that once foo_channel_client_new() has successfully returned, checking that there is an actual connection would make more sense than checking in the list, though both are probably roughly equivalent. Before foo_channel_client_new() returns though, the RedChannelClient instance is not fully initialized, and so calling methods on the instance could easily produce undefined behaviour. For example, even if the socket is already connected, an hypothetical red_channel_client_send_data() may or may not work as it could depend on RedChannelClient state which hasn't been initialized yet. So in such a situation, we would not want red_channel_client_is_connected() to return true, as for the user of RedChannelClient API, the network communication would fail, so it would not be much different from not having a connection. (just trying to clarify my thoughts, not sure there is anything actionable in the previous paragraphs ;) > OT, but not too much. Is red_channel_client_shutdown doing the > right thing? Is shutting down the socket but also removing the > watch which is supposed to trigger the client disconnection. > So if the watch is removed how the code will handle the disconnection? > red_channel_client_shutdown is not called that often, some uncommon > error in agent/vdi/char devices. Is this path of the code even > tested? Yes, this codepath is probably not tested much :-/ Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel