Re: [PATCH v5 02/12] blkio: add libblkio block driver

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6755 bytes --]

On Thu, Oct 06, 2022 at 05:41:39PM +0100, Alberto Faria wrote:
> On Tue, Sep 27, 2022 at 8:34 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > +static int blkio_virtio_blk_vhost_user_open(BlockDriverState *bs,
> > +        QDict *options, int flags, Error **errp)
> > +{
> > +    const char *path = qdict_get_try_str(options, "path");
> > +    BDRVBlkioState *s = bs->opaque;
> > +    int ret;
> > +
> > +    if (!path) {
> > +        error_setg(errp, "missing 'path' option");
> > +        return -EINVAL;
> > +    }
> > +
> > +    ret = blkio_set_str(s->blkio, "path", path);
> > +    qdict_del(options, "path");
> > +    if (ret < 0) {
> > +        error_setg_errno(errp, -ret, "failed to set path: %s",
> > +                         blkio_get_error_msg());
> > +        return ret;
> > +    }
> > +
> > +    if (!(flags & BDRV_O_NOCACHE)) {
> > +        error_setg(errp, "cache.direct=off is not supported");
> > +        return -EINVAL;
> > +    }
> > +    return 0;
> > +}
> > +
> > +static int blkio_virtio_blk_vhost_vdpa_open(BlockDriverState *bs,
> > +        QDict *options, int flags, Error **errp)
> > +{
> > +    const char *path = qdict_get_try_str(options, "path");
> > +    BDRVBlkioState *s = bs->opaque;
> > +    int ret;
> > +
> > +    if (!path) {
> > +        error_setg(errp, "missing 'path' option");
> > +        return -EINVAL;
> > +    }
> > +
> > +    ret = blkio_set_str(s->blkio, "path", path);
> > +    qdict_del(options, "path");
> > +    if (ret < 0) {
> > +        error_setg_errno(errp, -ret, "failed to set path: %s",
> > +                         blkio_get_error_msg());
> > +        return ret;
> > +    }
> > +
> > +    if (!(flags & BDRV_O_NOCACHE)) {
> > +        error_setg(errp, "cache.direct=off is not supported");
> > +        return -EINVAL;
> > +    }
> > +    return 0;
> > +}
> 
> These two functions are (currently) exact duplicates. Should we just
> have a single blkio_virtio_blk_open() function?

Will fix.

> > +static BlockDriver bdrv_io_uring = {
> > +    .format_name                = DRIVER_IO_URING,
> > +    .protocol_name              = DRIVER_IO_URING,
> > +    .instance_size              = sizeof(BDRVBlkioState),
> > +    .bdrv_needs_filename        = true,
> > +    .bdrv_file_open             = blkio_file_open,
> > +    .bdrv_close                 = blkio_close,
> > +    .bdrv_getlength             = blkio_getlength,
> > +    .bdrv_get_info              = blkio_get_info,
> > +    .bdrv_attach_aio_context    = blkio_attach_aio_context,
> > +    .bdrv_detach_aio_context    = blkio_detach_aio_context,
> > +    .bdrv_co_pdiscard           = blkio_co_pdiscard,
> > +    .bdrv_co_preadv             = blkio_co_preadv,
> > +    .bdrv_co_pwritev            = blkio_co_pwritev,
> > +    .bdrv_co_flush_to_disk      = blkio_co_flush,
> > +    .bdrv_co_pwrite_zeroes      = blkio_co_pwrite_zeroes,
> > +    .bdrv_io_unplug             = blkio_io_unplug,
> > +    .bdrv_refresh_limits        = blkio_refresh_limits,
> > +};
> > +
> > +static BlockDriver bdrv_virtio_blk_vhost_user = {
> > +    .format_name                = DRIVER_VIRTIO_BLK_VHOST_USER,
> > +    .protocol_name              = DRIVER_VIRTIO_BLK_VHOST_USER,
> > +    .instance_size              = sizeof(BDRVBlkioState),
> > +    .bdrv_file_open             = blkio_file_open,
> > +    .bdrv_close                 = blkio_close,
> > +    .bdrv_getlength             = blkio_getlength,
> > +    .bdrv_get_info              = blkio_get_info,
> > +    .bdrv_attach_aio_context    = blkio_attach_aio_context,
> > +    .bdrv_detach_aio_context    = blkio_detach_aio_context,
> > +    .bdrv_co_pdiscard           = blkio_co_pdiscard,
> > +    .bdrv_co_preadv             = blkio_co_preadv,
> > +    .bdrv_co_pwritev            = blkio_co_pwritev,
> > +    .bdrv_co_flush_to_disk      = blkio_co_flush,
> > +    .bdrv_co_pwrite_zeroes      = blkio_co_pwrite_zeroes,
> > +    .bdrv_io_unplug             = blkio_io_unplug,
> > +    .bdrv_refresh_limits        = blkio_refresh_limits,
> > +};
> > +
> > +static BlockDriver bdrv_virtio_blk_vhost_vdpa = {
> > +    .format_name                = DRIVER_VIRTIO_BLK_VHOST_VDPA,
> > +    .protocol_name              = DRIVER_VIRTIO_BLK_VHOST_VDPA,
> > +    .instance_size              = sizeof(BDRVBlkioState),
> > +    .bdrv_file_open             = blkio_file_open,
> > +    .bdrv_close                 = blkio_close,
> > +    .bdrv_getlength             = blkio_getlength,
> > +    .bdrv_get_info              = blkio_get_info,
> > +    .bdrv_attach_aio_context    = blkio_attach_aio_context,
> > +    .bdrv_detach_aio_context    = blkio_detach_aio_context,
> > +    .bdrv_co_pdiscard           = blkio_co_pdiscard,
> > +    .bdrv_co_preadv             = blkio_co_preadv,
> > +    .bdrv_co_pwritev            = blkio_co_pwritev,
> > +    .bdrv_co_flush_to_disk      = blkio_co_flush,
> > +    .bdrv_co_pwrite_zeroes      = blkio_co_pwrite_zeroes,
> > +    .bdrv_io_unplug             = blkio_io_unplug,
> > +    .bdrv_refresh_limits        = blkio_refresh_limits,
> > +};
> 
> It's difficult to identify the fields that differ between
> BlockDrivers. Maybe we could do something like:
> 
>     #define DRIVER(name, ...) \
>         { \
>             .format_name             = name, \
>             .protocol_name           = name, \
>             .instance_size           = sizeof(BDRVBlkioState), \
>             .bdrv_file_open          = blkio_file_open, \
>             .bdrv_close              = blkio_close, \
>             .bdrv_getlength          = blkio_getlength, \
>             .bdrv_get_info           = blkio_get_info, \
>             .bdrv_attach_aio_context = blkio_attach_aio_context, \
>             .bdrv_detach_aio_context = blkio_detach_aio_context, \
>             .bdrv_co_pdiscard        = blkio_co_pdiscard, \
>             .bdrv_co_preadv          = blkio_co_preadv, \
>             .bdrv_co_pwritev         = blkio_co_pwritev, \
>             .bdrv_co_flush_to_disk   = blkio_co_flush, \
>             .bdrv_co_pwrite_zeroes   = blkio_co_pwrite_zeroes, \
>             .bdrv_io_unplug          = blkio_io_unplug, \
>             .bdrv_refresh_limits     = blkio_refresh_limits, \
>             __VA_ARGS__
>         } \
> 
>     static BlockDriver bdrv_io_uring = DRIVER(
>         DRIVER_IO_URING,
>         .bdrv_needs_filename = true,
>     );
> 
>     static BlockDriver bdrv_virtio_blk_vhost_user = DRIVER(
>         DRIVER_VIRTIO_BLK_VHOST_USER
>     );
> 
>     static BlockDriver bdrv_virtio_blk_vhost_vdpa = DRIVER(
>         DRIVER_VIRTIO_BLK_VHOST_VDPA
>     );

Makes sense to me.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]