'discard' is one of the basic BlockdevOptions available for all
drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
the user cannot change it and doesn't get an error explaining that it
can't be changed.

This should update the discard setting, but does nothing:

   (qemu) qemu-io virtio0 "reopen -o discard=on"

Since there's no reason why we shouldn't allow changing it and the
implementation is simple let's just do it.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block.c b/block.c
index 804d557608..21f1eb9cd1 100644
--- a/block.c
+++ b/block.c
@@ -3178,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
 
     update_flags_from_options(&reopen_state->flags, opts);
 
+    value = qemu_opt_get(opts, "discard");
+    if (value != NULL) {
+        if (bdrv_parse_discard_flags(value, &reopen_state->flags) != 0) {
+            error_setg(errp, "Invalid discard option");
+            ret = -EINVAL;
+            goto error;
+        }
+    }
+
     /* node-name and driver must be unchanged. Put them back into the QDict, so
      * that they are checked at the end of this function. */
     value = qemu_opt_get(opts, "node-name");
-- 
2.11.0

On 2018-08-26 16:09, Alberto Garcia wrote:
> 'discard' is one of the basic BlockdevOptions available for all
> drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
> the user cannot change it and doesn't get an error explaining that it
> can't be changed.
> 
> This should update the discard setting, but does nothing:
> 
>    (qemu) qemu-io virtio0 "reopen -o discard=on"
> 
> Since there's no reason why we shouldn't allow changing it and the
> implementation is simple let's just do it.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 804d557608..21f1eb9cd1 100644
> --- a/block.c
> +++ b/block.c
> @@ -3178,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>  
>      update_flags_from_options(&reopen_state->flags, opts);
>  
> +    value = qemu_opt_get(opts, "discard");
> +    if (value != NULL) {
> +        if (bdrv_parse_discard_flags(value, &reopen_state->flags) != 0) {
> +            error_setg(errp, "Invalid discard option");
> +            ret = -EINVAL;
> +            goto error;
> +        }
> +    }
> +
>      /* node-name and driver must be unchanged. Put them back into the QDict, so
>       * that they are checked at the end of this function. */
>      value = qemu_opt_get(opts, "node-name");

Hm.  Why not just use the same "trick" here as in your last patch, i.e.
use qemu_opt_get_del() above and then qemu_opts_to_qdict() to return all
unhandled options to the QDict?

(We'd have to use *_del() in update_flags_from_options() as well, but
that would be OK for all callers, I believe.)

Max

On Wed 29 Aug 2018 01:39:10 PM CEST, Max Reitz wrote:
> On 2018-08-26 16:09, Alberto Garcia wrote:
>> 'discard' is one of the basic BlockdevOptions available for all
>> drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
>> the user cannot change it and doesn't get an error explaining that it
>> can't be changed.
>> 
>> This should update the discard setting, but does nothing:
>> 
>>    (qemu) qemu-io virtio0 "reopen -o discard=on"
>> 
>> Since there's no reason why we shouldn't allow changing it and the
>> implementation is simple let's just do it.
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/block.c b/block.c
>> index 804d557608..21f1eb9cd1 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3178,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>>  
>>      update_flags_from_options(&reopen_state->flags, opts);
>>  
>> +    value = qemu_opt_get(opts, "discard");
>> +    if (value != NULL) {
>> +        if (bdrv_parse_discard_flags(value, &reopen_state->flags) != 0) {
>> +            error_setg(errp, "Invalid discard option");
>> +            ret = -EINVAL;
>> +            goto error;
>> +        }
>> +    }
>> +
>>      /* node-name and driver must be unchanged. Put them back into the QDict, so
>>       * that they are checked at the end of this function. */
>>      value = qemu_opt_get(opts, "node-name");
>
> Hm.  Why not just use the same "trick" here as in your last patch,
> i.e.  use qemu_opt_get_del() above and then qemu_opts_to_qdict() to
> return all unhandled options to the QDict?

Sure, I could do that and therefore forbid changing all of these
options. But as I said in the commit message, there's no reason why we
should not allow changing them.

Berto

On Wed 29 Aug 2018 01:39:10 PM CEST, Max Reitz wrote:
> On 2018-08-26 16:09, Alberto Garcia wrote:
>> 'discard' is one of the basic BlockdevOptions available for all
>> drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
>> the user cannot change it and doesn't get an error explaining that it
>> can't be changed.
>> 
>> This should update the discard setting, but does nothing:
>> 
>>    (qemu) qemu-io virtio0 "reopen -o discard=on"
>> 
>> Since there's no reason why we shouldn't allow changing it and the
>> implementation is simple let's just do it.
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/block.c b/block.c
>> index 804d557608..21f1eb9cd1 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3178,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>>  
>>      update_flags_from_options(&reopen_state->flags, opts);
>>  
>> +    value = qemu_opt_get(opts, "discard");
>> +    if (value != NULL) {
>> +        if (bdrv_parse_discard_flags(value, &reopen_state->flags) != 0) {
>> +            error_setg(errp, "Invalid discard option");
>> +            ret = -EINVAL;
>> +            goto error;
>> +        }
>> +    }
>> +
>>      /* node-name and driver must be unchanged. Put them back into the QDict, so
>>       * that they are checked at the end of this function. */
>>      value = qemu_opt_get(opts, "node-name");
>
> Hm.  Why not just use the same "trick" here as in your last patch,
> i.e.  use qemu_opt_get_del() above and then qemu_opts_to_qdict() to
> return all unhandled options to the QDict?

Hm, I think I misunderstood you earlier. You propose that I use
qemu_opts_to_qdict() to catch all other bdrv_runtime_opts options not
processed in this function.

Yes, that sounds like a good idea.

Berto

On 2018-08-26 16:09, Alberto Garcia wrote:
> 'discard' is one of the basic BlockdevOptions available for all
> drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
> the user cannot change it and doesn't get an error explaining that it
> can't be changed.
> 
> This should update the discard setting, but does nothing:
> 
>    (qemu) qemu-io virtio0 "reopen -o discard=on"
> 
> Since there's no reason why we shouldn't allow changing it and the
> implementation is simple let's just do it.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 804d557608..21f1eb9cd1 100644
> --- a/block.c
> +++ b/block.c
> @@ -3178,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>  
>      update_flags_from_options(&reopen_state->flags, opts);
>  
> +    value = qemu_opt_get(opts, "discard");
> +    if (value != NULL) {
> +        if (bdrv_parse_discard_flags(value, &reopen_state->flags) != 0) {
> +            error_setg(errp, "Invalid discard option");
> +            ret = -EINVAL;
> +            goto error;
> +        }
> +    }
> +

Let me add a:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Although I think it might need a _del.