Re: [Qemu-devel] [PATCH 5/8] virtio-serial-bus: Add support for buffering guest output, throttling guests

[Amit Shah]

On (Mon) Jan 11 2010 [23:33:56], Jamie Lokier wrote:
> Amit Shah wrote:
> > > Are you talking about a VNC protocol command between qemu's VNC server
> > > and the user's VNC client, or a private protocol between the guest and
> > > qemu's VNC server?
> > 
> > What happens is:
> > 
> > 1. Guest puts something on its clipboard
> > 2. An agent on the guest gets notified of new clipboard contents
> > 3. This agent sends over the entire clipboard contents to qemu via
> >    virtio-serial
> > 4. virtio-serial sends off this data to the virtio-serial-vnc code
> > 5. ServerCutText message from the vnc backend is sent to the vnc client
> > 6. vnc client's clipboard gets updated
> > 7. You can see guest's clipboard contents in your client's clipboard.
> > 
> > I'm talking about steps 3, 4, 5 here.
> 
> Ok. Let's not worry about 5; it doesn't seem relevant, only that the
> guest clipboad is sent to the host somehow.

Actually, it is important...

> > > You have already told it the total length to expect.  There is no
> > > ambiguity about where it ends.
> > 
> > Where does the total length come from? It has to come from the guest.
> > Otherwise, the vnc code will not know if a byte stream contains two
> > separate clipboard entries or just one huge clipboard entry.
> 
> I see.  So it's a *really simple* protocol where the clipboard entry
> is sent by the guest agent with a single write() without any framing bytes?
> 
> > Earlier, I used to send the length of one write as issued by a guest to
> > qemu. I just changed that to send a START and END flag so that I don't
> > have to send the length.
> 
> Why not just have the guest agent send a 4-byte header which is the
> integer length of the clipboard blob to follow?
> 
> I.e. instead of
> 
>     int guest_send_clipboard(const char *data, size_t length)
>     {
>         return write_full(virtio_fd, data, length);
>     }
> 
> do this:
> 
>     int guest_send_clipboard(const char *data, size_t length)
>     {
>         u32 encoded_length = cpu_to_be32(length);
>         int err = write_full(virtio_serial_fd, &encoded_length,
>                              sizeof(encoded_length));
>         if (err == 0)
>             err = write_full(virtio_serial_fd, data, length);
>         return err;
>     }
> 
> > If this doesn't explain it, then I think we're not understanding each
> > other here.
> 
> It does explain it very well, thanks.  I think you're misguided about
> the solution :-)

The above solution you specify works if it's assumed that we hold off
writes to the vnc client till we get a complete buffer according to the
header received.

Now, a header might contain the length 10000, meaning 10000 bytes are to
be expected.

What if the write() on the guest fails after writing 8000 bytes? There's
no way for us to signal that.

So this vnc port might just be waiting for all 10000 bytes to be
received, and it may never receive anything more.

Or, it might receive the start of the next clipboard entry and it could
be interpreted as data from the previous copy.

> What confused me was you mentioned the VNC ServerCutText command
> having to receive the whole data in one go.  ServerCutText isn't
> really relevant to this,

It is relevant. You can't split up one ServerCutText command in multiple
buffers. You can also not execute any other commands while one command
is in progress, so you have to hold off on executing ServerCutText till
all the data is available. And you can't reliably do that from guest
userspace because of the previously-mentioned scenario.

> I really can't see any merit in making virtio-serial not be a serial
> port, being instead like a unix datagram socket, to support a specific
> user of virtio-serial when a trivial 4-byte header in the guest agent
> code would be easier for that user anyway.

BTW I don't really want this too, I can get rid of it if everyone agrees
we won't support clipboard writes > 4k over vnc or if there's a better
idea.

                Amit