Add basic support for server-side

[proton-decay]

This is basic support for server-side of the protocol.

I tried to make it similar to client side but could not understand the need for extra thread. At least on server side I think it is not necessary.

Plus: support for ExtendedKeyEvent extension.

[whitequark]

This is a very high quality PR, thanks! I think there are a few improvements that could be done to it, though:

• Can you add support for ExtendedKeyEvent on the client side? It seems only logical.
• I'm rather wary of exposing the raw TCP stream, it seems like poor API design to me. Can you perhaps add the encoding logic to the server side like it is done on the client side? You will need to handle it in any case, and it's better to have it all in one place.

[proton-decay]

Yeah, I don't really like that too. I'll check the client side.

[proton-decay]

Pushed new proposition. It's just a sketch. Places with XXX have to be finished.

If you like the new approach I will finish this and add unit tests to be sure it works because I don't use these encodings in my project.

[proton-decay]

And adding ExtendedKeyEvent on the client side is more complex. Client has to send its supported pseudo-encodings. Then server replies if it supports this encoding and extensions may be used only if server confirmed it supports it.

[whitequark]

So, this Update enum is rather ugly. It does not afford us any opportunity for error-checking or choice of internal representation, and currently, adding variants to it breaks the API... I have a better proposition.

Add a builder for framebuffer updates. This will negate the need for a check (you could keep the API contract by hiding the internals), it will be possible to e.g. just call a method and have rust-vnc decide if raw pixels or ZRLE are the correct way to compress, or do any other kind of state tracking. So the API should be used something like the following:

let update = server::FramebufferUpdate::new();
update.add_raw_pixels(rect, pixel_data);
update.add_zrle_pixels(rect, pixel_data, compression_level); // same pixel data! rust-vnc does the compression
update.add_set_cursor(...);
server.send_framebuffer_update(&update);

[whitequark]

And adding ExtendedKeyEvent on the client side is more complex. Client has to send its supported pseudo-encodings. Then server replies if it supports this encoding and extensions may be used only if server confirmed it supports it.

Ah, okay then, disregard it for this PR.

[proton-decay]

Yes, builder will be much better. But I don't see why rust-vnc would ever want to decide if data should be compressed or track any state. It should be up to high level logic in server to do that.

[whitequark]

But I don't see why rust-vnc would ever want to decide if data should be compressed or track any state. It should be up to high level logic in server to do that.

I think most servers want to just throw pixels at the underlying library and expect it to do something sensible. But even barring that, right now, the client API is misuse-resistant--you can't violate the wire protocol with it. I think having the server API be misuse-resistant would be great as well--so for example it ought to track which encodings the client understands, and:

• if the client doesn't understand cursor updates, then calling update.add_set_cursor should be a no-op
• if the client doesn't understand ZRLE, then calling update.add_zrle_pixels should be an error, and update.add_pixels should not choose ZRLE,
• etc.

This is what you're going to have to do in every server (to a larger or smaller degree), why not write the code once?

[proton-decay]

New version pushed.

I wanted to add tests for checking interoperability of Server and Client. I can see two solutions:

• make Server and Client generic over Read + Write and find something that could be used as mock
• use real TcpStream. This doesn't sound good, but that's how it is tested in libstd https://doc.rust-lang.org/src/std/net/tcp.rs.html#762

Any better solutions? Define GenericServer<T: Read+Write> and pub type Server = GenericServer<TcpStream>?

[whitequark]

use real TcpStream. This doesn't sound good

I don't think it's troublesome in any way.

[whitequark]

To avoid exposing Update I would have to add another intermediate struct FrambufferUpdateBuilder and use it like this:

Ah. Actually, that wouldn't work because that would still let you to send an update that's not valid in the current state of the server. Let's just use a closure:

server.send_update(|update| {
  update.add_pixels(...)
});

[proton-decay]

How is the send_update supposed to work in this case?

[whitequark]

How is the send_update supposed to work in this case?

It creates a FramebufferUpdate internally and passes a &mut FramebufferUpdate into the closure. This means that a FramebufferUpdate is never desynced with the actual server state.

[elmarco]

fwiw, I have updated the series https://github.com/elmarco/rust-vnc/tree/server