On Tue, May 04, 2021 at 03:44:23PM +0200, Kevin Wolf wrote:
> Am 30.04.2021 um 17:49 hat Stefan Hajnoczi geschrieben:
> > On Thu, Apr 29, 2021 at 05:51:16PM +0200, Kevin Wolf wrote:
> > > Am 29.04.2021 um 16:05 hat Stefan Hajnoczi geschrieben:
> There is one more thing I'm wondering right now: Why do we have separate
> states for connecting to the backend (created) and configuring it
> (initialized)? The property setters check for the right state, but they
> don't really do anything that wouldn't be possible in the other state.
> A state machine exposed as two boolean rather than a tristate enum feels
> a bit awkward, too, but even more so if two states could possibly be
> enough.
> 
> The reason why I'm asking is that in order to address my points, it
> would be best to have separate property accessors for each state, and
> two pairs of accessors would make property declarations more managable
> than three pairs.

There is no need to have separate boolean properties, it's just how I
implemented it. There could be a single state property. For example, a
string that is "uninitialized", "initialized", and "started".

> > > Alternatives in QEMU are qdev properties (which are internally QOM
> > > properties, but provide default implementations and are at least
> > > automatically read-only after realize, avoiding that whole class of
> > > bugs) and QAPI.
> > > If this was QEMU code, I would of course go for QAPI, but a library is
> > > something different and adding the code generator would probably be a
> > > bit too much anyway. But the idea in the resulting code would be dealing
> > > with native structs instead of a bunch of function calls. This would
> > > probably be the least error prone way for the implementation, but of
> > > course, it would make binary compatibility a bit harder when adding new
> > > properties.
> > 
> > An alternative I considered was the typestate and builder patterns:
> > 
> >   /* Create a new io_uring driver in the uninitialized state */
> >   struct blkio_iou_uninit *blkio_new_io_uring(void);
> > 
> >   /* Uninitialized state property setters */
> >   int blkio_iou_uninit_set_path(struct blkio_iou_uninit *u,
> >                                 const char *path);
> >   int blkio_iou_uninit_set_direct(struct blkio_iou_uninit *u,
> >                                   bool o_direct);
> >   ...
> > 
> >   /* Transition to initialized state. Frees u on success. */
> >   struct blkio_iou_init *blkio_iou_init(struct blkio_iou_uninit *u);
> > 
> >   /* Initialized state property setters/getters */
> >   int blkio_iou_init_get_capacity(struct blkio_iou_init *i,
> >                                   uint64_t *capacity);
> >   ...
> > 
> >   /* Transition to started state. Frees i on success. */
> >   struct blkio_iou_started *blkio_iou_start(struct blkio_iou_init *i);
> > 
> >   ...
> > 
> >   /* Transition back to initialized state. Frees s on success. */
> >   struct blkio_iou_init *blkio_iou_stop(struct blkio_iou_started *s);
> > 
> > On the plus side:
> > 
> > - No state checks are needed because an API won't even exist if it's
> >   unavailable in a given state (uninitialized/initialized/started).
> > 
> > - State structs come with pre-initialized default values, so the caller
> >   only needs to set non-default values. For example O_DIRECT is false by
> >   default and callers happy with that don't need to set the property.
> > 
> > - ABI compatibility is easy since the state structs are opaque (their
> >   size is not defined) and new properties can be added at any time.
> > 
> > On the minus side:
> > 
> > - Completely static. Hard to introspect and requires a dedicated call
> >   site for each property (applications cannot simply assign a property
> >   string given to them on the command-line). This means every single
> >   property must be explicitly coded into every application :(.
> 
> How are you going to deal with this for QEMU integration, by the way?
> Put all the properties that we know into the QAPI schema and then some
> way of passing key/value pairs for the rest?

In QEMU's case let's define each property explicitly instead of passing
them through. That's due to QAPI's philosophy rather than libblkio.

> > - So many functions! This makes understanding the API harder.
> > 
> > - Very verbose. The function and type names get long and there is a lot
> >   of repetition in the API.
> 
> I think it wouldn't be too bad if all drivers exposed the same
> properties, but you're explicitly expecting driver-specific properties.
> If drivers add an external APIs that just fail for other drivers, it
> would indeed make understanding the API much harder.
> 
> We could consider a mix where you would first create a configuration
> object, then use the generic property functions to set options for it
> and finally have a separate blkio_initialize() function where you turn
> that config into a struct blkio that is needed to actually do I/O (and
> also supports generic property functions for runtime option updates).
> 
> I'm not sure it provides much except making the state machine more
> prominent than just two random bool properties.

I prefer to keep the configuration public API as it is. We can change
the properties.rs implementation however we want though.

Do you think the public API should be a typestate API instead with
struct blkio_init_info, struct blkio_start_info, and struct blkio
expressing the 3 states instead?

Stefan