Re: [PATCH 2/3] block: do not check bdrv_file_open
On Thu, Jan 19, 2023 at 2:17 PM Kevin Wolf wrote:
> > assert(!drv->bdrv_needs_filename || bs->filename[0]);
> > -if (drv->bdrv_file_open) {
> > +if (drv->bdrv_open) {
> > ret = drv->bdrv_file_open(bs, options, open_flags, _err);
> > } else if (drv->bdrv_open) {
> > ret = drv->bdrv_open(bs, options, open_flags, _err);
>
> I suppose you mean drv->protocol_name for the first if condition?
>
> The bug will disappear again after patch 3, but this intermediate state
> is very broken.
Yep, I split the patch wrong. Will resend after you merge block-next.
Paolo
[PATCH 2/3] block: do not check bdrv_file_open
The set of BlockDrivers that have .bdrv_file_open coincides with those
that have .protocol_name and guess what---checking drv->bdrv_file_open
is done to see if the driver is a protocol. So check drv->protocol_name
instead.
Signed-off-by: Paolo Bonzini
---
block.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index 075da6517b7f..71f0ea24870e 100644
--- a/block.c
+++ b/block.c
@@ -914,7 +914,6 @@ BlockDriver *bdrv_find_protocol(const char *filename,
int i;
GLOBAL_STATE_CODE();
-/* TODO Drivers without bdrv_file_open must be specified explicitly */
/*
* XXX(hch): we really should not let host device detection
@@ -1628,7 +1627,7 @@ static int bdrv_open_driver(BlockDriverState *bs,
BlockDriver *drv,
bs->opaque = g_malloc0(drv->instance_size);
assert(!drv->bdrv_needs_filename || bs->filename[0]);
-if (drv->bdrv_file_open) {
+if (drv->bdrv_open) {
ret = drv->bdrv_file_open(bs, options, open_flags, _err);
} else if (drv->bdrv_open) {
ret = drv->bdrv_open(bs, options, open_flags, _err);
@@ -1940,7 +1939,7 @@ static int bdrv_open_common(BlockDriverState *bs,
BlockBackend *file,
open_flags = bdrv_open_flags(bs, bs->open_flags);
node_name = qemu_opt_get(opts, "node-name");
-assert(!drv->bdrv_file_open || file == NULL);
+assert(!drv->protocol_name || file == NULL);
ret = bdrv_open_driver(bs, drv, node_name, options, open_flags, errp);
if (ret < 0) {
goto fail_opts;
@@ -2040,7 +2039,7 @@ static int bdrv_fill_options(QDict **options, const char
*filename,
}
/* If the user has explicitly specified the driver, this choice should
* override the BDRV_O_PROTOCOL flag */
-protocol = drv->bdrv_file_open;
+protocol = drv->protocol_name;
}
if (protocol) {
@@ -4000,7 +3999,7 @@ bdrv_open_inherit(const char *filename, const char
*reference, QDict *options,
}
/* BDRV_O_PROTOCOL must be set iff a protocol BDS is about to be created */
-assert(!!(flags & BDRV_O_PROTOCOL) == !!drv->bdrv_file_open);
+assert(!!(flags & BDRV_O_PROTOCOL) == !!drv->protocol_name);
/* file must be NULL if a protocol BDS is about to be created
* (the inverse results in an error message from bdrv_open_common()) */
assert(!(flags & BDRV_O_PROTOCOL) || !file);
@@ -5785,7 +5784,7 @@ int64_t coroutine_fn
bdrv_co_get_allocated_file_size(BlockDriverState *bs)
return drv->bdrv_co_get_allocated_file_size(bs);
}
-if (drv->bdrv_file_open) {
+if (drv->protocol_name) {
/*
* Protocol drivers default to -ENOTSUP (most of their data is
* not stored in any of their children (if they even have any),
@@ -7888,7 +7887,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
* Both of these conditions are represented by
generate_json_filename.
*/
if (primary_child_bs->exact_filename[0] &&
-primary_child_bs->drv->bdrv_file_open &&
+primary_child_bs->drv->protocol_name &&
!drv->is_filter && !generate_json_filename)
{
strcpy(bs->exact_filename, primary_child_bs->exact_filename);
--
2.39.2
Re: [PATCH 2/3] block: do not check bdrv_file_open
Am 12.12.2022 um 14:16 hat Paolo Bonzini geschrieben:
> The set of BlockDrivers that have .bdrv_file_open coincides with those
> that have .protocol_name and guess what---checking drv->bdrv_file_open
> is done to see if the driver is a protocol. So check drv->protocol_name
> instead.
>
> Signed-off-by: Paolo Bonzini
> ---
> block.c | 13 ++---
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/block.c b/block.c
> index 0a625a489a6e..7a66cc2ea23a 100644
> --- a/block.c
> +++ b/block.c
> @@ -911,7 +911,6 @@ BlockDriver *bdrv_find_protocol(const char *filename,
> int i;
>
> GLOBAL_STATE_CODE();
> -/* TODO Drivers without bdrv_file_open must be specified explicitly */
>
> /*
> * XXX(hch): we really should not let host device detection
> @@ -1618,7 +1617,7 @@ static int bdrv_open_driver(BlockDriverState *bs,
> BlockDriver *drv,
> bs->opaque = g_malloc0(drv->instance_size);
>
> assert(!drv->bdrv_needs_filename || bs->filename[0]);
> -if (drv->bdrv_file_open) {
> +if (drv->bdrv_open) {
> ret = drv->bdrv_file_open(bs, options, open_flags, _err);
> } else if (drv->bdrv_open) {
> ret = drv->bdrv_open(bs, options, open_flags, _err);
I suppose you mean drv->protocol_name for the first if condition?
The bug will disappear again after patch 3, but this intermediate state
is very broken.
Kevin
[PATCH 2/3] block: do not check bdrv_file_open
The set of BlockDrivers that have .bdrv_file_open coincides with those
that have .protocol_name and guess what---checking drv->bdrv_file_open
is done to see if the driver is a protocol. So check drv->protocol_name
instead.
Signed-off-by: Paolo Bonzini
---
block.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index 0a625a489a6e..7a66cc2ea23a 100644
--- a/block.c
+++ b/block.c
@@ -911,7 +911,6 @@ BlockDriver *bdrv_find_protocol(const char *filename,
int i;
GLOBAL_STATE_CODE();
-/* TODO Drivers without bdrv_file_open must be specified explicitly */
/*
* XXX(hch): we really should not let host device detection
@@ -1618,7 +1617,7 @@ static int bdrv_open_driver(BlockDriverState *bs,
BlockDriver *drv,
bs->opaque = g_malloc0(drv->instance_size);
assert(!drv->bdrv_needs_filename || bs->filename[0]);
-if (drv->bdrv_file_open) {
+if (drv->bdrv_open) {
ret = drv->bdrv_file_open(bs, options, open_flags, _err);
} else if (drv->bdrv_open) {
ret = drv->bdrv_open(bs, options, open_flags, _err);
@@ -1930,7 +1929,7 @@ static int bdrv_open_common(BlockDriverState *bs,
BlockBackend *file,
open_flags = bdrv_open_flags(bs, bs->open_flags);
node_name = qemu_opt_get(opts, "node-name");
-assert(!drv->bdrv_file_open || file == NULL);
+assert(!drv->protocol_name || file == NULL);
ret = bdrv_open_driver(bs, drv, node_name, options, open_flags, errp);
if (ret < 0) {
goto fail_opts;
@@ -2030,7 +2029,7 @@ static int bdrv_fill_options(QDict **options, const char
*filename,
}
/* If the user has explicitly specified the driver, this choice should
* override the BDRV_O_PROTOCOL flag */
-protocol = drv->bdrv_file_open;
+protocol = drv->protocol_name;
}
if (protocol) {
@@ -3932,7 +3931,7 @@ static BlockDriverState *bdrv_open_inherit(const char
*filename,
}
/* BDRV_O_PROTOCOL must be set iff a protocol BDS is about to be created */
-assert(!!(flags & BDRV_O_PROTOCOL) == !!drv->bdrv_file_open);
+assert(!!(flags & BDRV_O_PROTOCOL) == !!drv->protocol_name);
/* file must be NULL if a protocol BDS is about to be created
* (the inverse results in an error message from bdrv_open_common()) */
assert(!(flags & BDRV_O_PROTOCOL) || !file);
@@ -5671,7 +5670,7 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs)
return drv->bdrv_get_allocated_file_size(bs);
}
-if (drv->bdrv_file_open) {
+if (drv->protocol_name) {
/*
* Protocol drivers default to -ENOTSUP (most of their data is
* not stored in any of their children (if they even have any),
@@ -7772,7 +7771,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
* Both of these conditions are represented by
generate_json_filename.
*/
if (primary_child_bs->exact_filename[0] &&
-primary_child_bs->drv->bdrv_file_open &&
+primary_child_bs->drv->protocol_name &&
!drv->is_filter && !generate_json_filename)
{
strcpy(bs->exact_filename, primary_child_bs->exact_filename);
--
2.38.1
