Re: [Xen-devel] [PATCH v4 17/32] libxl_qmp: Parse JSON input from QMP

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jul 27, 2018 at 03:05:59PM +0100, Anthony PERARD wrote:
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
> 
> Notes:
>     v4:
>         simplification of the patch due to use of a single allocated space 
> for the
>         receive buffer.
> 
>  tools/libxl/libxl_qmp.c | 54 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index b0554df843..665b6f5d05 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -1292,6 +1292,7 @@ static int qmp_ev_callback_readable(libxl__egc *egc, 
> libxl__ev_qmp *ev, int fd)
>  {
>      EGC_GC;
>      ssize_t r;
> +    char *end = NULL;
>  
>      if (!ev->rx_buf) {
>          ev->rx_buf = libxl__malloc(NOGC, QMP_RECEIVE_BUFFER_SIZE);
> @@ -1333,6 +1334,59 @@ static int qmp_ev_callback_readable(libxl__egc *egc, 
> libxl__ev_qmp *ev, int fd)
>      ev->buf_used += r;
>      assert(ev->buf_used < ev->buf_size);
>  
> +    /* workaround strstr limitation */
> +    ev->rx_buf[ev->buf_used] = '\0';

Why not use strnstr to limit the size of the rx_buf that's searched? I
think that would allow you to get rid of the '-1' that you have to
take into account in several places?

> +
> +    /*
> +     * Search for the end of a QMP message: "\r\n" in the newly received
> +     * bytes + the last byte on the previous read() if any
> +     *
> +     * end: This will point to the byte right after \r\n
> +     */
> +    end = strstr(ev->rx_buf + ev->buf_used - r
> +                 + (ev->buf_used - r == 0 ? 0 : -1),
> +                 "\r\n");
> +    if (end)
> +        end += 2;
> +
> +    while (end) {
> +        libxl__json_object *o;
> +        size_t len;
> +        char *s;
> +
> +        /* Start parsing from s */
> +        s = ev->rx_buf + ev->buf_consumed;
> +        /* Findout how much can be parsed */
> +        len = end - s;

You can init both s and len above when defining them.

> +
> +        LOG_QMP("parsing %luB: '%.*s'", len, (int)len, s);
> +
> +        /* Replace \n by \0 so that libxl__json_parse can use strlen */
> +        s[len - 1] = '\0';
> +        o = libxl__json_parse(gc, s); //, len);

Doesn't look like the above line will compile.

> +
> +        if (!o) {
> +            LOGD(ERROR, ev->domid, "Parse error");
> +            return ERROR_FAIL;
> +        }
> +
> +        ev->buf_consumed += len;
> +
> +        if (ev->buf_consumed >= ev->buf_used) {

I'm not sure I see how the above check can ever be true, you search
the buffer up to buf_used, so 'end' can never be past buf_used?

> +            free(ev->rx_buf);
> +            ev->rx_buf = NULL;
> +        }
> +
> +        /* check if there is another message received at the same time */
> +        if (ev->rx_buf) {
> +            end = strstr(ev->rx_buf + ev->buf_consumed, "\r\n");
> +            if (end)
> +                end += 2;
> +        } else
> +            end = NULL;
> +
> +        LOG_QMP("JSON object received: %s", libxl__json_object_to_json(gc, 
> o));
> +    }

Newline.

>      return 0;

Thanks, Roger.

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