bdrv_set_read_only() is used by some block drivers to override the
read-only option given by the user. This is not how read-only images
generally work in QEMU: Instead of second guessing what the user really
meant (which currently includes making an image read-only even if the
user didn't only use the default, but explicitly said read-only=off), we
should error out if we can't provide what the user requested.

This adds deprecation warnings to all callers of bdrv_set_read_only() so
that the behaviour can be corrected after the usual deprecation period.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c       |  5 +++++
 block/bochs.c | 13 ++++++++++---
 block/cloop.c | 13 ++++++++++---
 block/dmg.c   | 12 +++++++++---
 block/rbd.c   | 14 ++++++++++----
 block/vvfat.c |  6 +++++-
 6 files changed, 49 insertions(+), 14 deletions(-)

diff --git a/block.c b/block.c
index f6415547fe..0ed0c27140 100644
--- a/block.c
+++ b/block.c
@@ -261,6 +261,11 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
     return 0;
 }
 
+/* TODO Remove (deprecated since 2.11)
+ * Block drivers are not supposed to automatically change bs->read_only.
+ * Instead, they should just check whether they can provide what the user
+ * explicitly requested and error out if read-write is requested, but they can
+ * only provide read-only access. */
 int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
 {
     int ret = 0;
diff --git a/block/bochs.c b/block/bochs.c
index a759b6eff0..50c630047b 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -28,6 +28,7 @@
 #include "block/block_int.h"
 #include "qemu/module.h"
 #include "qemu/bswap.h"
+#include "qemu/error-report.h"
 
 /**************************************************************/
 
@@ -110,9 +111,15 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
-    if (ret < 0) {
-        return ret;
+    if (!bdrv_is_read_only(bs)) {
+        error_report("Opening bochs images without an explicit read-only=on "
+                     "option is deprecated. Future versions will refuse to "
+                     "open the image instead of automatically marking the "
+                     "image read-only.");
+        ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
+        if (ret < 0) {
+            return ret;
+        }
     }
 
     ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
diff --git a/block/cloop.c b/block/cloop.c
index d6597fcf78..2be68987bd 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -23,6 +23,7 @@
  */
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "qemu-common.h"
 #include "block/block_int.h"
 #include "qemu/module.h"
@@ -72,9 +73,15 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    ret = bdrv_set_read_only(bs, true, errp);
-    if (ret < 0) {
-        return ret;
+    if (!bdrv_is_read_only(bs)) {
+        error_report("Opening cloop images without an explicit read-only=on "
+                     "option is deprecated. Future versions will refuse to "
+                     "open the image instead of automatically marking the "
+                     "image read-only.");
+        ret = bdrv_set_read_only(bs, true, errp);
+        if (ret < 0) {
+            return ret;
+        }
     }
 
     /* read header */
diff --git a/block/dmg.c b/block/dmg.c
index 6c0711f563..c9b3c519c4 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -419,9 +419,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    ret = bdrv_set_read_only(bs, true, errp);
-    if (ret < 0) {
-        return ret;
+    if (!bdrv_is_read_only(bs)) {
+        error_report("Opening dmg images without an explicit read-only=on "
+                     "option is deprecated. Future versions will refuse to "
+                     "open the image instead of automatically marking the "
+                     "image read-only.");
+        ret = bdrv_set_read_only(bs, true, errp);
+        if (ret < 0) {
+            return ret;
+        }
     }
 
     block_module_load_one("dmg-bz2");
diff --git a/block/rbd.c b/block/rbd.c
index 144f350e1f..a76a5e8755 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -665,10 +665,16 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     /* If we are using an rbd snapshot, we must be r/o, otherwise
      * leave as-is */
     if (s->snap != NULL) {
-        r = bdrv_set_read_only(bs, true, &local_err);
-        if (r < 0) {
-            error_propagate(errp, local_err);
-            goto failed_open;
+        if (!bdrv_is_read_only(bs)) {
+            error_report("Opening rbd snapshots without an explicit "
+                         "read-only=on option is deprecated. Future versions "
+                         "will refuse to open the image instead of "
+                         "automatically marking the image read-only.");
+            r = bdrv_set_read_only(bs, true, &local_err);
+            if (r < 0) {
+                error_propagate(errp, local_err);
+                goto failed_open;
+            }
         }
     }
 
diff --git a/block/vvfat.c b/block/vvfat.c
index a0f2335894..0841cc42fc 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1259,7 +1259,11 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
                        "Unable to set VVFAT to 'rw' when drive is read-only");
             goto fail;
         }
-    } else  {
+    } else  if (!bdrv_is_read_only(bs)) {
+        error_report("Opening non-rw vvfat images without an explicit "
+                     "read-only=on option is deprecated. Future versions "
+                     "will refuse to open the image instead of "
+                     "automatically marking the image read-only.");
         /* read only is the default for safety */
         ret = bdrv_set_read_only(bs, true, &local_err);
         if (ret < 0) {
-- 
2.13.6

On Tue, Nov 07, 2017 at 06:26:38PM +0100, Kevin Wolf wrote:
> bdrv_set_read_only() is used by some block drivers to override the
> read-only option given by the user. This is not how read-only images
> generally work in QEMU: Instead of second guessing what the user really
> meant (which currently includes making an image read-only even if the
> user didn't only use the default, but explicitly said read-only=off), we
> should error out if we can't provide what the user requested.
> 
> This adds deprecation warnings to all callers of bdrv_set_read_only() so
> that the behaviour can be corrected after the usual deprecation period.

All deprecations should be listed in "Deprecated features" appendix
in qemu-doc.texi. This probably fits in the 'system emulator command
line arguments' section, even though its talking about the need for
the user to add something extra, rather than deleting something they
currently use.

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c       |  5 +++++
>  block/bochs.c | 13 ++++++++++---
>  block/cloop.c | 13 ++++++++++---
>  block/dmg.c   | 12 +++++++++---
>  block/rbd.c   | 14 ++++++++++----
>  block/vvfat.c |  6 +++++-
>  6 files changed, 49 insertions(+), 14 deletions(-)
> 
> diff --git a/block.c b/block.c
> index f6415547fe..0ed0c27140 100644
> --- a/block.c
> +++ b/block.c
> @@ -261,6 +261,11 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
>      return 0;
>  }
>  
> +/* TODO Remove (deprecated since 2.11)
> + * Block drivers are not supposed to automatically change bs->read_only.
> + * Instead, they should just check whether they can provide what the user
> + * explicitly requested and error out if read-write is requested, but they can
> + * only provide read-only access. */
>  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>  {
>      int ret = 0;
> diff --git a/block/bochs.c b/block/bochs.c
> index a759b6eff0..50c630047b 100644
> --- a/block/bochs.c
> +++ b/block/bochs.c
> @@ -28,6 +28,7 @@
>  #include "block/block_int.h"
>  #include "qemu/module.h"
>  #include "qemu/bswap.h"
> +#include "qemu/error-report.h"
>  
>  /**************************************************************/
>  
> @@ -110,9 +111,15 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
>          return -EINVAL;
>      }
>  
> -    ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
> -    if (ret < 0) {
> -        return ret;
> +    if (!bdrv_is_read_only(bs)) {
> +        error_report("Opening bochs images without an explicit read-only=on "
> +                     "option is deprecated. Future versions will refuse to "
> +                     "open the image instead of automatically marking the "
> +                     "image read-only.");
> +        ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
> +        if (ret < 0) {
> +            return ret;
> +        }
>      }
>  
>      ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
> diff --git a/block/cloop.c b/block/cloop.c
> index d6597fcf78..2be68987bd 100644
> --- a/block/cloop.c
> +++ b/block/cloop.c
> @@ -23,6 +23,7 @@
>   */
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qemu/error-report.h"
>  #include "qemu-common.h"
>  #include "block/block_int.h"
>  #include "qemu/module.h"
> @@ -72,9 +73,15 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
>          return -EINVAL;
>      }
>  
> -    ret = bdrv_set_read_only(bs, true, errp);
> -    if (ret < 0) {
> -        return ret;
> +    if (!bdrv_is_read_only(bs)) {
> +        error_report("Opening cloop images without an explicit read-only=on "
> +                     "option is deprecated. Future versions will refuse to "
> +                     "open the image instead of automatically marking the "
> +                     "image read-only.");
> +        ret = bdrv_set_read_only(bs, true, errp);
> +        if (ret < 0) {
> +            return ret;
> +        }
>      }
>  
>      /* read header */
> diff --git a/block/dmg.c b/block/dmg.c
> index 6c0711f563..c9b3c519c4 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -419,9 +419,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>          return -EINVAL;
>      }
>  
> -    ret = bdrv_set_read_only(bs, true, errp);
> -    if (ret < 0) {
> -        return ret;
> +    if (!bdrv_is_read_only(bs)) {
> +        error_report("Opening dmg images without an explicit read-only=on "
> +                     "option is deprecated. Future versions will refuse to "
> +                     "open the image instead of automatically marking the "
> +                     "image read-only.");
> +        ret = bdrv_set_read_only(bs, true, errp);
> +        if (ret < 0) {
> +            return ret;
> +        }
>      }
>  
>      block_module_load_one("dmg-bz2");
> diff --git a/block/rbd.c b/block/rbd.c
> index 144f350e1f..a76a5e8755 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -665,10 +665,16 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>      /* If we are using an rbd snapshot, we must be r/o, otherwise
>       * leave as-is */
>      if (s->snap != NULL) {
> -        r = bdrv_set_read_only(bs, true, &local_err);
> -        if (r < 0) {
> -            error_propagate(errp, local_err);
> -            goto failed_open;
> +        if (!bdrv_is_read_only(bs)) {
> +            error_report("Opening rbd snapshots without an explicit "
> +                         "read-only=on option is deprecated. Future versions "
> +                         "will refuse to open the image instead of "
> +                         "automatically marking the image read-only.");
> +            r = bdrv_set_read_only(bs, true, &local_err);
> +            if (r < 0) {
> +                error_propagate(errp, local_err);
> +                goto failed_open;
> +            }
>          }
>      }
>  
> diff --git a/block/vvfat.c b/block/vvfat.c
> index a0f2335894..0841cc42fc 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1259,7 +1259,11 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>                         "Unable to set VVFAT to 'rw' when drive is read-only");
>              goto fail;
>          }
> -    } else  {
> +    } else  if (!bdrv_is_read_only(bs)) {
> +        error_report("Opening non-rw vvfat images without an explicit "
> +                     "read-only=on option is deprecated. Future versions "
> +                     "will refuse to open the image instead of "
> +                     "automatically marking the image read-only.");
>          /* read only is the default for safety */
>          ret = bdrv_set_read_only(bs, true, &local_err);
>          if (ret < 0) {
> -- 
> 2.13.6
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 07/11/2017 18:39, Daniel P. Berrange wrote:
> On Tue, Nov 07, 2017 at 06:26:38PM +0100, Kevin Wolf wrote:
>> bdrv_set_read_only() is used by some block drivers to override the
>> read-only option given by the user. This is not how read-only images
>> generally work in QEMU: Instead of second guessing what the user really
>> meant (which currently includes making an image read-only even if the
>> user didn't only use the default, but explicitly said read-only=off), we
>> should error out if we can't provide what the user requested.
>>
>> This adds deprecation warnings to all callers of bdrv_set_read_only() so
>> that the behaviour can be corrected after the usual deprecation period.
> 
> All deprecations should be listed in "Deprecated features" appendix
> in qemu-doc.texi. This probably fits in the 'system emulator command
> line arguments' section, even though its talking about the need for
> the user to add something extra, rather than deleting something they
> currently use.

I am not sure this counts as deprecation, but it should go in the
release notes as "future incompatible changes", and that section
probably should go in qemu-doc.texi itself.

Paolo

> 
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  block.c       |  5 +++++
>>  block/bochs.c | 13 ++++++++++---
>>  block/cloop.c | 13 ++++++++++---
>>  block/dmg.c   | 12 +++++++++---
>>  block/rbd.c   | 14 ++++++++++----
>>  block/vvfat.c |  6 +++++-
>>  6 files changed, 49 insertions(+), 14 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index f6415547fe..0ed0c27140 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -261,6 +261,11 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
>>      return 0;
>>  }
>>  
>> +/* TODO Remove (deprecated since 2.11)
>> + * Block drivers are not supposed to automatically change bs->read_only.
>> + * Instead, they should just check whether they can provide what the user
>> + * explicitly requested and error out if read-write is requested, but they can
>> + * only provide read-only access. */
>>  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>>  {
>>      int ret = 0;
>> diff --git a/block/bochs.c b/block/bochs.c
>> index a759b6eff0..50c630047b 100644
>> --- a/block/bochs.c
>> +++ b/block/bochs.c
>> @@ -28,6 +28,7 @@
>>  #include "block/block_int.h"
>>  #include "qemu/module.h"
>>  #include "qemu/bswap.h"
>> +#include "qemu/error-report.h"
>>  
>>  /**************************************************************/
>>  
>> @@ -110,9 +111,15 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
>>          return -EINVAL;
>>      }
>>  
>> -    ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
>> -    if (ret < 0) {
>> -        return ret;
>> +    if (!bdrv_is_read_only(bs)) {
>> +        error_report("Opening bochs images without an explicit read-only=on "
>> +                     "option is deprecated. Future versions will refuse to "
>> +                     "open the image instead of automatically marking the "
>> +                     "image read-only.");
>> +        ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>>      }
>>  
>>      ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
>> diff --git a/block/cloop.c b/block/cloop.c
>> index d6597fcf78..2be68987bd 100644
>> --- a/block/cloop.c
>> +++ b/block/cloop.c
>> @@ -23,6 +23,7 @@
>>   */
>>  #include "qemu/osdep.h"
>>  #include "qapi/error.h"
>> +#include "qemu/error-report.h"
>>  #include "qemu-common.h"
>>  #include "block/block_int.h"
>>  #include "qemu/module.h"
>> @@ -72,9 +73,15 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
>>          return -EINVAL;
>>      }
>>  
>> -    ret = bdrv_set_read_only(bs, true, errp);
>> -    if (ret < 0) {
>> -        return ret;
>> +    if (!bdrv_is_read_only(bs)) {
>> +        error_report("Opening cloop images without an explicit read-only=on "
>> +                     "option is deprecated. Future versions will refuse to "
>> +                     "open the image instead of automatically marking the "
>> +                     "image read-only.");
>> +        ret = bdrv_set_read_only(bs, true, errp);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>>      }
>>  
>>      /* read header */
>> diff --git a/block/dmg.c b/block/dmg.c
>> index 6c0711f563..c9b3c519c4 100644
>> --- a/block/dmg.c
>> +++ b/block/dmg.c
>> @@ -419,9 +419,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>>          return -EINVAL;
>>      }
>>  
>> -    ret = bdrv_set_read_only(bs, true, errp);
>> -    if (ret < 0) {
>> -        return ret;
>> +    if (!bdrv_is_read_only(bs)) {
>> +        error_report("Opening dmg images without an explicit read-only=on "
>> +                     "option is deprecated. Future versions will refuse to "
>> +                     "open the image instead of automatically marking the "
>> +                     "image read-only.");
>> +        ret = bdrv_set_read_only(bs, true, errp);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>>      }
>>  
>>      block_module_load_one("dmg-bz2");
>> diff --git a/block/rbd.c b/block/rbd.c
>> index 144f350e1f..a76a5e8755 100644
>> --- a/block/rbd.c
>> +++ b/block/rbd.c
>> @@ -665,10 +665,16 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>>      /* If we are using an rbd snapshot, we must be r/o, otherwise
>>       * leave as-is */
>>      if (s->snap != NULL) {
>> -        r = bdrv_set_read_only(bs, true, &local_err);
>> -        if (r < 0) {
>> -            error_propagate(errp, local_err);
>> -            goto failed_open;
>> +        if (!bdrv_is_read_only(bs)) {
>> +            error_report("Opening rbd snapshots without an explicit "
>> +                         "read-only=on option is deprecated. Future versions "
>> +                         "will refuse to open the image instead of "
>> +                         "automatically marking the image read-only.");
>> +            r = bdrv_set_read_only(bs, true, &local_err);
>> +            if (r < 0) {
>> +                error_propagate(errp, local_err);
>> +                goto failed_open;
>> +            }
>>          }
>>      }
>>  
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index a0f2335894..0841cc42fc 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -1259,7 +1259,11 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>>                         "Unable to set VVFAT to 'rw' when drive is read-only");
>>              goto fail;
>>          }
>> -    } else  {
>> +    } else  if (!bdrv_is_read_only(bs)) {
>> +        error_report("Opening non-rw vvfat images without an explicit "
>> +                     "read-only=on option is deprecated. Future versions "
>> +                     "will refuse to open the image instead of "
>> +                     "automatically marking the image read-only.");
>>          /* read only is the default for safety */
>>          ret = bdrv_set_read_only(bs, true, &local_err);
>>          if (ret < 0) {
>> -- 
>> 2.13.6
>>
>>
> 
> Regards,
> Daniel
> 

On Wed, Nov 08, 2017 at 11:44:01AM +0100, Paolo Bonzini wrote:
> On 07/11/2017 18:39, Daniel P. Berrange wrote:
> > On Tue, Nov 07, 2017 at 06:26:38PM +0100, Kevin Wolf wrote:
> >> bdrv_set_read_only() is used by some block drivers to override the
> >> read-only option given by the user. This is not how read-only images
> >> generally work in QEMU: Instead of second guessing what the user really
> >> meant (which currently includes making an image read-only even if the
> >> user didn't only use the default, but explicitly said read-only=off), we
> >> should error out if we can't provide what the user requested.
> >>
> >> This adds deprecation warnings to all callers of bdrv_set_read_only() so
> >> that the behaviour can be corrected after the usual deprecation period.
> > 
> > All deprecations should be listed in "Deprecated features" appendix
> > in qemu-doc.texi. This probably fits in the 'system emulator command
> > line arguments' section, even though its talking about the need for
> > the user to add something extra, rather than deleting something they
> > currently use.
> 
> I am not sure this counts as deprecation, but it should go in the
> release notes as "future incompatible changes", and that section
> probably should go in qemu-doc.texi itself.

Yeah, adding a "Incompatible changes" appendix to the qemu-doc.texi
would be useful, listing the planned change, and when it is actually
made. That way apps adding support for a feature have an indication
of any incompatiblities they might need to care about.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Am 08.11.2017 um 11:49 hat Daniel P. Berrange geschrieben:
> On Wed, Nov 08, 2017 at 11:44:01AM +0100, Paolo Bonzini wrote:
> > On 07/11/2017 18:39, Daniel P. Berrange wrote:
> > > On Tue, Nov 07, 2017 at 06:26:38PM +0100, Kevin Wolf wrote:
> > >> bdrv_set_read_only() is used by some block drivers to override the
> > >> read-only option given by the user. This is not how read-only images
> > >> generally work in QEMU: Instead of second guessing what the user really
> > >> meant (which currently includes making an image read-only even if the
> > >> user didn't only use the default, but explicitly said read-only=off), we
> > >> should error out if we can't provide what the user requested.
> > >>
> > >> This adds deprecation warnings to all callers of bdrv_set_read_only() so
> > >> that the behaviour can be corrected after the usual deprecation period.
> > > 
> > > All deprecations should be listed in "Deprecated features" appendix
> > > in qemu-doc.texi. This probably fits in the 'system emulator command
> > > line arguments' section, even though its talking about the need for
> > > the user to add something extra, rather than deleting something they
> > > currently use.
> > 
> > I am not sure this counts as deprecation, but it should go in the
> > release notes as "future incompatible changes", and that section
> > probably should go in qemu-doc.texi itself.
> 
> Yeah, adding a "Incompatible changes" appendix to the qemu-doc.texi
> would be useful, listing the planned change, and when it is actually
> made. That way apps adding support for a feature have an indication
> of any incompatiblities they might need to care about.

You mean a section containing future incompatible changes as well as
already implemented incompatible changes?

What would we do with the existing "Deprecated features" section? Would
it become a subsection of "Incompatible changes"? Or would we just
rename it and the subsections would stay on the same level and get
"deprecated" added to their title? Or a completely different structure?

I'm okay with adding a little documentation in this patch if I know what
it should look like, but if it turns into a major overhaul of the
documentation on incompatible changes, it's probably out of scope for
this patch.

Kevin

On 08/11/2017 12:51, Kevin Wolf wrote:
> Am 08.11.2017 um 11:49 hat Daniel P. Berrange geschrieben:
>> On Wed, Nov 08, 2017 at 11:44:01AM +0100, Paolo Bonzini wrote:
>>> I am not sure this counts as deprecation, but it should go in the
>>> release notes as "future incompatible changes", and that section
>>> probably should go in qemu-doc.texi itself.
>>
>> Yeah, adding a "Incompatible changes" appendix to the qemu-doc.texi
>> would be useful, listing the planned change, and when it is actually
>> made. That way apps adding support for a feature have an indication
>> of any incompatiblities they might need to care about.
> 
> You mean a section containing future incompatible changes as well as
> already implemented incompatible changes?
> 
> What would we do with the existing "Deprecated features" section? Would
> it become a subsection of "Incompatible changes"? Or would we just
> rename it and the subsections would stay on the same level and get
> "deprecated" added to their title? Or a completely different structure?
> 
> I'm okay with adding a little documentation in this patch if I know what
> it should look like, but if it turns into a major overhaul of the
> documentation on incompatible changes, it's probably out of scope for
> this patch.

For now I would just add a section to the changelog.  That ensures that
we don't forget and end up doing nothing.

Paolo

Am 08.11.2017 um 13:00 hat Paolo Bonzini geschrieben:
> On 08/11/2017 12:51, Kevin Wolf wrote:
> > Am 08.11.2017 um 11:49 hat Daniel P. Berrange geschrieben:
> >> On Wed, Nov 08, 2017 at 11:44:01AM +0100, Paolo Bonzini wrote:
> >>> I am not sure this counts as deprecation, but it should go in the
> >>> release notes as "future incompatible changes", and that section
> >>> probably should go in qemu-doc.texi itself.
> >>
> >> Yeah, adding a "Incompatible changes" appendix to the qemu-doc.texi
> >> would be useful, listing the planned change, and when it is actually
> >> made. That way apps adding support for a feature have an indication
> >> of any incompatiblities they might need to care about.
> > 
> > You mean a section containing future incompatible changes as well as
> > already implemented incompatible changes?
> > 
> > What would we do with the existing "Deprecated features" section? Would
> > it become a subsection of "Incompatible changes"? Or would we just
> > rename it and the subsections would stay on the same level and get
> > "deprecated" added to their title? Or a completely different structure?
> > 
> > I'm okay with adding a little documentation in this patch if I know what
> > it should look like, but if it turns into a major overhaul of the
> > documentation on incompatible changes, it's probably out of scope for
> > this patch.
> 
> For now I would just add a section to the changelog.  That ensures that
> we don't forget and end up doing nothing.

Okay, done. Thanks!

Kevin

On Wed, Nov 08, 2017 at 12:51:27PM +0100, Kevin Wolf wrote:
> Am 08.11.2017 um 11:49 hat Daniel P. Berrange geschrieben:
> > On Wed, Nov 08, 2017 at 11:44:01AM +0100, Paolo Bonzini wrote:
> > > On 07/11/2017 18:39, Daniel P. Berrange wrote:
> > > > On Tue, Nov 07, 2017 at 06:26:38PM +0100, Kevin Wolf wrote:
> > > >> bdrv_set_read_only() is used by some block drivers to override the
> > > >> read-only option given by the user. This is not how read-only images
> > > >> generally work in QEMU: Instead of second guessing what the user really
> > > >> meant (which currently includes making an image read-only even if the
> > > >> user didn't only use the default, but explicitly said read-only=off), we
> > > >> should error out if we can't provide what the user requested.
> > > >>
> > > >> This adds deprecation warnings to all callers of bdrv_set_read_only() so
> > > >> that the behaviour can be corrected after the usual deprecation period.
> > > > 
> > > > All deprecations should be listed in "Deprecated features" appendix
> > > > in qemu-doc.texi. This probably fits in the 'system emulator command
> > > > line arguments' section, even though its talking about the need for
> > > > the user to add something extra, rather than deleting something they
> > > > currently use.
> > > 
> > > I am not sure this counts as deprecation, but it should go in the
> > > release notes as "future incompatible changes", and that section
> > > probably should go in qemu-doc.texi itself.
> > 
> > Yeah, adding a "Incompatible changes" appendix to the qemu-doc.texi
> > would be useful, listing the planned change, and when it is actually
> > made. That way apps adding support for a feature have an indication
> > of any incompatiblities they might need to care about.
> 
> You mean a section containing future incompatible changes as well as
> already implemented incompatible changes?
> 
> What would we do with the existing "Deprecated features" section? Would
> it become a subsection of "Incompatible changes"? Or would we just
> rename it and the subsections would stay on the same level and get
> "deprecated" added to their title? Or a completely different structure?

Yes, we could rename "Deprecated features" to "Deprecations & incompatible 
changes",  And then add the word "Deprecated" to the current @section
headings, and add a separate @section for things which are simply warning
about future incompatible changes which aren't strictly deprcations.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 11/07/2017 11:26 AM, Kevin Wolf wrote:
> bdrv_set_read_only() is used by some block drivers to override the
> read-only option given by the user. This is not how read-only images
> generally work in QEMU: Instead of second guessing what the user really
> meant (which currently includes making an image read-only even if the
> user didn't only use the default, but explicitly said read-only=off), we
> should error out if we can't provide what the user requested.
> 
> This adds deprecation warnings to all callers of bdrv_set_read_only() so
> that the behaviour can be corrected after the usual deprecation period.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c       |  5 +++++
>  block/bochs.c | 13 ++++++++++---
>  block/cloop.c | 13 ++++++++++---
>  block/dmg.c   | 12 +++++++++---
>  block/rbd.c   | 14 ++++++++++----
>  block/vvfat.c |  6 +++++-
>  6 files changed, 49 insertions(+), 14 deletions(-)

Dan pointed out the missing documentation, but for the code itself, the
approach looks sane (especially since it was my attempt to make it worse
by extending the idiom to NBD that triggered you to write this patch).

Other documentation: In qapi/block-core.json, @BlockdevOptions, we
probably ought to mention under @read-only that some block drivers
require the use of an explicit read-only.

> +++ b/block/vvfat.c
> @@ -1259,7 +1259,11 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>                         "Unable to set VVFAT to 'rw' when drive is read-only");
>              goto fail;
>          }
> -    } else  {
> +    } else  if (!bdrv_is_read_only(bs)) {
> +        error_report("Opening non-rw vvfat images without an explicit "
> +                     "read-only=on option is deprecated. Future versions "
> +                     "will refuse to open the image instead of "
> +                     "automatically marking the image read-only.");
>          /* read only is the default for safety */
>          ret = bdrv_set_read_only(bs, true, &local_err);

Is this also a good time to deprecate vvfat's duplication of rw vs.
read-only, and consolidate that into a single option?  No other device
defaults to read-only, so the deprecation period is a good point to warn
that a future version may default to read-write without an explicit
read-only.  I guess vvfat is the only driver with a device-specific QAPI
change (for 'rw') that might be impacted if you make that additional change.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Am 07.11.2017 um 21:29 hat Eric Blake geschrieben:
> On 11/07/2017 11:26 AM, Kevin Wolf wrote:
> > bdrv_set_read_only() is used by some block drivers to override the
> > read-only option given by the user. This is not how read-only images
> > generally work in QEMU: Instead of second guessing what the user really
> > meant (which currently includes making an image read-only even if the
> > user didn't only use the default, but explicitly said read-only=off), we
> > should error out if we can't provide what the user requested.
> > 
> > This adds deprecation warnings to all callers of bdrv_set_read_only() so
> > that the behaviour can be corrected after the usual deprecation period.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c       |  5 +++++
> >  block/bochs.c | 13 ++++++++++---
> >  block/cloop.c | 13 ++++++++++---
> >  block/dmg.c   | 12 +++++++++---
> >  block/rbd.c   | 14 ++++++++++----
> >  block/vvfat.c |  6 +++++-
> >  6 files changed, 49 insertions(+), 14 deletions(-)
> 
> Dan pointed out the missing documentation, but for the code itself, the
> approach looks sane (especially since it was my attempt to make it worse
> by extending the idiom to NBD that triggered you to write this patch).
> 
> Other documentation: In qapi/block-core.json, @BlockdevOptions, we
> probably ought to mention under @read-only that some block drivers
> require the use of an explicit read-only.

Well, they don't only need an explicitly set option, but the important
point is that they don't work with the default value. But I can add
something to this effect.

> > +++ b/block/vvfat.c
> > @@ -1259,7 +1259,11 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> >                         "Unable to set VVFAT to 'rw' when drive is read-only");
> >              goto fail;
> >          }
> > -    } else  {
> > +    } else  if (!bdrv_is_read_only(bs)) {
> > +        error_report("Opening non-rw vvfat images without an explicit "
> > +                     "read-only=on option is deprecated. Future versions "
> > +                     "will refuse to open the image instead of "
> > +                     "automatically marking the image read-only.");
> >          /* read only is the default for safety */
> >          ret = bdrv_set_read_only(bs, true, &local_err);
> 
> Is this also a good time to deprecate vvfat's duplication of rw vs.
> read-only, and consolidate that into a single option?  No other device
> defaults to read-only, so the deprecation period is a good point to warn
> that a future version may default to read-write without an explicit
> read-only.  I guess vvfat is the only driver with a device-specific QAPI
> change (for 'rw') that might be impacted if you make that additional change.

I would love to get rid of the duplication, but there's a reason why
vvfat defaults to read-only. I think we're relatively confident that a
read-only vvfat can be safely implemented (and hopefully is), but write
support is really a clever hack that may or may not work reliably
depending on how crazy the guest OS goes.

So if we removed the 'rw' option, would we want 'read-only' to default
to true for vvfat? I'm not sure if we want to go there, it would mean
making the default value of some base BlockdevOptions depend on the
driver.

On the other hand, I'm not sure how useful 'read-only' even is apart
from the protocol layer... Should it have been driver-specific? But it's
too late for that anyway.

Kevin

Am 08.11.2017 um 11:04 hat Kevin Wolf geschrieben:
> Am 07.11.2017 um 21:29 hat Eric Blake geschrieben:
> > On 11/07/2017 11:26 AM, Kevin Wolf wrote:
> > > bdrv_set_read_only() is used by some block drivers to override the
> > > read-only option given by the user. This is not how read-only images
> > > generally work in QEMU: Instead of second guessing what the user really
> > > meant (which currently includes making an image read-only even if the
> > > user didn't only use the default, but explicitly said read-only=off), we
> > > should error out if we can't provide what the user requested.
> > > 
> > > This adds deprecation warnings to all callers of bdrv_set_read_only() so
> > > that the behaviour can be corrected after the usual deprecation period.
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  block.c       |  5 +++++
> > >  block/bochs.c | 13 ++++++++++---
> > >  block/cloop.c | 13 ++++++++++---
> > >  block/dmg.c   | 12 +++++++++---
> > >  block/rbd.c   | 14 ++++++++++----
> > >  block/vvfat.c |  6 +++++-
> > >  6 files changed, 49 insertions(+), 14 deletions(-)
> > 
> > Dan pointed out the missing documentation, but for the code itself, the
> > approach looks sane (especially since it was my attempt to make it worse
> > by extending the idiom to NBD that triggered you to write this patch).
> > 
> > Other documentation: In qapi/block-core.json, @BlockdevOptions, we
> > probably ought to mention under @read-only that some block drivers
> > require the use of an explicit read-only.
> 
> Well, they don't only need an explicitly set option, but the important
> point is that they don't work with the default value. But I can add
> something to this effect.

I'll squash this in if it looks good to you:

diff --git a/qapi/block-core.json b/qapi/block-core.json
index ab96e348e6..76bf50f813 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3134,8 +3134,11 @@
 #                 This option is required on the top level of blockdev-add.
 # @discard:       discard-related options (default: ignore)
 # @cache:         cache-related options
-# @read-only:     whether the block device should be read-only
-#                 (default: false)
+# @read-only:     whether the block device should be read-only (default: false).
+#                 Note that some block drivers support only read-only access,
+#                 either generally or in certain configurations. In this case,
+#                 the default value does not work and the option must be
+#                 specified explicitly.
 # @detect-zeroes: detect and optimize zero writes (Since 2.1)
 #                 (default: off)
 # @force-share:   force share all permission on added nodes.

On 11/08/2017 06:20 AM, Kevin Wolf wrote:

>> Well, they don't only need an explicitly set option, but the important
>> point is that they don't work with the default value. But I can add
>> something to this effect.
> 
> I'll squash this in if it looks good to you:
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ab96e348e6..76bf50f813 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3134,8 +3134,11 @@
>  #                 This option is required on the top level of blockdev-add.
>  # @discard:       discard-related options (default: ignore)
>  # @cache:         cache-related options
> -# @read-only:     whether the block device should be read-only
> -#                 (default: false)
> +# @read-only:     whether the block device should be read-only (default: false).
> +#                 Note that some block drivers support only read-only access,
> +#                 either generally or in certain configurations. In this case,
> +#                 the default value does not work and the option must be
> +#                 specified explicitly.

Yes, that looks reasonable, if we aren't interested in toying with the
idea of a per-driver default instead.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 11/08/2017 04:04 AM, Kevin Wolf wrote:

> 
> Well, they don't only need an explicitly set option, but the important
> point is that they don't work with the default value. But I can add
> something to this effect.
> 
>>> +++ b/block/vvfat.c
>>> @@ -1259,7 +1259,11 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>>>                         "Unable to set VVFAT to 'rw' when drive is read-only");
>>>              goto fail;
>>>          }
>>> -    } else  {
>>> +    } else  if (!bdrv_is_read_only(bs)) {
>>> +        error_report("Opening non-rw vvfat images without an explicit "
>>> +                     "read-only=on option is deprecated. Future versions "
>>> +                     "will refuse to open the image instead of "
>>> +                     "automatically marking the image read-only.");
>>>          /* read only is the default for safety */
>>>          ret = bdrv_set_read_only(bs, true, &local_err);
>>
>> Is this also a good time to deprecate vvfat's duplication of rw vs.
>> read-only, and consolidate that into a single option?  No other device
>> defaults to read-only, so the deprecation period is a good point to warn
>> that a future version may default to read-write without an explicit
>> read-only.  I guess vvfat is the only driver with a device-specific QAPI
>> change (for 'rw') that might be impacted if you make that additional change.
> 
> I would love to get rid of the duplication, but there's a reason why
> vvfat defaults to read-only. I think we're relatively confident that a
> read-only vvfat can be safely implemented (and hopefully is), but write
> support is really a clever hack that may or may not work reliably
> depending on how crazy the guest OS goes.
> 
> So if we removed the 'rw' option, would we want 'read-only' to default
> to true for vvfat? I'm not sure if we want to go there, it would mean
> making the default value of some base BlockdevOptions depend on the
> driver.
> 
> On the other hand, I'm not sure how useful 'read-only' even is apart
> from the protocol layer... Should it have been driver-specific? But it's
> too late for that anyway.

Having a driver-specific default for read-only MIGHT make sense, as a
plan for something down the road (it matches current behavior, after
all, in that some drivers force read-only as their default).  I guess
now is the time to decide WHAT we want to do after the deprecation
period ends, so that we're only making an incompatible change once, and
tweak the deprecation (and resulting warning messages in the meantime)
to fit in with that plan.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org