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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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?

> > 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.

> > +            incomplete_size += rd;
> > +            s_end = incomplete + incomplete_size;
> > +        } else {
> > +            incomplete = libxl__strndup(gc, qmp->buffer, rd);
> > +            incomplete_size = rd;
> > +            s = incomplete;
> > +            s_end = s + rd;
> > +            rd = 0;
> 
> This can be dropped.
> 
> And I think we should take this change to change "incomplete" to
> something more meaningful, like "qmp_msg_buf".

I think that can be done.


-- 
Anthony PERARD

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