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

[Anthony PERARD <anthony.perard@citrix.com>]

On Mon, Nov 12, 2018 at 05:20:53PM +0000, Ian Jackson wrote:
> 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"):
> > -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".

The memory allocated from malloc is new, before that it was allocated in
`gc` (aside from the `yajl_gen hand` which needs to be freed before the
function returns).

I've make the change to return a NOGC buffers because I don't know how
to have an allocation survive an `egc`.

Do you think I could call qmp_send_prepare with an `ao_gc` ? Not in this
patch, but later, in the context of libxl__ev_qmp which I think
shouldn't survive an AO.

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

I'll try to improve the patch description and awnser all those things.

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

I guess I can recalculate the lenght at the time when it will be needed
in a later patch.

-- 
Anthony PERARD

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