Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data [and 1 more messages]

[Anthony PERARD <anthony.perard@citrix.com>]

On Mon, Oct 29, 2018 at 05:31:59PM +0000, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data [and 1 more messages]"):
> > On Mon, Oct 15, 2018 at 05:35:36PM +0100, Ian Jackson wrote:
> > > > +        ev->buf_consumed += len;
> > > > +
> > > > +        if (ev->buf_consumed >= ev->buf_used) {
> > > > +            free(ev->rx_buf);
> > > 
> > > Surely buf_consumed <= buf_used ?  Maybe you should assert that.
> > 
> > I'm attempting to detect here if there's something left in the buffer
> > that hasn't been parsed yet. If there is nothing left, we can free the
> > buffer.
> > 
> > Or do you mean that the condition should be buf_consumed == buf_used ?
> 
> It felt odd to me to read `ev->buf_consumed >= ev->buf_used' when I
> knew that buf_consumed <= buf_used.  I am suggesting adding
>   assert(buf_consumed <= buf_used);
> and then writing
>   if (ev->buf_consumed == ev->buf_used)
> 
> To be honest this condition is a bit fiddly.  Why not do this after
> subtracting buf_consumed from buf_used and copying the old data down ?

Somehow, that feels wrong to move the data inside the buffer.

> Then it would read
>   if (!ev->buf_used)
> 
> But even better, if you change your code to permit ev->rx_buf != NULL
> when ev->rx_buf_used==0, this code goes away completely.

I guess you mean to keep the buffer even once there's nothing left to
parse? (And only free it once we call _dispose().)


-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel