Re: [Xen-devel] [PATCH 5 of 7 v2] blktap3/tapback: Introduce front-end XenStore path handler

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-01-25 at 15:09 +0000, Thanos Makatos wrote:
> > On Fri, 2013-01-04 at 12:14 +0000, Thanos Makatos wrote:
> > > +static inline int
> > > +blkback_frontend_changed(vbd_t * const xbdev, const XenbusState
> > state)
> > > +{
> > > +    /*
> > > +     * XXX The size of the array (9) comes from the XenbusState
> > enum.
> > > +     *
> > > +     * TODO Send a patch that adds XenbusStateMin, XenbusStateMax,
> > > +     * XenbusStateInvalid and in the XenbusState enum (located in
> > xenbus.h).
> > > +     */
> > > +    struct frontend_state_change {
> > > +        int (*fn)(vbd_t * const, const XenbusState);
> > > +        const XenbusState state;
> > 
> > Is this the next backend state or the current or?...
> > 
> > > +    } static const frontend_state_change_map[] = {
> > > +        {NULL, 0},                                          /*
> > Unknown */
> > 
> >     [XenbusStateUnknown] = {NULL,0},
> >     [XenbusStateInitialising] = {tapback_device_switch_state,
> > XenbusStateInitWait},
> > 
> > ? Bit less error prone.
> > 
> > > +        {tapback_device_switch_state, XenbusStateInitWait}, /*
> > Initialising */
> > > +        {NULL, 0},                                          /*
> > InitWait */
> > > +        {blkback_connect_tap, 0},                           /*
> > Initialised */
> > > +        {blkback_connect_tap, 0},                           /*
> > Connected */
> > > +        {tapback_device_switch_state, XenbusStateClosing},  /*
> > Closing */
> > > +        {backend_close, 0},                                 /*
> > StateClosed */
> > > +        {NULL, 0},                                          /*
> > Reconfiguring */
> > > +        {NULL, 0}                                           /*
> > Reconfigured */
> > > +    };
> 
> The position of each element in the frontend_state_change_map array
> reflects the state into which the front-end just switched. Each
> element contains a call-back function to be executed in response, and
> an optional state for the back-end to switch to. Does this make sense?

Absolutely.

>  Actually, there was a switch statement and I replaced it with this in
>  order to make it less error-prone!

The switch is a pretty common idiom in xenbus state machines but this
works too if you prefer it.

My suggestion is just to use:
        [XenbusStateInitialising] = {tapback_device_switch_state, 
XenbusStateInitWait},
(so, when f.e. goes to Initialising, call tapback_... and switch to InitWait).

instead of relying on the order of the array, otherwise you have to be
careful to get thing in the correct slot in the array.


Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel