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

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

(I finally remembered to drop the message-id from the CC header...)

Anthony PERARD writes ("Re: [PATCH v6.1 05/11] libxl_qmp: Implementation of 
libxl__ev_qmp_*"):
> Here is a simpler comment that is true:
>   !disconnected -> same state
> 
> Even if in this function ev_fd is modified, it needs to be Active on
> entry, and stay Active on return. But just saying that qmp_state needs
> to be different than disconnected is enough.

LGTM.

> > Does it change the state to a new one ?  Are the old and new states
> > pure states as described in the state table, or are they
> > half-transitioned ?  (More on this below.)
> 
> They are half-transitioned, here is a new comments:
> 
>     on entry: connecting/connected but with `msg` free
>     on return: same state but with `msg` set

LGTM.


> > But assuming that what you write here in your proposed comment is
> > true, ...
> > 
> > >   State transition
> > >     connected -> waiting_reply
> > >     * -> state unchange
> > >     on error: disconnected
> > >   The state of the transmiting buffer will be changed.
> > 
> > ... this looks good.
> 
> Do I need to say here and everywhere else that on error the new state
> isn't really disconnected, and that the ev_qmp needs to be cleaned?
> On one hand, saying that the new state is disconnected means that the
> ev_qmp functions that only deal with !disconnected, but on the other,
> the caller still need to call _dispose.

I think you are saying you can leave your ev_qmp in a state like
disconnected, but with things allocated.  Let us call that state
`broken' (in internal nomenclature).

But that state `broken' does not correspond to any of your public
states.  So your API document does not match the code.

You need to either change the API to document this state as a public
Broken state, or arrange for every ev_qmp-internal function to clean
it up appropriately before control passes outside the ev_qmp
implementation.

It is probably easier to introduce the new public Broken state.

As for the commentary: if you prefer not to document the `error ->
Broken' transition next to each function, you can write a general
statement in the internal design.  Eg, `for any internal function
which returns an error code, the state on error return will be Broken,
unless otherwise stated'.  Or something.

Thanks,
Ian.

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