Re: [Xen-devel] [RFC v2 4/9] libxl_qmp: Move the buffer realloc to the same scope level as read

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Apr 23, 2018 at 03:50:27PM +0100, Anthony PERARD wrote:
> On Mon, Apr 23, 2018 at 10:03:39AM +0100, Wei Liu wrote:
> > On Mon, Apr 16, 2018 at 06:32:22PM +0100, Anthony PERARD wrote:
> > > In qmp_next(), the inner loop should only try to parse messages from
> > > QMP, if there is more than one.
> > > 
> > > The handling of the receive buffer ('incomplete'), should be done at the
> > > same scope level as read(). It doesn't need to be handle more that once
> > > after a read.
> > > 
> > 
> > In general I agree this is a better idea than the current code.
> > 
> > > Before this patch, when on message what handled, the inner loop would
> > 
> > Sorry, I failed to parse "when on message what handled".
> 
> I probably wanted to write: "when one message was handled". Is this
> better?

Yes.

> 
> > > restart by adding the 'buffer' into 'incomplete' (after reallocation).
> > > Since 'rd' was not reset, the buffer would be strcat a second time.
> > > After that, the stream from the QMP server would have syntax error, and
> > > the parsor would throw errors.
> > > 
> > > This is unlikely to happen as the receive buffer is very large. And
> > > receiving two messages in a row is unlikely. In the current case, this
> > > could be an event and a response to a command.
> > > 
> > > Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > > ---
> > >  tools/libxl/libxl_qmp.c | 31 ++++++++++++++++---------------
> > >  1 file changed, 16 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> > > index 4d207c3842..a25f445fb6 100644
> > > --- a/tools/libxl/libxl_qmp.c
> > > +++ b/tools/libxl/libxl_qmp.c
> > > @@ -524,23 +524,24 @@ static int qmp_next(libxl__gc *gc, 
> > > libxl__qmp_handler *qmp)
> > >  
> > >          DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, (int)rd);
> > >  
> > > +        if (incomplete) {
> > > +            size_t current_pos = s - incomplete;
> > > +            incomplete = libxl__realloc(gc, incomplete,
> > > +                                        incomplete_size + rd + 1);
> > > +            strncat(incomplete + incomplete_size, qmp->buffer, rd);
> > > +            s = incomplete + current_pos;
> > 
> > This can be dropped, because s is not changed. It is just the reversal
> > of what is a few lines above.
> 
> No, realloc may change the location of the allocated memory. So, between
> "current_pos = s - incomplete" and "s = incomplete + current_pos", the
> value of 'incomplete' may have changed.

Indeed, I missed that somehow.

Wei.

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