| Message ID | 20181001171901.11004-9-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/23] file-posix: Include filename in locking error message | expand |
On 1 October 2018 at 18:18, Kevin Wolf <kwolf@redhat.com> wrote: > From: Alberto Garcia <berto@igalia.com> > > 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> > Reviewed-by: Max Reitz <mreitz@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/file-posix.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index bc5e54560a..2da3a76355 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -849,8 +849,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); Hi. Coverity is suspicious about this call to qemu_opts_to_qdict() because it returns a value which this callsite is ignoring but almost all others don't ignore (CID 1395991). Is it correct? (It also doesn't like the call in block.c: CID 1395989.) thanks -- PMM
Am 05.10.2018 um 14:55 hat Peter Maydell geschrieben: > On 1 October 2018 at 18:18, Kevin Wolf <kwolf@redhat.com> wrote: > > From: Alberto Garcia <berto@igalia.com> > > > > 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> > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block/file-posix.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > index bc5e54560a..2da3a76355 100644 > > --- a/block/file-posix.c > > +++ b/block/file-posix.c > > @@ -849,8 +849,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); > > Hi. Coverity is suspicious about this call to qemu_opts_to_qdict() > because it returns a value which this callsite is ignoring but > almost all others don't ignore (CID 1395991). Is it correct? > > (It also doesn't like the call in block.c: CID 1395989.) I think this is a false positive. qemu_opts_to_qdict() returns the dict it was given, except if NULL was given, then it returns a newly allocated one. Kevin
On Fri 05 Oct 2018 02:55:13 PM CEST, Peter Maydell wrote: >> + 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); > > Hi. Coverity is suspicious about this call to qemu_opts_to_qdict() > because it returns a value which this callsite is ignoring but almost > all others don't ignore (CID 1395991). Is it correct? It's a false positive because qemu_opts_to_qdict() returns the same value that is passed (unless that value is NULL). We can fix the warning by doing state->options = qemu_opts_to_qdict(opts, state->options); or by modifying qemu_opts_to_qdict() to get a QDict ** and return void. Berto
On Fri 05 Oct 2018 03:40:09 PM CEST, Alberto Garcia wrote: > On Fri 05 Oct 2018 02:55:13 PM CEST, Peter Maydell wrote: >>> + 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); >> >> Hi. Coverity is suspicious about this call to qemu_opts_to_qdict() >> because it returns a value which this callsite is ignoring but almost >> all others don't ignore (CID 1395991). Is it correct? > > It's a false positive because qemu_opts_to_qdict() returns the same > value that is passed (unless that value is NULL). and I forgot to add: in this case it's guaranteed to be non-NULL, since it's initialized in bdrv_reopen_queue_child(). Berto
On 5 October 2018 at 14:40, Alberto Garcia <berto@igalia.com> wrote: > On Fri 05 Oct 2018 02:55:13 PM CEST, Peter Maydell wrote: >>> + 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); >> >> Hi. Coverity is suspicious about this call to qemu_opts_to_qdict() >> because it returns a value which this callsite is ignoring but almost >> all others don't ignore (CID 1395991). Is it correct? > > It's a false positive because qemu_opts_to_qdict() returns the same > value that is passed (unless that value is NULL). > > We can fix the warning by doing > > state->options = qemu_opts_to_qdict(opts, state->options); > > or by modifying qemu_opts_to_qdict() to get a QDict ** and return void. I think just marking the issues as false-positives in the coverity UI is sufficient here. thanks -- PMM
diff --git a/block/file-posix.c b/block/file-posix.c index bc5e54560a..2da3a76355 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -849,8 +849,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;