[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 02/11] libxl_qmp: Separate QMP message generation from qmp_send_prepare
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@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |