Add basic support for server-side #4

proton-decay · 2017-06-18T16:19:16Z

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 · 2017-06-18T16:57:52Z

src/protocol.rs

+                        let keycode = try!(reader.read_u32::<BigEndian>());
+                        Ok(C2S::ExtendedKeyEvent { down: down, keysym: keysym, keycode: keycode })
+                    }
+                    _ => Err(Error::Unexpected("server to client QEMU submessage type"))


is it QEMU specifically?

Yes, according to http://vncdotool.readthedocs.io/en/latest/rfbproto.html#qemu-extended-key-event-message and some history background here: https://www.berrange.com/tags/key-codes/

My god, what a disaster.

I think it's worth adding a comment indicating the source for the extended message formats.

whitequark · 2017-06-18T17:04:41Z

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 · 2017-06-18T17:14:32Z

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

proton-decay · 2017-06-18T20:25:19Z

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 · 2017-06-18T20:35:44Z

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

A few issues with this kind of API, plus there's a bigger one I'll write up separately.

whitequark · 2017-06-18T20:31:33Z

src/server.rs

+                if expected_num_bytes == pixel_data.len() {
+                    Ok(())
+                } else {
+                    let msg = format!("Expected data length for rectangle {:?} \


This should just be a panic, no need to have a separate error type for a trivially avoidable contract violation.

Slightly disagree. But actually ok.

whitequark · 2017-06-18T20:32:40Z

src/server.rs

@@ -101,9 +99,93 @@ pub enum Event {
    },
 }

+/// Data structure containing data to be sent by server in messages containing rectangles.
+#[derive(Debug)]
+pub enum Rectangle<'a> {


A more fitting name for this would be an Update, since half of the enum variants have nothing to do with rectangles at all.

whitequark · 2017-06-18T20:34:42Z

src/server.rs

+        }
+    }
+
+    fn write_rect_to<W: Write>(rect: &Rect, writer: &mut W) -> Result<()> {


This should definitely be a part of an impl on Rect, not here.

whitequark · 2017-06-18T20:36:29Z

src/server.rs

-        Ok((Server { stream: stream }, client_init.shared))
+        Ok((Server {
+            stream: stream,
+            bytes_per_pixel: (pixel_format.bits_per_pixel as u16 + 7) / 8,


Maybe factor out the conversion into an impl on PixelFormat?

whitequark · 2017-06-18T20:37:36Z

src/server.rs

-        Ok(())
-    }
+    /// This is all-or-nothing method. Rectangles will be sent only if all rectangles are valid.
+    pub fn send_framebuffer_update(&mut self, rectangles: &[Rectangle]) -> Result<()> {


send_update(&mut self, updates: &[Update]), as per comment above

whitequark · 2017-06-18T20:38:13Z

src/server.rs

-    pub fn send_raw_data(&mut self, data: &[u8]) -> Result<()> {
-        try!(self.stream.write_all(data));
+    /// Sends `FramebufferUpdate` message without checking validity of its contents.
+    pub fn send_framebuffer_update_unchecked(&mut self, rectangles: &[Rectangle]) -> Result<()> {


Do we need this? The check is not expensive at all, especially compared to cost of I/O. This just clutters the API.

Well, ok. Agree.

whitequark · 2017-06-18T20:38:26Z

src/server.rs

-            encoding: encoding,
-        }.write_to(&mut self.stream));
+        // Send the message
+        let count = rectangles.len() as u16;


Use a checked conversion.

whitequark · 2017-06-18T20:42:37Z

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 · 2017-06-18T20:43:12Z

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 · 2017-06-18T20:57:58Z

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 · 2017-06-18T21:04:55Z

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 · 2017-06-19T18:12:16Z

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 · 2017-06-19T18:20:26Z

use real TcpStream. This doesn't sound good

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

whitequark

Okay, this is almost it! Just a few minor issues left.

whitequark · 2017-06-19T18:21:04Z

src/lib.rs

+        }
+    }
+
+    /// Writes `Rect` to given stream.


Also factor out reading a Rect, perhaps, for consistency.

With Message trait? This is not a message. Without? That method would be unused generating warning.

We already use the Message trait for parts of messages as well: https://github.com/whitequark/rust-vnc/blob/master/src/protocol.rs#L486-L514

Ok, I'll move to to protocol and reexport in lib.

whitequark · 2017-06-19T18:21:44Z

src/lib.rs

+    }
+
+    /// Constructs new zero-sized `Rect` placed at (0, 0).
+    pub fn new_empty() -> Self {


Why isn't this a constant?

Why should it be? It is jet another way of construction just like new. Compiler should optimize it so in binary code it will be just in-place initialization.

Because if it's a constant then you can use it in other constants. Why have methods that always return the same value by design?

Ok, both approaches have small pros and cons.

What's the pros of new_empty()? They should result in exact same machine code...

whitequark · 2017-06-19T18:21:51Z

src/protocol.rs

@@ -237,6 +237,40 @@ pub struct PixelFormat {
    pub blue_shift:     u8,
 }

+impl PixelFormat {
+    /// Creates RGB pixel format with 4 bytes per pixel and 3 bytes of depth.
+    pub fn new_rgb8888() -> Self {


These two also should be constants.

The same argument as in Rect::new_empty().

whitequark · 2017-06-19T18:22:26Z

src/protocol.rs

@@ -513,17 +547,17 @@ impl Message for C2S {
 }

 #[derive(Debug)]
-pub struct Rectangle {
+pub struct RectangleHeader {


This renaming is no longer necessary as we have an Update now.

But it really is rectangle header. That's how RFC calls this.

whitequark · 2017-06-19T18:23:27Z

src/server.rs

+        zlib_data: &'a [u8],
+    },
+    SetCursor {
+        size: (u16, u16),


Let's use separate fields for x/y and width/height for consistency.

Ok. I copied it from Client. Maybe it should be changed there too.

Ok, this can be done after merging the PR. No big deal.

whitequark · 2017-06-19T18:26:10Z

src/server.rs

+    /// Checks if all updates are valid.
+    ///
+    /// Panics if any of the updates is not valid.
+    fn check(&self, validation_data: &ValidationData) {


No no no, this should not be necessary at all! Pass the PixelFormat into a FramebufferUpdate (e.g. by creating the FramebufferUpdate through a method on the Server) and that gets rid of both the strange ValidationData struct as well as separate checks.

Yes, I thought about creating FramebufferUpdate through a method on the Server. But I'd keep ValidationData. As you said rust-vnc may in future want to prevent sending encodings which client does not support. PixelFormat will be then insufficient and for that I introduced ValidationData.

It can just take a reference to the Server (you want to prevent Server from being modified anyway since that could desync ValidationData from the actual server state).

Actually I can't borrow anything from server

{ let mut update = server.create_update(); ------ immutable borrow occurs here update.add_raw_pixels(rect, data); server.send_update(&update); ^^^^^^ mutable borrow occurs here } - immutable borrow ends here

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

{ let update = { let mut builder = server.create_update(); builder.add_raw_pixels(rect, data); builder.done() }; server.send_update(&update); }

whitequark · 2017-06-19T18:26:28Z

src/server.rs

+    /// Serializes this structure and sends it using given `writer`.
+    fn write_to<W: Write>(&self, writer: &mut W) -> Result<()> {
+        for chunk in self.updates.chunks(u16::max_value() as usize) {
+            let count = chunk.len() as u16;


You still aren't checking that there aren't more than 2^16 chunks.

Why would I? Every chunk is now sent in separate message. There's no limit for number of messages. Alternative would be to limit self.update to u16::MAX updates in add_* methods.

whitequark · 2017-06-19T21:32:05Z

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 · 2017-06-20T04:58:12Z

How is the send_update supposed to work in this case?

whitequark · 2017-06-20T08:23:11Z

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 · 2021-02-25T11:50:59Z

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

proton-decay added 2 commits June 18, 2017 10:10

Provide support for ExtendedKeyEvent in protocol module

c266486

Add basic support for server-side

7e4453b

whitequark reviewed Jun 18, 2017

View reviewed changes

proton-decay force-pushed the master branch from 35b7c32 to b6922a5 Compare June 18, 2017 20:28

whitequark requested changes Jun 18, 2017

View reviewed changes

Improve server-side API

9637b43

proton-decay force-pushed the master branch from b6922a5 to 9637b43 Compare June 19, 2017 18:03

whitequark requested changes Jun 19, 2017

View reviewed changes

Improve server-side API - part 2

771716f

Add basic support for server-side #4

Are you sure you want to change the base?

Add basic support for server-side #4

Conversation

proton-decay commented Jun 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark commented Jun 18, 2017

proton-decay commented Jun 18, 2017

proton-decay commented Jun 18, 2017 • edited Loading

proton-decay commented Jun 18, 2017

whitequark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark commented Jun 18, 2017

whitequark commented Jun 18, 2017

proton-decay commented Jun 18, 2017

whitequark commented Jun 18, 2017

proton-decay commented Jun 19, 2017

whitequark commented Jun 19, 2017

whitequark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proton-decay Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark commented Jun 19, 2017

proton-decay commented Jun 20, 2017

whitequark commented Jun 20, 2017

elmarco commented Feb 25, 2021

proton-decay commented Jun 18, 2017 •

edited

Loading

proton-decay Jun 19, 2017 •

edited

Loading