Re: [Xen-devel] [PATCH v6 02/11] libxl_qmp: Separate QMP message generation from qmp_send_prepare

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Thanks for the repost.  I feel I am going to make some comments which
could perhaps have been made earlier, so sorry for that:

Anthony PERARD writes ("[PATCH v6 02/11] libxl_qmp: Separate QMP message 
generation from qmp_send_prepare"):
>     v6:
>         comment about ownership of buf

This is good.  But:

> -static char *qmp_send_prepare(libxl__gc *gc, libxl__qmp_handler *qmp,
> -                              const char *cmd, libxl__json_object *args,
> -                              qmp_callback_t callback, void *opaque,
> -                              qmp_request_context *context)

Previously this function returned memory allocated from malloc, and
this was not documented.  I think it should be, for both this and for
qmp_prepare_cmd.  See the big libxl.h comment on "memory allocation".

> @@ -608,6 +607,32 @@ static char *qmp_send_prepare(libxl__gc *gc, 
> libxl__qmp_handler *qmp,
>      s = yajl_gen_get_buf(hand, &buf, &len);
>  
>      if (s) {
> +        goto out;
> +    }

Should there be a log message here ?  If not, maybe you just meant
    if (s) goto out;
In libxl_json.c we find the pattern is usually:
    if (s != yajl_gen_status_ok) goto out;
but I guess we can hardcode the assumption that yajl_gen_status_ok==0.

> +    ret = libxl__sprintf(NOGC, "%*.*s\r\n", (int)len, (int)len, buf);
> +    len += 2;

Please use %n to get the length, instead.  This kind of ad-hoc
addition encodes the buffer usage information in two disjoint places
and can easily result in buffer length bugs when the code is later
modified.

>  static int qmp_send(libxl__qmp_handler *qmp,
> @@ -641,7 +663,8 @@ static int qmp_send(libxl__qmp_handler *qmp,
>      int rc = -1;
>      GC_INIT(qmp->ctx);
>  
> -    buf = qmp_send_prepare(gc, qmp, cmd, args, callback, opaque, context);
> +    buf = qmp_send_prepare(gc, qmp, cmd, args, callback, opaque, context,
> +                           NULL);
>  
>      if (buf == NULL) {
>          goto out;
> @@ -650,12 +673,10 @@ static int qmp_send(libxl__qmp_handler *qmp,
>      if (libxl_write_exactly(qmp->ctx, qmp->qmp_fd, buf, strlen(buf),
>                              "QMP command", "QMP socket"))
>          goto out;
> -    if (libxl_write_exactly(qmp->ctx, qmp->qmp_fd, "\r\n", 2,
> -                            "CRLF", "QMP socket"))
> -        goto out;
>  
>      rc = qmp->last_id_used;
>  out:
> +    free(buf);

This patch seems to be a mixture of four things:

 1. Changing the return value convention of qmp_send_prepare
    to expect the caller to free it.

 2. Changing qmp_send_prepare to include the \r\n

 3. Splitting qmp_prepare_cmd out from qmp_send_prepare

 4. Changing qmp_send_prepare to tell the caller the length

Overall I think there is supposed to be no functional change ?
This should be mentioned in the commit message.

I appreciate that these four things are small, perhaps too small to
split out, but they should all be mentioned in the commit message.

And then, the reasons for these changes are unstated.  AFAICT:

3 is wanted because we are going to have a new caller which wants to
handle the sending itself.  Fine.

2 is wanted because every caller will want this, so it should be done
in the common function.

1 is wanted because 2 demands it.

4 ???  The existing code uses strlen.  JSON can't contain nul bytes.
Why should the new caller not do similarly ?  If the use of strlen is
wrong why not change the old caller ?  (Maybe it is going away, in
which case that needs to be mentioned.)

Ian.

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