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

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

On Wed, Nov 14, 2018 at 03:43:41PM +0000, Ian Jackson wrote:
> (I finally remembered to drop the message-id from the CC header...)

:), I kept forgeting as well.

> Anthony PERARD writes ("Re: [PATCH v6.1 05/11] libxl_qmp: Implementation of libxl__ev_qmp_*"):
> > > 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.

The current implementation already cleanup the broken state before the
control is passed outside ev_qmp implementation. That result in the Idle
public state. Whenever an internal function throw an error, it lets the
main function qmp_ev_fd_callback taking care of cleanup the `broken` state
so that when the control passes outside ev_qmp implementation the state
is disconnected/Idle.

An internal `broken` state is just an half-transition toward the
disconnected/Idle state.

I guess I could add to the internal state documentation:

  When an internal function return an error, it can leave ev_qmp in a
  `broken` state but only if the caller is another internal function.
  That `broken` needs to be cleaned up, e.i. transitionned to the
  `disconnected` state, before the control of ev_qmp is released
  outsides of ev_qmp implementation.

Thanks,

-- 
Anthony PERARD

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