xrdpapi: Add a way to monitor connect/disconnect events

[matt335672]

Fixes #3622

Demo program included.

@vscfreire - this is a draft for now. I'd very much appreciate any comments you have. The API is a little different from what we discussed, and you may have a different (and better) mental image of how this all should work.

[vscfreire]

Hi,

First of all, apologies for the delay.

I think this is a possible solution even if it is different from what we've discussed. My main issue, and please correct me if I am wrong, is that we might not be using virtual channels appropriately here. As I understand it, virtual channels are intended for sending and receiving data between the RDP client and server, however this implementation seems to go against that for two reasons:

1. This channel is not something that is requested by the RDP client
2. It is not possible to send data to that channel (in the chansrv changes there is a specific if clause to prevent that)

From my perspective, this should be something that is separate from virtual channel communication. The quickest solution that comes into my mind is using a separate socket for this such as xrdpapi_$DISPLAY_management, however I am not sure how great of an undertaking this is.

Please do let me know your thoughts.

Thanks.

[matt335672]

@vscfreire - as ever, thanks for the feedback and an interesting discussion. No apologies are necessary.

Am I right in that your concerns are related to the implementation detail only? I don't think the choice of implementation would affect the API as presented to the user.

At an OS level, the sockets for each channel (including this one) are all separate anyway. I'd thought when I started looking at this that all the channels would be multiplexed over a single socket, but that's not the implementation that was chosen by the original developer. Each channel ends up with its own socket. The file system entry xrdpapi_$DISPLAY is simply used as an endpoint for the client to connect to.

It would be simple enough to modify the initial PDU sent to an open xrdpapi socket to include a value specifying whether or not this was a private channel of some sort. That would prevent the user being able to open xrdp/SessInfo directly, if this is a concern, and effectively separates all the sockets out from each other.

Let me know if that would help.

I've one other question; having thought about this myself in the last few days, I was expecting you to come up with a question regarding how the user determines the initial state of the RDP client connection when running an xrdpapi client up. Is this a concern of yours? If so, what sort of mechanism would you like to see to get this information at an API level?

[vscfreire]

Hi,

Am I right in that your concerns are related to the implementation detail only? I don't think the choice of implementation would affect the API as presented to the user.

Yes. In terms of functionality this should work.

It would be simple enough to modify the initial PDU sent to an open xrdpapi socket to include a value specifying whether or not this was a private channel of some sort. That would prevent the user being able to open xrdp/SessInfo directly, if this is a concern, and effectively separates all the sockets out from each other.

I think this is a great idea.

I've one other question; having thought about this myself in the last few days, I was expecting you to come up with a question regarding how the user determines the initial state of the RDP client connection when running an xrdpapi client up. Is this a concern of yours? If so, what sort of mechanism would you like to see to get this information at an API level?

From my perspective, the xrdpapi client first tries to use WTSRegisterSessionNotificationEx to begin listening to session events. Afterwards, if successful, the client could use an implementation of WTSQuerySessionInformationA to get the initial state of the connection. Chansrv can probably store the connection state internally. Even if the client disconnected when WTSQuerySessionInformationA got an initial connect state the client would be aware of that disconnection thanks to WTSRegisterSessionNotificationEx. Let me know your thoughts on this.

Another question I have is how the client should handle a WTSRegisterSessionNotificationEx fail. For my use case, I think it would be perfectly acceptable that if the client starts up before chansrv has initialized the virtual channels (causing WTSRegisterSessionNotificationEx to fail) the client can simply wait a bit before attempting again. However, since WTSVirtualChannelOpenEx can fail for different reasons (such as the display not being set) it might be helpful to have different error values as there are some situations where a retry might be useless (when the display is not set for instance).

Thanks.

[matt335672]

OK - I'll add a private channels flag.

WTSQuerySessionInformationA should be easy enough to do. The simplest implementation might be to send an event immediately after the channel is opened to prime the client with the current state. This event would not be returned to the client and would be consumed before the WTSRegisterSessionNotificationEx call returns. Subsequent events would be cached before the client callback is issued.

I see the problem with WTSRegisterSessionNotificationEx. Since there's a race beween chansrv starting and the session starting, this could easily happen.

We could implement a special function WTSGetLastError() maybe which would return a restricted subset of values for errors. Or we could return an error from the WTS* functions. The WTSGetLastError() function wouldn't be thread-safe, unlike the Windows GetLastError(). Is that a problem do you think?

[vscfreire]

Hi,

I can't think of a reason as to why that would be a problem for this use case, but I also don't see the benefit of using that over returning an error from the WTS functions. Could you clarify why implementing WTSGetLastError() might be better?

Thanks

[matt335672]

I'm not really fussed either way myself. The WTSGetLastError() would allow for a code flow more like the Windows API, but that's not a big consideration. Do you have a preference?

[vscfreire]

Hi,

To simplify for now, I think it is best to just stick with returning an error error from the WTS functions

[matt335672]

Updated as requested, with minimal testing so far!

The error return was trickier to do than I expected. Because Windows uses true for OK, I've only got the value 0 for an error return, so as to preserve some kind of compatibility. Consequently I've added an optional error code to some of the calls.

Let me know what you think. We're still in draft at the moment.

[matt335672]

As a small note, I had a think about this last night, and I'm sure the interface between xrdpapi and chansrv can be improved somewhat.

That's not related to the API however. I'll save more updates until my next batch of changes.

[vscfreire]

Hi,

Apologies for the delay.

It seems to me this is exactly what I am looking for. When I get the chance, I will test this code in my custom channel.

Thanks for all the hard work

[matt335672]

That's great @vscfreire

I'll currently looking at a couple of other issues, but when I've got those done I'll rework the internal API. That's not going to affect xrdpapi however, so we should be able to work in parallel.

[matt335672]

Internal API reworked to remove 'Windows' definitions from chansrv.