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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Aug 02, 2018 at 12:25:53PM +0200, Roger Pau Monné wrote:
> On Fri, Jul 27, 2018 at 03:05:59PM +0100, Anthony PERARD wrote:
> > +    /* 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?

Because that's a FreeBSD API, at least on my machine (Arch Linux), to
attempt to use it, I will need to install a new package, libbsd. So I
don't think strnstr was an option.

> > +
> > +    /*
> > +     * 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.

Would not that looks a bit weird?

+    while (end) {
+        libxl__json_object *o;
+        /* Start parsing from s */
+        char *s = ev->rx_buf + ev->buf_consumed;
+        /* Findout how much can be parsed */
+        size_t len = end - s;

But that can be done.

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

:-(, just left over from previous attempt.

> > +
> > +        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?

We should have buf_consumed == buf_used when everything received has
been parsed.

Nevertheless, I'll do a debug run to find out, if that free ever happen.

Also, if for e.g., we receive:

nop\r\n
       ^ end should point here, right after the \n, or after the last
       byte received in this case, as per my comment abrove.

> > +            free(ev->rx_buf);
> > +            ev->rx_buf = NULL;
> > +        }
> > +

Thanks,

-- 
Anthony PERARD

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