From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42612) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsNDd-0007FE-JG for qemu-devel@nongnu.org; Mon, 07 Jan 2013 19:39:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TsNDZ-0005Ma-R9 for qemu-devel@nongnu.org; Mon, 07 Jan 2013 19:39:05 -0500 Received: from e7.ny.us.ibm.com ([32.97.182.137]:48466) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsNDZ-0005MR-Nc for qemu-devel@nongnu.org; Mon, 07 Jan 2013 19:39:01 -0500 Received: from /spool/local by e7.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 7 Jan 2013 19:38:58 -0500 Received: from d01relay03.pok.ibm.com (d01relay03.pok.ibm.com [9.56.227.235]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id 98205C90041 for ; Mon, 7 Jan 2013 19:38:39 -0500 (EST) Received: from d01av01.pok.ibm.com (d01av01.pok.ibm.com [9.56.224.215]) by d01relay03.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r080cdmC246824 for ; Mon, 7 Jan 2013 19:38:39 -0500 Received: from d01av01.pok.ibm.com (loopback [127.0.0.1]) by d01av01.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r080cc2w000571 for ; Mon, 7 Jan 2013 19:38:39 -0500 From: Anthony Liguori In-Reply-To: <1357603021.1939.64.camel@Thinktank.site> References: <1357133386-7643-1-git-send-email-thardeck@suse.de> <1357133386-7643-3-git-send-email-thardeck@suse.de> <87sj6cu5y0.fsf@codemonkey.ws> <1357603021.1939.64.camel@Thinktank.site> Date: Mon, 07 Jan 2013 18:38:30 -0600 Message-ID: <87pq1gcxvd.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/3] vnc: added initial websocket protocol support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Tim Hardeck Cc: stefanha@gmail.com, github@martintribe.org, qemu-devel@nongnu.org, alevy@redhat.com, kraxel@redhat.com, corentin.chary@gmail.com Tim Hardeck writes: > Hi Anthony, > > thanks for your feedback. > > On Mon, 2013-01-07 at 13:52 -0600, Anthony Liguori wrote: >> Tim Hardeck writes: > > +void vncws_handshake_read(void *opaque) >> > +{ >> > + VncState *vs =3D opaque; >> > + long ret; >> > + buffer_reserve(&vs->ws_input, 4096); >> > + ret =3D vnc_client_read_buf(vs, buffer_end(&vs->ws_input), 4096); >> > + >> > + if (!ret) { >> > + if (vs->csock =3D=3D -1) { >> > + vnc_disconnect_finish(vs); >> > + } >> > + return; >> > + } >> > + vs->ws_input.offset +=3D 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); >>=20 >> 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 +=3D ret; >=20=20 > - 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" >=20=20 > #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. > > >>=20 >> > +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 =3D vnc_client_read_buf(vs, buffer_end(&vs->ws_input), 4096); >> > + if (!ret) { >> > + return 0; >> > + } >> > + vs->ws_input.offset +=3D ret; >> > + >> > + /* make sure that nothing is left in the ws_input buffer */ >> > + do { >> > + err =3D vncws_decode_frame(&vs->ws_input, &payload, >> > + &payload_size, &frame_size); >> > + if (err <=3D 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); >>=20 >> 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 > > > --=20 > SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix > Imend=C3=B6rffer, HRB 16746 (AG N=C3=BCrnberg) > Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany > T: +49 (0) 911 74053-0 F: +49 (0) 911 74053-483 > http://www.suse.de/