| Message ID | 61bbfa3721938326edb03db58a6a246b384f3260.1547739122.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add a 'x-blockdev-reopen' QMP command | expand |
Am 17.01.2019 um 16:34 hat Alberto Garcia geschrieben: > This patch adds two new fields to BlockDriver: > > - runtime_opts: list of runtime options for a particular block > driver. We'll use this list later to detect what options are > missing when we try to reopen a block device. > > - mutable_opts: names of the runtime options that can be reset to > their default value after a block device has been added. This way > an option can not be reset by omitting it during reopen unless we > explicitly allow it. > > This also sets runtime_opts (and mutable_opts where appropriate) in > all drivers that allow reopening. qcow2 most certainly does support reopening, and is probably one of the few drivers that actually allow changing things at runtime. Still, it's missing from this patch. > Most of those drivers don't actually > support changing any of their options. If the user specifies a new > value for an option that can't be changed then we already detect that > and forbid it (in bdrv_reopen_prepare()). But if the user omits an > option in order to try to reset it to its default value we need to > detect that, so we'll use these two new fields for that. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > diff --git a/include/block/block_int.h b/include/block/block_int.h > index fd0e88d17a..e680dda86b 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -345,6 +345,13 @@ struct BlockDriver { > > /* List of options for creating images, terminated by name == NULL */ > QemuOptsList *create_opts; > + /* Runtime options for a block device, terminated by name == NULL */ > + QemuOptsList *runtime_opts; I'm not sure if using a QemuOptsList here is a good idea. Currently, we use QemuOptsLists for most options, but there are some drivers that use it only for part of their options, or not at all, using direct QDict accesses or QAPI objects for the rest. Especially the part with the QAPI objects is something that I'd hope to see more in the future, and tying things to QemuOpts might make this conversion harder. > + /* > + * Names of the runtime options that can be reset by omitting > + * their value on reopen, NULL-terminated. > + */ > + const char *const *mutable_opts; > > /* > * Returns 0 for completed check, -errno for internal errors. > diff --git a/block/rbd.c b/block/rbd.c > index 8a1a9f4b6e..6de6112ce8 100644 > --- a/block/rbd.c > +++ b/block/rbd.c > @@ -1231,6 +1231,7 @@ static QemuOptsList qemu_rbd_create_opts = { > static BlockDriver bdrv_rbd = { > .format_name = "rbd", > .instance_size = sizeof(BDRVRBDState), > + .runtime_opts = &runtime_opts, > .bdrv_parse_filename = qemu_rbd_parse_filename, > .bdrv_refresh_limits = qemu_rbd_refresh_limits, > .bdrv_file_open = qemu_rbd_open, rbd is one of these drivers. In fact, its runtime_opts seems to be completely unused before this patch. It has a comemnt 'server.* extracted manually, see qemu_rbd_mon_host()', but if you compare it to the schema, more options that have been added since are not covered in runtime_opts. > diff --git a/block/sheepdog.c b/block/sheepdog.c > index 90ab43baa4..6dd66d0b99 100644 > --- a/block/sheepdog.c > +++ b/block/sheepdog.c > @@ -3288,6 +3288,7 @@ static BlockDriver bdrv_sheepdog_tcp = { > .bdrv_attach_aio_context = sd_attach_aio_context, > > .create_opts = &sd_create_opts, > + .runtime_opts = &runtime_opts, > }; Sheepdog is another driver where runtime_opts is incomplete (the 'server' struct is missing). Kevin
On Tue 12 Feb 2019 07:02:31 PM CET, Kevin Wolf wrote: >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index fd0e88d17a..e680dda86b 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -345,6 +345,13 @@ struct BlockDriver { >> >> /* List of options for creating images, terminated by name == NULL */ >> QemuOptsList *create_opts; >> + /* Runtime options for a block device, terminated by name == NULL */ >> + QemuOptsList *runtime_opts; > > I'm not sure if using a QemuOptsList here is a good idea. Currently, > we use QemuOptsLists for most options, but there are some drivers that > use it only for part of their options, or not at all, using direct > QDict accesses or QAPI objects for the rest. My intention was to avoid having two separate lists with the runtime options of a driver. For this feature we really need that list to contain all options, otherwise there's no way to know whether a missing option is really missing or if it doesn't exist in the first place. Berto
Am 01.03.2019 um 13:12 hat Alberto Garcia geschrieben: > On Tue 12 Feb 2019 07:02:31 PM CET, Kevin Wolf wrote: > >> diff --git a/include/block/block_int.h b/include/block/block_int.h > >> index fd0e88d17a..e680dda86b 100644 > >> --- a/include/block/block_int.h > >> +++ b/include/block/block_int.h > >> @@ -345,6 +345,13 @@ struct BlockDriver { > >> > >> /* List of options for creating images, terminated by name == NULL */ > >> QemuOptsList *create_opts; > >> + /* Runtime options for a block device, terminated by name == NULL */ > >> + QemuOptsList *runtime_opts; > > > > I'm not sure if using a QemuOptsList here is a good idea. Currently, > > we use QemuOptsLists for most options, but there are some drivers that > > use it only for part of their options, or not at all, using direct > > QDict accesses or QAPI objects for the rest. > > My intention was to avoid having two separate lists with the runtime > options of a driver. For this feature we really need that list to > contain all options, otherwise there's no way to know whether a missing > option is really missing or if it doesn't exist in the first place. Yes, I understand your intention and the requirement. My point is just that the existing QemuOptsLists are often _not_ complete, so they can't fulfill the requirement. Kevin
On Fri 01 Mar 2019 01:36:08 PM CET, Kevin Wolf wrote: > Am 01.03.2019 um 13:12 hat Alberto Garcia geschrieben: >> On Tue 12 Feb 2019 07:02:31 PM CET, Kevin Wolf wrote: >> >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> >> index fd0e88d17a..e680dda86b 100644 >> >> --- a/include/block/block_int.h >> >> +++ b/include/block/block_int.h >> >> @@ -345,6 +345,13 @@ struct BlockDriver { >> >> >> >> /* List of options for creating images, terminated by name == NULL */ >> >> QemuOptsList *create_opts; >> >> + /* Runtime options for a block device, terminated by name == NULL */ >> >> + QemuOptsList *runtime_opts; >> > >> > I'm not sure if using a QemuOptsList here is a good idea. Currently, >> > we use QemuOptsLists for most options, but there are some drivers that >> > use it only for part of their options, or not at all, using direct >> > QDict accesses or QAPI objects for the rest. >> >> My intention was to avoid having two separate lists with the runtime >> options of a driver. For this feature we really need that list to >> contain all options, otherwise there's no way to know whether a missing >> option is really missing or if it doesn't exist in the first place. > > Yes, I understand your intention and the requirement. My point is just > that the existing QemuOptsLists are often _not_ complete, so they > can't fulfill the requirement. Perhaps a new data structure with a list of runtime options and whether they can be resetted to their default value or not. Berto
Am 01.03.2019 um 13:42 hat Alberto Garcia geschrieben: > On Fri 01 Mar 2019 01:36:08 PM CET, Kevin Wolf wrote: > > Am 01.03.2019 um 13:12 hat Alberto Garcia geschrieben: > >> On Tue 12 Feb 2019 07:02:31 PM CET, Kevin Wolf wrote: > >> >> diff --git a/include/block/block_int.h b/include/block/block_int.h > >> >> index fd0e88d17a..e680dda86b 100644 > >> >> --- a/include/block/block_int.h > >> >> +++ b/include/block/block_int.h > >> >> @@ -345,6 +345,13 @@ struct BlockDriver { > >> >> > >> >> /* List of options for creating images, terminated by name == NULL */ > >> >> QemuOptsList *create_opts; > >> >> + /* Runtime options for a block device, terminated by name == NULL */ > >> >> + QemuOptsList *runtime_opts; > >> > > >> > I'm not sure if using a QemuOptsList here is a good idea. Currently, > >> > we use QemuOptsLists for most options, but there are some drivers that > >> > use it only for part of their options, or not at all, using direct > >> > QDict accesses or QAPI objects for the rest. > >> > >> My intention was to avoid having two separate lists with the runtime > >> options of a driver. For this feature we really need that list to > >> contain all options, otherwise there's no way to know whether a missing > >> option is really missing or if it doesn't exist in the first place. > > > > Yes, I understand your intention and the requirement. My point is just > > that the existing QemuOptsLists are often _not_ complete, so they > > can't fulfill the requirement. > > Perhaps a new data structure with a list of runtime options and whether > they can be resetted to their default value or not. Basically just an array that contains names and bools, right? I think that would work. Kevin
On Fri 01 Mar 2019 01:56:42 PM CET, Kevin Wolf wrote: >> >> >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> >> >> index fd0e88d17a..e680dda86b 100644 >> >> >> --- a/include/block/block_int.h >> >> >> +++ b/include/block/block_int.h >> >> >> @@ -345,6 +345,13 @@ struct BlockDriver { >> >> >> >> >> >> /* List of options for creating images, terminated by name == NULL */ >> >> >> QemuOptsList *create_opts; >> >> >> + /* Runtime options for a block device, terminated by name == NULL */ >> >> >> + QemuOptsList *runtime_opts; >> >> > >> >> > I'm not sure if using a QemuOptsList here is a good idea. Currently, >> >> > we use QemuOptsLists for most options, but there are some drivers that >> >> > use it only for part of their options, or not at all, using direct >> >> > QDict accesses or QAPI objects for the rest. >> >> >> >> My intention was to avoid having two separate lists with the runtime >> >> options of a driver. For this feature we really need that list to >> >> contain all options, otherwise there's no way to know whether a missing >> >> option is really missing or if it doesn't exist in the first place. >> > >> > Yes, I understand your intention and the requirement. My point is just >> > that the existing QemuOptsLists are often _not_ complete, so they >> > can't fulfill the requirement. >> >> Perhaps a new data structure with a list of runtime options and whether >> they can be resetted to their default value or not. > > Basically just an array that contains names and bools, right? I think > that would work. Yes exactly. My concern is that the array has to be updated by hand every time a new option is added to the driver, so they could easily be out of sync. Berto
Am 01.03.2019 um 14:05 hat Alberto Garcia geschrieben: > On Fri 01 Mar 2019 01:56:42 PM CET, Kevin Wolf wrote: > >> >> >> diff --git a/include/block/block_int.h b/include/block/block_int.h > >> >> >> index fd0e88d17a..e680dda86b 100644 > >> >> >> --- a/include/block/block_int.h > >> >> >> +++ b/include/block/block_int.h > >> >> >> @@ -345,6 +345,13 @@ struct BlockDriver { > >> >> >> > >> >> >> /* List of options for creating images, terminated by name == NULL */ > >> >> >> QemuOptsList *create_opts; > >> >> >> + /* Runtime options for a block device, terminated by name == NULL */ > >> >> >> + QemuOptsList *runtime_opts; > >> >> > > >> >> > I'm not sure if using a QemuOptsList here is a good idea. Currently, > >> >> > we use QemuOptsLists for most options, but there are some drivers that > >> >> > use it only for part of their options, or not at all, using direct > >> >> > QDict accesses or QAPI objects for the rest. > >> >> > >> >> My intention was to avoid having two separate lists with the runtime > >> >> options of a driver. For this feature we really need that list to > >> >> contain all options, otherwise there's no way to know whether a missing > >> >> option is really missing or if it doesn't exist in the first place. > >> > > >> > Yes, I understand your intention and the requirement. My point is just > >> > that the existing QemuOptsLists are often _not_ complete, so they > >> > can't fulfill the requirement. > >> > >> Perhaps a new data structure with a list of runtime options and whether > >> they can be resetted to their default value or not. > > > > Basically just an array that contains names and bools, right? I think > > that would work. > > Yes exactly. My concern is that the array has to be updated by hand > every time a new option is added to the driver, so they could easily be > out of sync. Hm, actually, do you even need the runtime_opts list? You use it in bdrv_reset_options_allowed() to iterate through each option and then check whether it was present in the old set of options, but not in the new set of options. In order to achieve this, you can just iterate the old options dict because you don't do anything with options that are present in the QemuOptsList, but not the old options dict. Kevin
On Fri 01 Mar 2019 03:18:09 PM CET, Kevin Wolf wrote: > Hm, actually, do you even need the runtime_opts list? > > You use it in bdrv_reset_options_allowed() to iterate through each > option and then check whether it was present in the old set of > options, but not in the new set of options. > > In order to achieve this, you can just iterate the old options dict > because you don't do anything with options that are present in the > QemuOptsList, but not the old options dict. Hmm, that could work, I'll give it a try. Berto
diff --git a/block/blkdebug.c b/block/blkdebug.c index 0759452925..bba7645e09 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -893,6 +893,7 @@ static BlockDriver bdrv_blkdebug = { .protocol_name = "blkdebug", .instance_size = sizeof(BDRVBlkdebugState), .is_filter = true, + .runtime_opts = &runtime_opts, .bdrv_parse_filename = blkdebug_parse_filename, .bdrv_file_open = blkdebug_open, diff --git a/block/crypto.c b/block/crypto.c index f0a5f6b987..ba4da53191 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -635,6 +635,7 @@ BlockDriver bdrv_crypto_luks = { .bdrv_co_create_opts = block_crypto_co_create_opts_luks, .bdrv_co_truncate = block_crypto_co_truncate, .create_opts = &block_crypto_create_opts_luks, + .runtime_opts = &block_crypto_runtime_opts_luks, .bdrv_reopen_prepare = block_crypto_reopen_prepare, .bdrv_refresh_limits = block_crypto_refresh_limits, diff --git a/block/file-posix.c b/block/file-posix.c index 8aee7a3fb8..d08233ece3 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -443,6 +443,8 @@ static QemuOptsList raw_runtime_opts = { }, }; +static const char *const mutable_opts[] = { "x-check-cache-dropped", NULL }; + static int raw_open_common(BlockDriverState *bs, QDict *options, int bdrv_flags, int open_flags, bool device, Error **errp) @@ -2743,6 +2745,8 @@ BlockDriver bdrv_file = { .format_name = "file", .protocol_name = "file", .instance_size = sizeof(BDRVRawState), + .runtime_opts = &raw_runtime_opts, + .mutable_opts = mutable_opts, .bdrv_needs_filename = true, .bdrv_probe = NULL, /* no probe for protocols */ .bdrv_parse_filename = raw_parse_filename, @@ -3220,6 +3224,8 @@ static BlockDriver bdrv_host_device = { .format_name = "host_device", .protocol_name = "host_device", .instance_size = sizeof(BDRVRawState), + .runtime_opts = &raw_runtime_opts, + .mutable_opts = mutable_opts, .bdrv_needs_filename = true, .bdrv_probe_device = hdev_probe_device, .bdrv_parse_filename = hdev_parse_filename, @@ -3346,6 +3352,8 @@ static BlockDriver bdrv_host_cdrom = { .format_name = "host_cdrom", .protocol_name = "host_cdrom", .instance_size = sizeof(BDRVRawState), + .runtime_opts = &raw_runtime_opts, + .mutable_opts = mutable_opts, .bdrv_needs_filename = true, .bdrv_probe_device = cdrom_probe_device, .bdrv_parse_filename = cdrom_parse_filename, @@ -3479,6 +3487,8 @@ static BlockDriver bdrv_host_cdrom = { .format_name = "host_cdrom", .protocol_name = "host_cdrom", .instance_size = sizeof(BDRVRawState), + .runtime_opts = &raw_runtime_opts, + .mutable_opts = mutable_opts, .bdrv_needs_filename = true, .bdrv_probe_device = cdrom_probe_device, .bdrv_parse_filename = cdrom_parse_filename, diff --git a/block/iscsi.c b/block/iscsi.c index a7e8c1ffaf..70f07b2105 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -2456,6 +2456,7 @@ static BlockDriver bdrv_iscsi = { .bdrv_close = iscsi_close, .bdrv_co_create_opts = iscsi_co_create_opts, .create_opts = &iscsi_create_opts, + .runtime_opts = &runtime_opts, .bdrv_reopen_prepare = iscsi_reopen_prepare, .bdrv_reopen_commit = iscsi_reopen_commit, .bdrv_co_invalidate_cache = iscsi_co_invalidate_cache, @@ -2493,6 +2494,7 @@ static BlockDriver bdrv_iser = { .bdrv_close = iscsi_close, .bdrv_co_create_opts = iscsi_co_create_opts, .create_opts = &iscsi_create_opts, + .runtime_opts = &runtime_opts, .bdrv_reopen_prepare = iscsi_reopen_prepare, .bdrv_reopen_commit = iscsi_reopen_commit, .bdrv_co_invalidate_cache = iscsi_co_invalidate_cache, diff --git a/block/null.c b/block/null.c index d442d3e901..5df4b46723 100644 --- a/block/null.c +++ b/block/null.c @@ -256,6 +256,7 @@ static BlockDriver bdrv_null_co = { .format_name = "null-co", .protocol_name = "null-co", .instance_size = sizeof(BDRVNullState), + .runtime_opts = &runtime_opts, .bdrv_file_open = null_file_open, .bdrv_parse_filename = null_co_parse_filename, @@ -275,6 +276,7 @@ static BlockDriver bdrv_null_aio = { .format_name = "null-aio", .protocol_name = "null-aio", .instance_size = sizeof(BDRVNullState), + .runtime_opts = &runtime_opts, .bdrv_file_open = null_file_open, .bdrv_parse_filename = null_aio_parse_filename, diff --git a/block/nvme.c b/block/nvme.c index 982097b5b1..7c71fba057 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -1139,6 +1139,7 @@ static BlockDriver bdrv_nvme = { .format_name = "nvme", .protocol_name = "nvme", .instance_size = sizeof(BDRVNVMeState), + .runtime_opts = &runtime_opts, .bdrv_parse_filename = nvme_parse_filename, .bdrv_file_open = nvme_file_open, diff --git a/block/qcow.c b/block/qcow.c index 0a235bf393..bffece818a 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -1186,6 +1186,7 @@ static QemuOptsList qcow_create_opts = { static BlockDriver bdrv_qcow = { .format_name = "qcow", .instance_size = sizeof(BDRVQcowState), + .runtime_opts = &qcow_runtime_opts, .bdrv_probe = qcow_probe, .bdrv_open = qcow_open, .bdrv_close = qcow_close, diff --git a/block/rbd.c b/block/rbd.c index 8a1a9f4b6e..6de6112ce8 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -1231,6 +1231,7 @@ static QemuOptsList qemu_rbd_create_opts = { static BlockDriver bdrv_rbd = { .format_name = "rbd", .instance_size = sizeof(BDRVRBDState), + .runtime_opts = &runtime_opts, .bdrv_parse_filename = qemu_rbd_parse_filename, .bdrv_refresh_limits = qemu_rbd_refresh_limits, .bdrv_file_open = qemu_rbd_open, diff --git a/block/sheepdog.c b/block/sheepdog.c index 90ab43baa4..6dd66d0b99 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -3288,6 +3288,7 @@ static BlockDriver bdrv_sheepdog_tcp = { .bdrv_attach_aio_context = sd_attach_aio_context, .create_opts = &sd_create_opts, + .runtime_opts = &runtime_opts, }; static BlockDriver bdrv_sheepdog_unix = { @@ -3325,6 +3326,7 @@ static BlockDriver bdrv_sheepdog_unix = { .bdrv_attach_aio_context = sd_attach_aio_context, .create_opts = &sd_create_opts, + .runtime_opts = &runtime_opts, }; static void bdrv_sheepdog_init(void) diff --git a/block/vpc.c b/block/vpc.c index d886465b7e..640c956d0d 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -1235,6 +1235,7 @@ static BlockDriver bdrv_vpc = { .bdrv_get_info = vpc_get_info, .create_opts = &vpc_create_opts, + .runtime_opts = &vpc_runtime_opts, .bdrv_has_zero_init = vpc_has_zero_init, }; diff --git a/include/block/block_int.h b/include/block/block_int.h index fd0e88d17a..e680dda86b 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -345,6 +345,13 @@ struct BlockDriver { /* List of options for creating images, terminated by name == NULL */ QemuOptsList *create_opts; + /* Runtime options for a block device, terminated by name == NULL */ + QemuOptsList *runtime_opts; + /* + * Names of the runtime options that can be reset by omitting + * their value on reopen, NULL-terminated. + */ + const char *const *mutable_opts; /* * Returns 0 for completed check, -errno for internal errors.
This patch adds two new fields to BlockDriver: - runtime_opts: list of runtime options for a particular block driver. We'll use this list later to detect what options are missing when we try to reopen a block device. - mutable_opts: names of the runtime options that can be reset to their default value after a block device has been added. This way an option can not be reset by omitting it during reopen unless we explicitly allow it. This also sets runtime_opts (and mutable_opts where appropriate) in all drivers that allow reopening. Most of those drivers don't actually support changing any of their options. If the user specifies a new value for an option that can't be changed then we already detect that and forbid it (in bdrv_reopen_prepare()). But if the user omits an option in order to try to reset it to its default value we need to detect that, so we'll use these two new fields for that. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/blkdebug.c | 1 + block/crypto.c | 1 + block/file-posix.c | 10 ++++++++++ block/iscsi.c | 2 ++ block/null.c | 2 ++ block/nvme.c | 1 + block/qcow.c | 1 + block/rbd.c | 1 + block/sheepdog.c | 2 ++ block/vpc.c | 1 + include/block/block_int.h | 7 +++++++ 11 files changed, 29 insertions(+)