Re: [Xen-devel] [PATCH v6 05/11] libxl_qmp: Implementation of libxl__ev_qmp_*

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[PATCH v6 05/11] libxl_qmp: Implementation of 
libxl__ev_qmp_*"):
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

I started reviewing this in detail but I got bogged down in the main
implementation details because none of the internal functions like
qmp_ev_connect have doc comments saying what state they expect to find,
and what state they promise to leave.

I started trying to reverse engineer it, but I think this will be
both error prone and time consuming.  I think you have the answers to
this in your head but just didn't write it down.

Can you please respin just this patch, as a v6.1, with no changes
other than to add a comment next to the implementation of each
function (both the internal ones like qmp_ev_connect, and the external
ones like libxl__ev_qmp_send) saying what state(s) they accept and
produce (internal states, that is) ?

I guess that won't take you very long.

Eg

  static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
    /* disconnected/connecting/connected -> connecting */
  {

(if that is indeed true).

I think such comments would be useful even for external-facing
functions, because although they have to cope with any internal state
corresponding to any of the legal external states, the reader wants to
know what internal state results, not just what external state.  These
comments should be in libxl_qmp.c because they're part of the
implementation, not the API.

Thanks,
Ian.

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