Re: [PULL v2 03/16] block/block-backend: add block layer APIs resembling Linux ZonedBlockDevice ioctls
On Fri, May 03, 2024 at 01:33:51PM +0100, Peter Maydell wrote:
> On Mon, 15 May 2023 at 17:07, Stefan Hajnoczi wrote:
> >
> > From: Sam Li
> >
> > Add zoned device option to host_device BlockDriver. It will be presented
> > only
> > for zoned host block devices. By adding zone management operations to the
> > host_block_device BlockDriver, users can use the new block layer APIs
> > including Report Zone and four zone management operations
> > (open, close, finish, reset, reset_all).
> >
> > Qemu-io uses the new APIs to perform zoned storage commands of the device:
> > zone_report(zrp), zone_open(zo), zone_close(zc), zone_reset(zrs),
> > zone_finish(zf).
> >
> > For example, to test zone_report, use following command:
> > $ ./build/qemu-io --image-opts -n driver=host_device, filename=/dev/nullb0
> > -c "zrp offset nr_zones"
>
> Hi; Coverity points out an issue in this commit (CID 1544771):
>
> > +static int zone_report_f(BlockBackend *blk, int argc, char **argv)
> > +{
> > +int ret;
> > +int64_t offset;
> > +unsigned int nr_zones;
> > +
> > +++optind;
> > +offset = cvtnum(argv[optind]);
> > +++optind;
> > +nr_zones = cvtnum(argv[optind]);
>
> cvtnum() can fail and return a negative value on error
> (e.g. if the number in the string is out of range),
> but we are not checking for that. Instead we stuff
> the value into an 'unsigned int' and then pass that to
> g_new(), which will result in our trying to allocate a large
> amount of memory.
>
> Here, and also in the other functions below that use cvtnum(),
> I think we should follow the pattern for use of that function
> that is used in the pre-existing code in this function:
>
> int64_t foo; /* NB: not an unsigned or some smaller type */
>
> foo = cvtnum(arg)
> if (foo < 0) {
> print_cvtnum_err(foo, arg);
> return foo; /* or otherwise handle returning an error upward */
> }
>
> It looks like all the uses of cvtnum in this patch should be
> adjusted to handle errors.
Thanks for letting me know. I will send a patch.
Stefan
signature.asc
Description: PGP signature
Re: [PULL v2 03/16] block/block-backend: add block layer APIs resembling Linux ZonedBlockDevice ioctls
On Mon, 15 May 2023 at 17:07, Stefan Hajnoczi wrote:
>
> From: Sam Li
>
> Add zoned device option to host_device BlockDriver. It will be presented only
> for zoned host block devices. By adding zone management operations to the
> host_block_device BlockDriver, users can use the new block layer APIs
> including Report Zone and four zone management operations
> (open, close, finish, reset, reset_all).
>
> Qemu-io uses the new APIs to perform zoned storage commands of the device:
> zone_report(zrp), zone_open(zo), zone_close(zc), zone_reset(zrs),
> zone_finish(zf).
>
> For example, to test zone_report, use following command:
> $ ./build/qemu-io --image-opts -n driver=host_device, filename=/dev/nullb0
> -c "zrp offset nr_zones"
Hi; Coverity points out an issue in this commit (CID 1544771):
> +static int zone_report_f(BlockBackend *blk, int argc, char **argv)
> +{
> +int ret;
> +int64_t offset;
> +unsigned int nr_zones;
> +
> +++optind;
> +offset = cvtnum(argv[optind]);
> +++optind;
> +nr_zones = cvtnum(argv[optind]);
cvtnum() can fail and return a negative value on error
(e.g. if the number in the string is out of range),
but we are not checking for that. Instead we stuff
the value into an 'unsigned int' and then pass that to
g_new(), which will result in our trying to allocate a large
amount of memory.
Here, and also in the other functions below that use cvtnum(),
I think we should follow the pattern for use of that function
that is used in the pre-existing code in this function:
int64_t foo; /* NB: not an unsigned or some smaller type */
foo = cvtnum(arg)
if (foo < 0) {
print_cvtnum_err(foo, arg);
return foo; /* or otherwise handle returning an error upward */
}
It looks like all the uses of cvtnum in this patch should be
adjusted to handle errors.
thanks
-- PMM
[PULL v2 03/16] block/block-backend: add block layer APIs resembling Linux ZonedBlockDevice ioctls
From: Sam Li
Add zoned device option to host_device BlockDriver. It will be presented only
for zoned host block devices. By adding zone management operations to the
host_block_device BlockDriver, users can use the new block layer APIs
including Report Zone and four zone management operations
(open, close, finish, reset, reset_all).
Qemu-io uses the new APIs to perform zoned storage commands of the device:
zone_report(zrp), zone_open(zo), zone_close(zc), zone_reset(zrs),
zone_finish(zf).
For example, to test zone_report, use following command:
$ ./build/qemu-io --image-opts -n driver=host_device, filename=/dev/nullb0
-c "zrp offset nr_zones"
Signed-off-by: Sam Li
Reviewed-by: Hannes Reinecke
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Dmitry Fomichev
Acked-by: Kevin Wolf
Signed-off-by: Stefan Hajnoczi
Message-id: 20230508045533.175575-4-faithilike...@gmail.com
Message-id: 20230324090605.28361-4-faithilike...@gmail.com
[Adjust commit message prefix as suggested by Philippe Mathieu-Daudé
and remove spurious ret = -errno in
raw_co_zone_mgmt().
--Stefan]
Signed-off-by: Stefan Hajnoczi
---
meson.build | 5 +
include/block/block-io.h | 9 +
include/block/block_int-common.h | 21 ++
include/block/raw-aio.h | 6 +-
include/sysemu/block-backend-io.h | 18 ++
block/block-backend.c | 137 +
block/file-posix.c| 313 +-
block/io.c| 41
qemu-io-cmds.c| 149 ++
9 files changed, 696 insertions(+), 3 deletions(-)
diff --git a/meson.build b/meson.build
index d3cf48960b..25a4b9f2c1 100644
--- a/meson.build
+++ b/meson.build
@@ -2025,6 +2025,8 @@ if rdma.found()
endif
# has_header_symbol
+config_host_data.set('CONFIG_BLKZONED',
+ cc.has_header_symbol('linux/blkzoned.h', 'BLKOPENZONE'))
config_host_data.set('CONFIG_EPOLL_CREATE1',
cc.has_header_symbol('sys/epoll.h', 'epoll_create1'))
config_host_data.set('CONFIG_FALLOCATE_PUNCH_HOLE',
@@ -2060,6 +2062,9 @@ config_host_data.set('HAVE_SIGEV_NOTIFY_THREAD_ID',
config_host_data.set('HAVE_STRUCT_STAT_ST_ATIM',
cc.has_member('struct stat', 'st_atim',
prefix: '#include '))
+config_host_data.set('HAVE_BLK_ZONE_REP_CAPACITY',
+ cc.has_member('struct blk_zone', 'capacity',
+ prefix: '#include '))
# has_type
config_host_data.set('CONFIG_IOVEC',
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 1f612ec5bd..f099b204bc 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -114,6 +114,15 @@ int coroutine_fn GRAPH_RDLOCK
bdrv_co_flush(BlockDriverState *bs);
int coroutine_fn GRAPH_RDLOCK bdrv_co_pdiscard(BdrvChild *child, int64_t
offset,
int64_t bytes);
+/* Report zone information of zone block device. */
+int coroutine_fn GRAPH_RDLOCK bdrv_co_zone_report(BlockDriverState *bs,
+ int64_t offset,
+ unsigned int *nr_zones,
+ BlockZoneDescriptor *zones);
+int coroutine_fn GRAPH_RDLOCK bdrv_co_zone_mgmt(BlockDriverState *bs,
+BlockZoneOp op,
+int64_t offset, int64_t len);
+
bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
int bdrv_block_status(BlockDriverState *bs, int64_t offset,
int64_t bytes, int64_t *pnum, int64_t *map,
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index c7ca5a83e9..b2612f06ec 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -713,6 +713,12 @@ struct BlockDriver {
int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_load_vmstate)(
BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
+int coroutine_fn (*bdrv_co_zone_report)(BlockDriverState *bs,
+int64_t offset, unsigned int *nr_zones,
+BlockZoneDescriptor *zones);
+int coroutine_fn (*bdrv_co_zone_mgmt)(BlockDriverState *bs, BlockZoneOp op,
+int64_t offset, int64_t len);
+
/* removable device specific */
bool coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_is_inserted)(
BlockDriverState *bs);
@@ -865,6 +871,21 @@ typedef struct BlockLimits {
/* device zone model */
BlockZoneModel zoned;
+
+/* zone size expressed in bytes */
+uint32_t zone_size;
+
+/* total number of zones */
+uint32_t nr_zones;
+
+/* maximum sectors of a zone append write operation */
+uint32_t max_append_sectors;
+
+/* maximum number of open zones */
+uint32_t max_open_zones;
+
+/* maximum number of active zones */
+uint32_t max_active_zones;
} BlockLimits;
