Re: [ANNOUNCE] libblkio v0.1.0 preview release

[Stefan Hajnoczi]

On Thu, Apr 29, 2021 at 05:51:16PM +0200, Kevin Wolf wrote:
> Am 29.04.2021 um 16:05 hat Stefan Hajnoczi geschrieben:
> > Hi,
> > A preview release of libblkio, a library for high-performance block I/O,
> > is now available:
> > 
> >   https://gitlab.com/libblkio/libblkio/-/tree/v0.1.0
> > 
> > Applications are increasingly integrating high-performance I/O
> > interfaces such as Linux io_uring, userspace device drivers, and
> > vhost-user device support. The effort required to add each of these
> > low-level interfaces into an application is relatively high. libblkio
> > provides a single API for efficiently accessing block devices and
> > eliminates the need to write custom code for each one.
> > 
> > The library is not yet ready to use and currently lacks many features.
> > In fact, I hope this news doesn't spread too far yet because libblkio is
> > at a very early stage, but feedback from the QEMU community is necessary
> > at this time.
> 
> I'm a bit worried about the configuration interface. This looks an awful
> lot like plain QOM properties, which have proven to result in both very
> verbose (not to say messy) and rather error prone implementations.
> 
> You have to write setters/getters for every property, even if it usually
> ends up doing the same thing, storing the value somewhere. Worse, these
> getters and setters have to work in very different circumstances, namely
> initialisation where you usually have to store the value away so that it
> can be checked for consistency with other properties in .realize() or
> .complete(), and property updates during runtime. Often enough, we
> forget the latter and create bugs. If we don't create bugs, we usually
> start with 'if (realized)' and have two completely different code paths.
> Another common bug in QOM objects is forgetting to check if mandatory
> properties were actually set.
>
> Did you already consider these problems and decided to go this way
> anyway, or is this more of an accidental design? And if the former, what
> were the reasons that made it appealing?

That's true. Here is the code to reject accesses when the instance is
not initialized:

  self.must_be_initialized()?;

It's very concise but you still need to remember to add it.

The overall reasons for choosing the properties API were:

1. It keeps the library's API very minimal (just generic getters/setters
   for primitive types). It minimizes ABI compatibility issues because
   there are no configuration structs or functions exported by the
   library.

2. It's trivial to have a string setter/getter that automatically
   converts to the primitive type representation, so application config
   file or command-line values can easily be set.

   This is kind of the inverse of what you're saying. If the library
   offers dedicated interfaces for each configuration value then the
   library doesn't need getters/setters for each one but all
   applications need special-purpose code for each configuration value.

That said, this is exactly why I published the preview release. If
someone has a better way to do this or the feedback is just that this is
bad style, then I'm happy to change it.

> 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 :(.

- 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.

> Thinking a bit further, we'll likely get the same problems as with QOM
> in other places, too, e.g. how do you introspect which properties exist
> in a given build?

Right, there is no introspection although probing individual properties
is possible (-ENOENT). Adding introspection wouldn't be hard but the
library API just doesn't exist today.

Stefan

signature.asc
Description: PGP signature