Re: [Qemu-devel] [PATCH v1 2/7] io: simplify websocket ping reply handling

[Daniel P. Berrange]

On Tue, Oct 10, 2017 at 11:55:25AM -0500, Eric Blake wrote:
> On 10/10/2017 10:43 AM, Daniel P. Berrange wrote:
> > We must ensure we don't get flooded with ping replies if the outbound
> > channel is slow. Currently we do this by keeping the ping reply in a
> > separate temporary buffer and only writing it if the encoutput buffer
> > is completely empty. This is overly pessimistic, as it is reasonable
> > to add a ping reply to the encoutput buffer even if it has previous
> > data in it, as long as that previous data doesn't include a ping
> > reply.
> > 
> > To track this better, put the ping reply directly into the encoutput
> > buffer, and then record the size of encoutput at this time in
> > ping_remain. As we write encoutput to the underlying channel, we
> > can decrement the ping_remain counter. Once it hits zero, we can
> > accept further ping replies for transmission.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  include/io/channel-websock.h |  2 +-
> >  io/channel-websock.c         | 28 +++++++++++++++-------------
> >  2 files changed, 16 insertions(+), 14 deletions(-)
> > 
> 
> > +++ b/io/channel-websock.c
> > @@ -825,11 +825,14 @@ static int 
> > qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
> >          }
> >          return -1;
> >      } else if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_PING) {
> > -        /* ping frames produce an immediate reply */
> > -        buffer_reset(&ioc->ping_reply);
> > -        qio_channel_websock_encode_buffer(
> > -            ioc, &ioc->ping_reply, QIO_CHANNEL_WEBSOCK_OPCODE_PONG,
> > -            &ioc->encinput);
> > +        /* ping frames produce an immediate reply, as long as we've not 
> > still
> > +         * got a previous ping queued, in which case we drop the new pong 
> > */
> 
> Wouldn't that be a 'previous pong queued'?

Indeed

> 
> > +        if (ioc->ping_remain == 0) {
> > +            qio_channel_websock_encode_buffer(
> > +                ioc, &ioc->encoutput, QIO_CHANNEL_WEBSOCK_OPCODE_PONG,
> > +                &ioc->encinput);
> > +            ioc->ping_remain = ioc->encoutput.offset;
> > +        }
> 
> But if you change the comment, then naming the variable pong_remain may
> make more sense.

Yeah, that's sensible.

> 
> But naming is a bikeshed issue, so either way,
> 
> Reviewed-by: Eric Blake <address@hidden>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|