* [PATCH 0/3] block: drop BlockDriverState::read_only
@ 2021-05-26 21:15 Vladimir Sementsov-Ogievskiy
2021-05-26 21:15 ` [PATCH 1/3] block: consistently use bdrv_is_read_only() Vladimir Sementsov-Ogievskiy
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-26 21:15 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, codyprime, armbru, fam, stefanha, vsementsov, jsnow,
mreitz, kwolf
Hi all!
The field duplicates information in .open_flags. We have to carefully
sync these two fields everywhere. It's simple to introduce a bug by
forgetting it.
Let's drop the field, and fix users to call bdrv_is_read_only() and
bdrv_is_writable() instead.
Vladimir Sementsov-Ogievskiy (3):
block: consistently use bdrv_is_read_only()
block: drop BlockDriverState::read_only
block: drop BlockBackendRootState::read_only
include/block/block_int.h | 2 --
block.c | 16 +++++++---------
block/block-backend.c | 10 ++--------
block/commit.c | 2 +-
block/io.c | 4 ++--
block/qapi.c | 2 +-
block/qcow2-snapshot.c | 2 +-
block/qcow2.c | 4 ++--
block/snapshot.c | 2 +-
block/vhdx-log.c | 2 +-
blockdev.c | 4 +++-
tests/unit/test-block-iothread.c | 6 ------
12 files changed, 21 insertions(+), 35 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/3] block: consistently use bdrv_is_read_only()
2021-05-26 21:15 [PATCH 0/3] block: drop BlockDriverState::read_only Vladimir Sementsov-Ogievskiy
@ 2021-05-26 21:15 ` Vladimir Sementsov-Ogievskiy
2021-05-27 14:48 ` Kevin Wolf
2021-05-26 21:15 ` [PATCH 2/3] block: drop BlockDriverState::read_only Vladimir Sementsov-Ogievskiy
2021-05-26 21:15 ` [PATCH 3/3] block: drop BlockBackendRootState::read_only Vladimir Sementsov-Ogievskiy
2 siblings, 1 reply; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-26 21:15 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, codyprime, armbru, fam, stefanha, vsementsov, jsnow,
mreitz, kwolf
It's better to use accessor function instead of bs->read_only directly.
In some places use bdrv_is_writable() instead of
checking both BDRV_O_RDWR set and BDRV_O_INACTIVE not set.
In bdrv_open_common() it's a bit strange to add one more variable, but
we are going to drop bs->read_only in the next patch, so new ro local
variable substitutes it here.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block.c | 11 +++++++----
block/block-backend.c | 2 +-
block/commit.c | 2 +-
block/io.c | 4 ++--
block/qapi.c | 2 +-
block/qcow2-snapshot.c | 2 +-
block/qcow2.c | 4 ++--
block/snapshot.c | 2 +-
block/vhdx-log.c | 2 +-
9 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/block.c b/block.c
index 0dc97281dc..0270b609c4 100644
--- a/block.c
+++ b/block.c
@@ -1720,6 +1720,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
QemuOpts *opts;
BlockDriver *drv;
Error *local_err = NULL;
+ bool ro;
assert(bs->file == NULL);
assert(options != NULL && bs->options != options);
@@ -1772,15 +1773,17 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
bs->read_only = !(bs->open_flags & BDRV_O_RDWR);
- if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
- if (!bs->read_only && bdrv_is_whitelisted(drv, true)) {
+ ro = bdrv_is_read_only(bs);
+
+ if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, ro)) {
+ if (!ro && bdrv_is_whitelisted(drv, true)) {
ret = bdrv_apply_auto_read_only(bs, NULL, NULL);
} else {
ret = -ENOTSUP;
}
if (ret < 0) {
error_setg(errp,
- !bs->read_only && bdrv_is_whitelisted(drv, true)
+ !ro && bdrv_is_whitelisted(drv, true)
? "Driver '%s' can only be used for read-only devices"
: "Driver '%s' is not whitelisted",
drv->format_name);
@@ -1792,7 +1795,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
assert(qatomic_read(&bs->copy_on_read) == 0);
if (bs->open_flags & BDRV_O_COPY_ON_READ) {
- if (!bs->read_only) {
+ if (!ro) {
bdrv_enable_copy_on_read(bs);
} else {
error_setg(errp, "Can't use copy-on-read on read-only device");
diff --git a/block/block-backend.c b/block/block-backend.c
index de5496af66..21b834e9df 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2269,7 +2269,7 @@ void blk_update_root_state(BlockBackend *blk)
assert(blk->root);
blk->root_state.open_flags = blk->root->bs->open_flags;
- blk->root_state.read_only = blk->root->bs->read_only;
+ blk->root_state.read_only = bdrv_is_read_only(blk->root->bs);
blk->root_state.detect_zeroes = blk->root->bs->detect_zeroes;
}
diff --git a/block/commit.c b/block/commit.c
index b89bb20b75..b7f0c7c061 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -453,7 +453,7 @@ int bdrv_commit(BlockDriverState *bs)
return -EBUSY;
}
- ro = backing_file_bs->read_only;
+ ro = bdrv_is_read_only(backing_file_bs);
if (ro) {
if (bdrv_reopen_set_read_only(backing_file_bs, false, NULL)) {
diff --git a/block/io.c b/block/io.c
index 1e826ba9e8..323854d063 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1973,7 +1973,7 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, int64_t bytes,
bdrv_check_request(offset, bytes, &error_abort);
- if (bs->read_only) {
+ if (bdrv_is_read_only(bs)) {
return -EPERM;
}
@@ -3406,7 +3406,7 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
if (new_bytes) {
bdrv_make_request_serialising(&req, 1);
}
- if (bs->read_only) {
+ if (bdrv_is_read_only(bs)) {
error_setg(errp, "Image is read-only");
ret = -EACCES;
goto out;
diff --git a/block/qapi.c b/block/qapi.c
index 943e7b15ad..dc69341bfe 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -59,7 +59,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
info = g_malloc0(sizeof(*info));
info->file = g_strdup(bs->filename);
- info->ro = bs->read_only;
+ info->ro = bdrv_is_read_only(bs);
info->drv = g_strdup(bs->drv->format_name);
info->encrypted = bs->encrypted;
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 2e98c7f4b6..71ddb08c21 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -1026,7 +1026,7 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
int new_l1_bytes;
int ret;
- assert(bs->read_only);
+ assert(bdrv_is_read_only(bs));
/* Search the snapshot */
snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
diff --git a/block/qcow2.c b/block/qcow2.c
index 39b91ef940..d39d9f4e9b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1724,7 +1724,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
/* Clear unknown autoclear feature bits */
update_header |= s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK;
update_header =
- update_header && !bs->read_only && !(flags & BDRV_O_INACTIVE);
+ update_header && bdrv_is_writable(bs);
if (update_header) {
s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
}
@@ -1811,7 +1811,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE;
/* Repair image if dirty */
- if (!(flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) && !bs->read_only &&
+ if (!(flags & BDRV_O_CHECK) && bdrv_is_writable(bs) &&
(s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) {
BdrvCheckResult result = {0};
diff --git a/block/snapshot.c b/block/snapshot.c
index e8ae9a28c1..6702c75e42 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -415,7 +415,7 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
error_setg(errp, "snapshot_id and name are both NULL");
return -EINVAL;
}
- if (!bs->read_only) {
+ if (!bdrv_is_read_only(bs)) {
error_setg(errp, "Device is not readonly");
return -EINVAL;
}
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 404fb5f3cb..7672161d95 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -801,7 +801,7 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
}
if (logs.valid) {
- if (bs->read_only) {
+ if (bdrv_is_read_only(bs)) {
bdrv_refresh_filename(bs);
ret = -EPERM;
error_setg(errp,
--
2.29.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] block: drop BlockDriverState::read_only
2021-05-26 21:15 [PATCH 0/3] block: drop BlockDriverState::read_only Vladimir Sementsov-Ogievskiy
2021-05-26 21:15 ` [PATCH 1/3] block: consistently use bdrv_is_read_only() Vladimir Sementsov-Ogievskiy
@ 2021-05-26 21:15 ` Vladimir Sementsov-Ogievskiy
2021-05-26 21:15 ` [PATCH 3/3] block: drop BlockBackendRootState::read_only Vladimir Sementsov-Ogievskiy
2 siblings, 0 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-26 21:15 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, codyprime, armbru, fam, stefanha, vsementsov, jsnow,
mreitz, kwolf
This variable is just a cache for !(bs->open_flags & BDRV_O_RDWR),
which we have to synchronize everywhere. Let's just drop it and
consistently use bdrv_is_read_only().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/block/block_int.h | 1 -
block.c | 7 +------
tests/unit/test-block-iothread.c | 6 ------
3 files changed, 1 insertion(+), 13 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index b2c8b09d0f..09661a134b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -843,7 +843,6 @@ struct BlockDriverState {
* locking needed during I/O...
*/
int open_flags; /* flags used to open the file, re-used for re-open */
- bool read_only; /* if true, the media is read only */
bool encrypted; /* if true, the media is encrypted */
bool sg; /* if true, the device is a /dev/sg* */
bool probed; /* if true, format was probed rather than specified */
diff --git a/block.c b/block.c
index 0270b609c4..d36329e712 100644
--- a/block.c
+++ b/block.c
@@ -265,7 +265,7 @@ void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
* image is inactivated. */
bool bdrv_is_read_only(BlockDriverState *bs)
{
- return bs->read_only;
+ return !(bs->open_flags & BDRV_O_RDWR);
}
int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
@@ -317,7 +317,6 @@ int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
goto fail;
}
- bs->read_only = true;
bs->open_flags &= ~BDRV_O_RDWR;
return 0;
@@ -1549,7 +1548,6 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
}
bs->drv = drv;
- bs->read_only = !(bs->open_flags & BDRV_O_RDWR);
bs->opaque = g_malloc0(drv->instance_size);
if (drv->bdrv_file_open) {
@@ -1771,8 +1769,6 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
trace_bdrv_open_common(bs, filename ?: "", bs->open_flags,
drv->format_name);
- bs->read_only = !(bs->open_flags & BDRV_O_RDWR);
-
ro = bdrv_is_read_only(bs);
if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, ro)) {
@@ -4548,7 +4544,6 @@ static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
bs->explicit_options = reopen_state->explicit_options;
bs->options = reopen_state->options;
bs->open_flags = reopen_state->flags;
- bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
bs->detect_zeroes = reopen_state->detect_zeroes;
if (reopen_state->replace_backing_bs) {
diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c
index 8cf172cb7a..c39e70b2f5 100644
--- a/tests/unit/test-block-iothread.c
+++ b/tests/unit/test-block-iothread.c
@@ -194,13 +194,11 @@ static void test_sync_op_truncate(BdrvChild *c)
g_assert_cmpint(ret, ==, -EINVAL);
/* Error: Read-only image */
- c->bs->read_only = true;
c->bs->open_flags &= ~BDRV_O_RDWR;
ret = bdrv_truncate(c, 65536, false, PREALLOC_MODE_OFF, 0, NULL);
g_assert_cmpint(ret, ==, -EACCES);
- c->bs->read_only = false;
c->bs->open_flags |= BDRV_O_RDWR;
}
@@ -236,13 +234,11 @@ static void test_sync_op_flush(BdrvChild *c)
g_assert_cmpint(ret, ==, 0);
/* Early success: Read-only image */
- c->bs->read_only = true;
c->bs->open_flags &= ~BDRV_O_RDWR;
ret = bdrv_flush(c->bs);
g_assert_cmpint(ret, ==, 0);
- c->bs->read_only = false;
c->bs->open_flags |= BDRV_O_RDWR;
}
@@ -256,13 +252,11 @@ static void test_sync_op_blk_flush(BlockBackend *blk)
g_assert_cmpint(ret, ==, 0);
/* Early success: Read-only image */
- bs->read_only = true;
bs->open_flags &= ~BDRV_O_RDWR;
ret = blk_flush(blk);
g_assert_cmpint(ret, ==, 0);
- bs->read_only = false;
bs->open_flags |= BDRV_O_RDWR;
}
--
2.29.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] block: drop BlockBackendRootState::read_only
2021-05-26 21:15 [PATCH 0/3] block: drop BlockDriverState::read_only Vladimir Sementsov-Ogievskiy
2021-05-26 21:15 ` [PATCH 1/3] block: consistently use bdrv_is_read_only() Vladimir Sementsov-Ogievskiy
2021-05-26 21:15 ` [PATCH 2/3] block: drop BlockDriverState::read_only Vladimir Sementsov-Ogievskiy
@ 2021-05-26 21:15 ` Vladimir Sementsov-Ogievskiy
2021-05-27 14:46 ` Kevin Wolf
2 siblings, 1 reply; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-26 21:15 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, codyprime, armbru, fam, stefanha, vsementsov, jsnow,
mreitz, kwolf
Instead of keeping additional boolean field, let's store the
information in BDRV_O_RDWR bit of BlockBackendRootState::open_flags.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/block/block_int.h | 1 -
block/block-backend.c | 10 ++--------
blockdev.c | 4 +++-
3 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 09661a134b..057d88b1fc 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1007,7 +1007,6 @@ struct BlockDriverState {
struct BlockBackendRootState {
int open_flags;
- bool read_only;
BlockdevDetectZeroesOptions detect_zeroes;
};
diff --git a/block/block-backend.c b/block/block-backend.c
index 21b834e9df..d1a33a2c8e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1852,7 +1852,7 @@ bool blk_supports_write_perm(BlockBackend *blk)
if (bs) {
return !bdrv_is_read_only(bs);
} else {
- return !blk->root_state.read_only;
+ return blk->root_state.open_flags & BDRV_O_RDWR;
}
}
@@ -2269,7 +2269,6 @@ void blk_update_root_state(BlockBackend *blk)
assert(blk->root);
blk->root_state.open_flags = blk->root->bs->open_flags;
- blk->root_state.read_only = bdrv_is_read_only(blk->root->bs);
blk->root_state.detect_zeroes = blk->root->bs->detect_zeroes;
}
@@ -2288,12 +2287,7 @@ bool blk_get_detect_zeroes_from_root_state(BlockBackend *blk)
*/
int blk_get_open_flags_from_root_state(BlockBackend *blk)
{
- int bs_flags;
-
- bs_flags = blk->root_state.read_only ? 0 : BDRV_O_RDWR;
- bs_flags |= blk->root_state.open_flags & ~BDRV_O_RDWR;
-
- return bs_flags;
+ return blk->root_state.open_flags;
}
BlockBackendRootState *blk_get_root_state(BlockBackend *blk)
diff --git a/blockdev.c b/blockdev.c
index 834c2304a1..c36884c691 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -576,6 +576,9 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
}
read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false);
+ if (!read_only) {
+ bdrv_flags |= BDRV_O_RDWR;
+ }
/* init */
if ((!file || !*file) && !qdict_size(bs_opts)) {
@@ -584,7 +587,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
blk_rs = blk_get_root_state(blk);
blk_rs->open_flags = bdrv_flags;
- blk_rs->read_only = read_only;
blk_rs->detect_zeroes = detect_zeroes;
qobject_unref(bs_opts);
--
2.29.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] block: drop BlockBackendRootState::read_only
2021-05-26 21:15 ` [PATCH 3/3] block: drop BlockBackendRootState::read_only Vladimir Sementsov-Ogievskiy
@ 2021-05-27 14:46 ` Kevin Wolf
2021-05-27 15:31 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 7+ messages in thread
From: Kevin Wolf @ 2021-05-27 14:46 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: fam, qemu-block, codyprime, qemu-devel, armbru, stefanha, mreitz, jsnow
Am 26.05.2021 um 23:15 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Instead of keeping additional boolean field, let's store the
> information in BDRV_O_RDWR bit of BlockBackendRootState::open_flags.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> diff --git a/blockdev.c b/blockdev.c
> index 834c2304a1..c36884c691 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -576,6 +576,9 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
> }
>
> read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false);
> + if (!read_only) {
> + bdrv_flags |= BDRV_O_RDWR;
> + }
>
> /* init */
> if ((!file || !*file) && !qdict_size(bs_opts)) {
> @@ -584,7 +587,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
> blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
> blk_rs = blk_get_root_state(blk);
> blk_rs->open_flags = bdrv_flags;
> - blk_rs->read_only = read_only;
> blk_rs->detect_zeroes = detect_zeroes;
>
> qobject_unref(bs_opts);
That we're now using different flags for blk_new_open() looked a bit
suspicious, though the new state intuitively makes more sense than the
old one.
Anyway, this patch breaks ahci-test:
# starting QEMU: exec build/x86_64-softmmu/qemu-system-x86_64 -qtest unix:/tmp/qtest-54104.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-54104.qmp,id=char0 -mon chardev=char0,mode=control -display none -drive file=/tmp/qtest.mGtxeg,if=none,id=drive0,cache=writeback,rerror=stop,werror=stop,format=raw -M q35 -device ide-hd,drive=drive0 -incoming unix:/tmp/qtest-migration.XVRZZi -accel qtest
qemu-system-x86_64: -drive file=/tmp/qtest.mGtxeg,if=none,id=drive0,cache=writeback,rerror=stop,werror=stop,format=raw: Block node is read-only
Kevin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] block: consistently use bdrv_is_read_only()
2021-05-26 21:15 ` [PATCH 1/3] block: consistently use bdrv_is_read_only() Vladimir Sementsov-Ogievskiy
@ 2021-05-27 14:48 ` Kevin Wolf
0 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2021-05-27 14:48 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: fam, qemu-block, codyprime, qemu-devel, armbru, stefanha, mreitz, jsnow
Am 26.05.2021 um 23:15 hat Vladimir Sementsov-Ogievskiy geschrieben:
> It's better to use accessor function instead of bs->read_only directly.
> In some places use bdrv_is_writable() instead of
> checking both BDRV_O_RDWR set and BDRV_O_INACTIVE not set.
>
> In bdrv_open_common() it's a bit strange to add one more variable, but
> we are going to drop bs->read_only in the next patch, so new ro local
> variable substitutes it here.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 39b91ef940..d39d9f4e9b 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1724,7 +1724,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> /* Clear unknown autoclear feature bits */
> update_header |= s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK;
> update_header =
> - update_header && !bs->read_only && !(flags & BDRV_O_INACTIVE);
> + update_header && bdrv_is_writable(bs);
This statement fits on a single line now.
Kevin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] block: drop BlockBackendRootState::read_only
2021-05-27 14:46 ` Kevin Wolf
@ 2021-05-27 15:31 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-27 15:31 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, qemu-devel, codyprime, armbru, fam, stefanha, jsnow, mreitz
27.05.2021 17:46, Kevin Wolf wrote:
> Am 26.05.2021 um 23:15 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Instead of keeping additional boolean field, let's store the
>> information in BDRV_O_RDWR bit of BlockBackendRootState::open_flags.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>> diff --git a/blockdev.c b/blockdev.c
>> index 834c2304a1..c36884c691 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -576,6 +576,9 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
>> }
>>
>> read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false);
>> + if (!read_only) {
>> + bdrv_flags |= BDRV_O_RDWR;
>> + }
>>
>> /* init */
>> if ((!file || !*file) && !qdict_size(bs_opts)) {
>> @@ -584,7 +587,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
>> blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
>> blk_rs = blk_get_root_state(blk);
>> blk_rs->open_flags = bdrv_flags;
>> - blk_rs->read_only = read_only;
>> blk_rs->detect_zeroes = detect_zeroes;
>>
>> qobject_unref(bs_opts);
>
> That we're now using different flags for blk_new_open() looked a bit
> suspicious, though the new state intuitively makes more sense than the
> old one.
>
> Anyway, this patch breaks ahci-test:
>
> # starting QEMU: exec build/x86_64-softmmu/qemu-system-x86_64 -qtest unix:/tmp/qtest-54104.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-54104.qmp,id=char0 -mon chardev=char0,mode=control -display none -drive file=/tmp/qtest.mGtxeg,if=none,id=drive0,cache=writeback,rerror=stop,werror=stop,format=raw -M q35 -device ide-hd,drive=drive0 -incoming unix:/tmp/qtest-migration.XVRZZi -accel qtest
> qemu-system-x86_64: -drive file=/tmp/qtest.mGtxeg,if=none,id=drive0,cache=writeback,rerror=stop,werror=stop,format=raw: Block node is read-only
>
Interesting (me again not running make check, sorry for this :\. And that's fixed by
diff --git a/blockdev.c b/blockdev.c
index c36884c691..2f3f9bc746 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -609,7 +609,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
bdrv_flags |= BDRV_O_INACTIVE;
}
- blk = blk_new_open(file, NULL, bs_opts, bdrv_flags, errp);
+ blk = blk_new_open(file, NULL, bs_opts, bdrv_flags & ~BDRV_O_RDWR, errp);
if (!blk) {
goto err_no_bs_opts;
}
so to make a RW node we should not pass both option and flag ??
Most probably that's because in blk_new_open we do
if (flags & BDRV_O_RDWR) {
perm |= BLK_PERM_WRITE;
}
Probably this should be refactored somehow, as it's strange that we tread read-only option and O_RDWR flags in a different ways.. Still not here. I'll resend so that this patch modifies only BlockBackendRootState, not the logic of blk_new_open() call.
--
Best regards,
Vladimir
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-05-27 15:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-26 21:15 [PATCH 0/3] block: drop BlockDriverState::read_only Vladimir Sementsov-Ogievskiy
2021-05-26 21:15 ` [PATCH 1/3] block: consistently use bdrv_is_read_only() Vladimir Sementsov-Ogievskiy
2021-05-27 14:48 ` Kevin Wolf
2021-05-26 21:15 ` [PATCH 2/3] block: drop BlockDriverState::read_only Vladimir Sementsov-Ogievskiy
2021-05-26 21:15 ` [PATCH 3/3] block: drop BlockBackendRootState::read_only Vladimir Sementsov-Ogievskiy
2021-05-27 14:46 ` Kevin Wolf
2021-05-27 15:31 ` Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.