[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] vnc: added initial websocket protocol suppo
From: |
Anthony Liguori |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] vnc: added initial websocket protocol support |
Date: |
Mon, 07 Jan 2013 18:38:30 -0600 |
User-agent: |
Notmuch/0.13.2+93~ged93d79 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu) |
Tim Hardeck <address@hidden> writes:
> Hi Anthony,
>
> thanks for your feedback.
>
> On Mon, 2013-01-07 at 13:52 -0600, Anthony Liguori wrote:
>> Tim Hardeck <address@hidden> writes:
> > +void vncws_handshake_read(void *opaque)
>> > +{
>> > + VncState *vs = opaque;
>> > + long ret;
>> > + buffer_reserve(&vs->ws_input, 4096);
>> > + ret = vnc_client_read_buf(vs, buffer_end(&vs->ws_input), 4096);
>> > +
>> > + if (!ret) {
>> > + if (vs->csock == -1) {
>> > + vnc_disconnect_finish(vs);
>> > + }
>> > + return;
>> > + }
>> > + vs->ws_input.offset += ret;
>> > +
>> > + if (vs->ws_input.offset > 0) {
>> > + qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
>> > + vncws_process_handshake(vs, vs->ws_input.buffer,
>> > vs->ws_input.offset);
>> > + buffer_reset(&vs->ws_input);
>>
>> This is making a bad assumption. You're assuming that
>> vnc_client_read_buf() is always going to return at least the amount of
>> data you need to do the handshake and never any more data.
>
> Following the Websocket protocol specification there shouldn't be more
> data until a handshake response from the server.
>
>> You need something more sophisticated that keeps reading data until
>> you've gotten the complete set of headers and the trailing double EOL.
>
> How about that:
Better, but I still think it's better to advance the buffer based on the
parsed header sizes that to assume there's no additional data.
Regards,
Anthony Liguori
>
> diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c
> index 6b98d6b..298bc20 100644
> --- a/ui/vnc-ws.c
> +++ b/ui/vnc-ws.c
> @@ -35,7 +35,8 @@ void vncws_handshake_read(void *opaque)
> }
> vs->ws_input.offset += ret;
>
> - if (vs->ws_input.offset > 0) {
> + if (g_strstr_len((char *)vs->ws_input.buffer, vs->ws_input.offset,
> + WS_HANDSHAKE_END)) {
> qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL,
> vs);
> vncws_process_handshake(vs, vs->ws_input.buffer,
> vs->ws_input.offset);
> buffer_reset(&vs->ws_input);
> diff --git a/ui/vnc-ws.h b/ui/vnc-ws.h
> index 7402e77..c8dfe34 100644
> --- a/ui/vnc-ws.h
> +++ b/ui/vnc-ws.h
> @@ -38,6 +38,7 @@ Sec-WebSocket-Accept: %s\r\n\
> Sec-WebSocket-Protocol: binary\r\n\
> \r\n"
> #define WS_HANDSHAKE_DELIM "\r\n"
> +#define WS_HANDSHAKE_END "\r\n\r\n"
> #define WS_SUPPORTED_VERSION "13"
>
> #define WS_HEAD_MIN_LEN sizeof(uint16_t)
>
>
> We also could calculate the handshake size and use buffer_advance
> instead but since there shouldn't be any additional data received from
> the client until a server response anyway I would prefer it this way.
>
>
>>
>> > +long vnc_client_read_ws(VncState *vs)
>> > +{
>> > + int ret, err;
>> > + uint8_t *payload;
>> > + size_t payload_size, frame_size;
>> > + VNC_DEBUG("Read websocket %p size %zd offset %zd\n",
>> > vs->ws_input.buffer,
>> > + vs->ws_input.capacity, vs->ws_input.offset);
>> > + buffer_reserve(&vs->ws_input, 4096);
>> > + ret = vnc_client_read_buf(vs, buffer_end(&vs->ws_input), 4096);
>> > + if (!ret) {
>> > + return 0;
>> > + }
>> > + vs->ws_input.offset += ret;
>> > +
>> > + /* make sure that nothing is left in the ws_input buffer */
>> > + do {
>> > + err = vncws_decode_frame(&vs->ws_input, &payload,
>> > + &payload_size, &frame_size);
>> > + if (err <= 0) {
>> > + return err;
>> > + }
>> > +
>> > + buffer_reserve(&vs->input, payload_size);
>> > + buffer_append(&vs->input, payload, payload_size);
>> > +
>> > + buffer_advance(&vs->ws_input, frame_size);
>> > + } while (vs->ws_input.offset > 0);
>>
>> This likewise seems wrong. What happens if you get a read of a partial
>> frame? This code will fail, no?
> In case of a partial frame err would be 0 and QEMU would wait for more
> data until processing.
>
> Regards
> Tim
>
>
> --
> SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix
> Imendörffer, HRB 16746 (AG Nürnberg)
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> T: +49 (0) 911 74053-0 F: +49 (0) 911 74053-483
> http://www.suse.de/
[Qemu-devel] [PATCH 1/3] vnc: added buffer_advance function, Tim Hardeck, 2013/01/02