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

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

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

> +        }
> +
>          do {
>              char *end = NULL;
> -            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;
> -                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;
> -            }
>  
>              end = strstr(s, "\r\n");
>              if (end) {
> -- 
> Anthony PERARD
> 

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