Re: [PATCH v6.1 05/11] libxl_qmp: Implementation of libxl__ev_qmp_*

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

On Tue, Nov 13, 2018 at 01:14:30PM +0000, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v6.1 05/11] libxl_qmp: Implementation of libxl__ev_qmp_*"):
> ...
> 
> I think this was intended to satisfy my request for comments about
> legal states:
> 
> > +/* helpers */
> > +
> > +static void qmp_ev_ensure_reading_writing(libxl__gc *gc, libxl__ev_qmp *ev)
> > +{
> > +    bool enable = false;
> 
> This one is probably an asisstant for transitioning between states so
> the pre- and post-conditions may not be pure.  Whatever it is should
> be documented...

It's hard to document the state transition of a function that doesn't
care of the current state when the function is called, and will attempt
to figure out the current state to find out if a function `writable`
needs to be called later.

But now I realise that `disconnected` would be an illigal state.

What about:

  Precondition: !disconnected
  ensure that qmp_ev_callback_writable is been called when needed

> > +static void qmp_ev_set_state(libxl__gc *gc, libxl__ev_qmp *ev,
> > +                             libxl__qmp_state new_state)
> > +{
> 
> This one at least does not need a comment :-).
> 
> > +static int qmp_ev_prepare_cmd(libxl__gc *gc,
> > +                              libxl__ev_qmp *ev,
> > +                              const char *cmd,
> > +                              const libxl__json_object *args)
> > +{
> > +    char *buf = NULL;
> 
> Missing state comment.

Maybe:

  Precondition: connecting/connected

> > +static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
> > +                               int fd, short events, short revents)
> > +{
> 
> Missing state comment, although I think the precondition can be easily
> inferred from the state of ev_fd and the postcondition varies, but
> would still be nice to discuss it.

This function is the main loop function, so almost everything happen in
this function. It should not be called directly. So I'm not sure what
kind of comment would be usefull here.

Maybe:
  Preconditions:
    `ev_fd` is Active
    this means that `ev` isn't disconnected
  Any allowed internal state transition can happen.
  A user callback may be called, when that happen, the function should
  return.

> > +static int qmp_ev_callback_writable(libxl__gc *gc,
> > +                                    libxl__ev_qmp *ev, int fd)
> > +    /* connected -> waiting_reply
> > +     * the state isn't change otherwise. */
> > +{
> 
> I don't know what `otherwise' means.  Maybe you mean all other states
> are legal and remain unchanged ?  But that does not seem to be
> likely.  Presumably disconnected is ruled out, at least.

If for some random reason this function is called with the state
disconnected, it would just return. Unless the state is disconnecting
and tx_buf haven't been cleared yet.

`Otherwise` would be the `otherwise` of haskell, or the `default` of a
switch case in C.

So a different comment could be:
  Precondition:
    !disconnected
  State transition
    connected -> waiting_reply
    * -> state unchange
    on error: disconnected
  The state of the transmiting buffer will be changed.



> > +static int qmp_ev_callback_readable(libxl__egc *egc,
> > +                                    libxl__ev_qmp *ev, int fd)
> > +    /* on error: * -> disconnected */
> 
> Precondition ?  And on non-error ?

Here is a more complete comment:
  Precondition:
    !disconnected
  State transition:
    Only the state of the receiving buffer is change on success
    on error: disconnected

> > +static int qmp_ev_get_next_msg(libxl__egc *egc, libxl__ev_qmp *ev,
> > +                               libxl__json_object **o_r)
> > +    /* Find a JSON object and store it in o_r.
> > +     * return ERROR_NOTFOUND if no object is found.
> > +     * `o_r` is allocated within `egc`.
> > +     */
> > +{
> 
> Missing state comment.

  precondition: !disconnected
  state of the receiving buffer can be changed.

> > +static int qmp_ev_parse_error_messages(libxl__egc *egc,
> > +                                       libxl__ev_qmp *ev,
> > +                                       const libxl__json_object *resp)
> > +{
> 
> This doesn't touch the state I think.  Should perhaps be mentioned in
> a comment.

The only thing that this function use is set by a user (of
libxl__ev_qmp): ev->domid. But I guess that comment would do:

  no state change


Are all those comments good enough? Also sometime the internal state
isn't fully changed in one go, and the transition could happen in
several functions (I think). Do we needs states like disconnecting,
connectinging, ... ? with a comment that say that the value of the
internal variables can be one of before or after the state transition.

Next time I'll write one BIG function, and there will be less of those
comments to write :).

Thanks,

-- 
Anthony PERARD

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