Re: [Xen-devel] [PATCH v4 18/32] libxl_qmp: Separate QMP message generation from qmp_send_prepare

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

On Fri, Jul 27, 2018 at 03:06:00PM +0100, Anthony PERARD wrote:
> To be able to re-use qmp_prepare_qmp_cmd with libxl__ev_qmp.
> 
> Also, add the QMP end of command '\r\n' into the generated string.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
>  tools/libxl/libxl_qmp.c | 62 +++++++++++++++++++++++++++++------------
>  1 file changed, 44 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 665b6f5d05..38a4395266 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -578,17 +578,17 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler 
> *qmp)
>      return rc;
>  }
>  
> -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)
> -{
> -    const unsigned char *buf = NULL;
> -    char *ret = NULL;
> -    libxl_yajl_length len = 0;
> +static char *qmp_prepare_qmp_cmd(libxl__gc *gc,

Is it really needed to have qmp twice in the function name?
qmp_prepare_cmd LGTM.

> +                                 const char *cmd,
> +                                 const libxl__json_object *args,
> +                                 int id,
> +                                 size_t *len_r)
> +{
> +    const unsigned char *buf;

While there I would drop the unsigned...

> +    libxl_yajl_length len;
>      yajl_gen_status s;
>      yajl_gen hand;
> -    callback_id_pair *elm = NULL;
> +    char *ret = NULL;
>  
>      hand = libxl_yajl_gen_alloc(NULL);
>  
> @@ -600,7 +600,7 @@ static char *qmp_send_prepare(libxl__gc *gc, 
> libxl__qmp_handler *qmp,
>      libxl__yajl_gen_asciiz(hand, "execute");
>      libxl__yajl_gen_asciiz(hand, cmd);
>      libxl__yajl_gen_asciiz(hand, "id");
> -    yajl_gen_integer(hand, ++qmp->last_id_used);
> +    yajl_gen_integer(hand, id);
>      if (args) {
>          libxl__yajl_gen_asciiz(hand, "arguments");
>          libxl__json_object_to_yajl_gen(gc, hand, args);
> @@ -610,6 +610,36 @@ static char *qmp_send_prepare(libxl__gc *gc, 
> libxl__qmp_handler *qmp,
>      s = yajl_gen_get_buf(hand, &buf, &len);
>  
>      if (s) {
> +        goto out;
> +    }
> +
> +    ret = libxl__malloc(NOGC, len + 3);
> +    strncpy(ret, (const char *)buf, len + 3);

... so that you can drop the cast from here.

> +    strncpy(ret + len, "\r\n", 3);
> +    len += 2;
> +
> +    if (len_r)
> +        *len_r = len;
> +
> +out:
> +    yajl_gen_free(hand);
> +    return ret;
> +}
> +
> +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,
> +                              size_t *len_r)
> +{
> +    char *buf;
> +    callback_id_pair *elm;
> +
> +    buf = qmp_prepare_qmp_cmd(gc,
> +                              cmd, args, ++qmp->last_id_used,
> +                              NULL);

Indentation is very weird here. AFAICT it can fit in a single line?

Thanks, Roger.

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