On Thu, Oct 06, 2022 at 05:41:39PM +0100, Alberto Faria wrote:
> On Tue, Sep 27, 2022 at 8:34 PM Stefan Hajnoczi <stefa...@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
signature.asc
Description: PGP signature
