Re: [Qemu-devel] [PATCH 1/2] xen-bus: Fix backend state transition on device reset

[Anthony PERARD]

On Thu, Aug 22, 2019 at 10:59:38AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Anthony PERARD <address@hidden>
> > Sent: 22 August 2019 10:51
> > To: Paul Durrant <address@hidden>
> > Cc: address@hidden; address@hidden; Stefano Stabellini <address@hidden>; 
> > xen-
> > address@hidden
> > Subject: Re: [PATCH 1/2] xen-bus: Fix backend state transition on device 
> > reset
> > 
> > On Wed, Aug 21, 2019 at 10:36:40AM +0100, Paul Durrant wrote:
> > > > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> > > > index e40500242d..982eca4533 100644
> > > > --- a/hw/xen/xen-bus.c
> > > > +++ b/hw/xen/xen-bus.c
> > > > @@ -540,9 +540,11 @@ static void xen_device_backend_changed(void 
> > > > *opaque)
> > > >      /*
> > > >       * If the toolstack (or unplug request callback) has set the 
> > > > backend
> > > >       * state to Closing, but there is no active frontend (i.e. the
> > > > -     * state is not Connected) then set the backend state to Closed.
> > > > +     * state is not Connected or Closing) then set the backend state
> > > > +     * to Closed.
> > > >       */
> > > >      if (xendev->backend_state == XenbusStateClosing &&
> > > > +        xendev->frontend_state != XenbusStateClosing &&
> > > >          xendev->frontend_state != XenbusStateConnected) {
> > > >          xen_device_backend_set_state(xendev, XenbusStateClosed);
> > >
> > > Actually, I wonder whether it is better to 'whitelist' here? AFAIK the 
> > > only valid frontend states
> > whether the backend should set itself 'closed' are 'closed' (i.e. the 
> > frontend is finished) and
> > 'initialising' (the frontend was never there).
> > 
> > Let's see, what are the reason backend=Closing?
> >     - frontend changed to Closing (because it wants to disconnect)
> >     - toolstack(libxl) or QEMU(unplug request) set the state to Closing,
> >       but also online to 0.
> > 
> > What should the backend do in both case:
> >     - frontend alive: backend should wait
> >         frontend state might be InitWait, Initialised, Connected,
> >         Closing.
> >     - frontend not existing or disconnected: backend can skip waiting
> >       and go to the next step, Closed.
> >         frontend might be Initialising, Closed.
> >         But there are also Unknown, Reconfiguring and Reconfigured which
> >         are probably errors.
> > 
> > So, the whitelist with Closed and Initialising is a good start, but what
> > about the Unknown state? (QEMU doesn't have backends were the state
> > Reconfigur* is possible, so they can be mapped to Unknown for now).
> 
> I guess we should consider Unknown (basically a missing xenstore state key) 
> to mean either an admin, or the frontend has screwed up or is malicious so I 
> think we just close down the backend straight away. So maybe listing 
> InitWait, Initialised, Connected, and Closing as frontend states that are 
> 'good' (i.e. we wait in anticipation of the frontend eventually getting to 
> Closed) and then say all other states result in immediate close of the 
> backend. Probably worth having a helper function for saying whether a state 
> is good or not.

Sounds good, but I'll use "active" instead of "good" to name the helper
as that feels more accurate to me. Also "active" is already used in the
comment. I'll name the new helper xen_device_state_is_active().

Thanks,

-- 
Anthony PERARD