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);
 
     if (s->type == FTYPE_CD) {
         rs->open_flags |= O_NONBLOCK;
-- 
2.13.6

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