| Message ID | 599f98e81407a82d8b05764eeac7dbcf5f52142d.1535291602.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc reopen-related patches | expand |
On 2018-08-26 16:09, Alberto Garcia wrote: > The file-posix code is used for the "file", "host_device" and > "host_cdrom" drivers, and it allows reopening images. However the only > option that is actually processed is "x-check-cache-dropped", and > changes in all other options (e.g. "filename") are silently ignored: > > (qemu) qemu-io virtio0 "reopen -o file.filename=no-such-file" > > While we could allow changing some of the other options, let's keep > things as they are for now but return an error if the user tries to > change any of them. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/file-posix.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) Looks OK, but the same question from the last patch arises. If unspecified options mean using the default, shouldn't the user have to re-specify all mandatory options, so at least @filename in this case? Max
On Wed 29 Aug 2018 01:33:13 PM CEST, Max Reitz wrote: > On 2018-08-26 16:09, Alberto Garcia wrote: >> The file-posix code is used for the "file", "host_device" and >> "host_cdrom" drivers, and it allows reopening images. However the only >> option that is actually processed is "x-check-cache-dropped", and >> changes in all other options (e.g. "filename") are silently ignored: >> >> (qemu) qemu-io virtio0 "reopen -o file.filename=no-such-file" >> >> While we could allow changing some of the other options, let's keep >> things as they are for now but return an error if the user tries to >> change any of them. >> >> Signed-off-by: Alberto Garcia <berto@igalia.com> >> --- >> block/file-posix.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) > > Looks OK, but the same question from the last patch arises. If > unspecified options mean using the default, shouldn't the user have to > re-specify all mandatory options, so at least @filename in this case? This is the old (existing) reopen command, which takes the current options and applies the user-specified changes on top. In this one we can leave options out because the old values are kept. It's in the new QMP blockdev-reopen command (not in this series yet) that you need to specify all options (among other things because it takes a BlockdevOptions struct, so there's no choice not to specify the mandatory ones). Berto
On 2018-08-29 14:33, Alberto Garcia wrote: > On Wed 29 Aug 2018 01:33:13 PM CEST, Max Reitz wrote: >> On 2018-08-26 16:09, Alberto Garcia wrote: >>> The file-posix code is used for the "file", "host_device" and >>> "host_cdrom" drivers, and it allows reopening images. However the only >>> option that is actually processed is "x-check-cache-dropped", and >>> changes in all other options (e.g. "filename") are silently ignored: >>> >>> (qemu) qemu-io virtio0 "reopen -o file.filename=no-such-file" >>> >>> While we could allow changing some of the other options, let's keep >>> things as they are for now but return an error if the user tries to >>> change any of them. >>> >>> Signed-off-by: Alberto Garcia <berto@igalia.com> >>> --- >>> block/file-posix.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> Looks OK, but the same question from the last patch arises. If >> unspecified options mean using the default, shouldn't the user have to >> re-specify all mandatory options, so at least @filename in this case? > > This is the old (existing) reopen command, which takes the current > options and applies the user-specified changes on top. In this one we > can leave options out because the old values are kept. > > It's in the new QMP blockdev-reopen command (not in this series yet) > that you need to specify all options (among other things because it > takes a BlockdevOptions struct, so there's no choice not to specify the > mandatory ones). Hmm... OK. I thought we'd just change the current reopen command. But that's OK, then. Max
diff --git a/block/file-posix.c b/block/file-posix.c index ddac0cc3e6..d4ec2cb3dd 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -850,8 +850,13 @@ static int raw_reopen_prepare(BDRVReopenState *state, goto out; } - rs->check_cache_dropped = qemu_opt_get_bool(opts, "x-check-cache-dropped", - false); + rs->check_cache_dropped = + qemu_opt_get_bool_del(opts, "x-check-cache-dropped", false); + + /* This driver's reopen function doesn't currently allow changing + * other options, so let's put them back in the original QDict and + * bdrv_reopen_prepare() will detect changes and complain. */ + qemu_opts_to_qdict(opts, state->options); if (s->type == FTYPE_CD) { rs->open_flags |= O_NONBLOCK;
The file-posix code is used for the "file", "host_device" and "host_cdrom" drivers, and it allows reopening images. However the only option that is actually processed is "x-check-cache-dropped", and changes in all other options (e.g. "filename") are silently ignored: (qemu) qemu-io virtio0 "reopen -o file.filename=no-such-file" While we could allow changing some of the other options, let's keep things as they are for now but return an error if the user tries to change any of them. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/file-posix.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)