[Qemu-devel] [PATCH for-2.10 0/9] RBD reopen, read_only cleanup

[Qemu-devel] [PATCH for-2.10 0/9] RBD reopen, read_only cleanup

[Jeff Cody]

This series does two things:

1.) Cleans up some of the logic behind setting the read_only flag
    for a BDS in the block layer, so that it is done consistently
    (and rules are applied consistently), and

2.) Adds .bdrv_reopen_prepare() implementation for RBD, so that block
    jobs can be run on backing chains that have rbd protocol nodes.

Jeff Cody (9):
  block: add bdrv_set_read_only() helper function
  block: do not set BDS read_only if copy_on_read enabled
  block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only
  block: code movement
  block: introduce bdrv_try_set_read_only()
  block: use bdrv_try_set_read_only() during reopen
  block/rbd - update variable names to more apt names
  block/rbd: do not blindly set bs->read_only
  block/rbd: Add support for reopen()

 block.c               | 56 +++++++++++++++++++++++++++++++++++---------
 block/bochs.c         |  5 +++-
 block/cloop.c         |  5 +++-
 block/dmg.c           |  6 ++++-
 block/rbd.c           | 65 +++++++++++++++++++++++++++++++++++++--------------
 block/vvfat.c         | 15 +++++++++---
 include/block/block.h |  2 ++
 7 files changed, 120 insertions(+), 34 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH for-2.10 1/9] block: add bdrv_set_read_only() helper function

[Jeff Cody]

We have a helper wrapper for checking for the BDS read_only flag,
add a helper wrapper to set the read_only flag as well.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c               | 5 +++++
 block/bochs.c         | 2 +-
 block/cloop.c         | 2 +-
 block/dmg.c           | 2 +-
 block/rbd.c           | 2 +-
 block/vvfat.c         | 4 ++--
 include/block/block.h | 1 +
 7 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 927ba89..7b4c7ef 100644
--- a/block.c
+++ b/block.c
@@ -192,6 +192,11 @@ void path_combine(char *dest, int dest_size,
     }
 }
 
+void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
+{
+    bs->read_only = read_only;
+}
+
 void bdrv_get_full_backing_filename_from_filename(const char *backed,
                                                   const char *backing,
                                                   char *dest, size_t sz,
diff --git a/block/bochs.c b/block/bochs.c
index 516da56..bdc2831 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -110,7 +110,7 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    bs->read_only = true; /* no write support yet */
+    bdrv_set_read_only(bs, true); /* no write support yet */
 
     ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
     if (ret < 0) {
diff --git a/block/cloop.c b/block/cloop.c
index a6c7b9d..11f17c8 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -72,7 +72,7 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    bs->read_only = true;
+    bdrv_set_read_only(bs, true);
 
     /* read header */
     ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
diff --git a/block/dmg.c b/block/dmg.c
index a7d25fc..27ce4a6 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -420,7 +420,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     block_module_load_one("dmg-bz2");
-    bs->read_only = true;
+    bdrv_set_read_only(bs, true);
 
     s->n_chunks = 0;
     s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
diff --git a/block/rbd.c b/block/rbd.c
index 1ceeeb5..6ad2904 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -641,7 +641,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_open;
     }
 
-    bs->read_only = (s->snap != NULL);
+    bdrv_set_read_only(bs, (s->snap != NULL));
 
     qemu_opts_del(opts);
     return 0;
diff --git a/block/vvfat.c b/block/vvfat.c
index af5153d..d4ce6d7 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1157,7 +1157,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
     s->current_cluster=0xffffffff;
 
     /* read only is the default for safety */
-    bs->read_only = true;
+    bdrv_set_read_only(bs, true);
     s->qcow = NULL;
     s->qcow_filename = NULL;
     s->fat2 = NULL;
@@ -1173,7 +1173,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         if (ret < 0) {
             goto fail;
         }
-        bs->read_only = false;
+        bdrv_set_read_only(bs, false);
     }
 
     bs->total_sectors = cyls * heads * secs;
diff --git a/include/block/block.h b/include/block/block.h
index 5149260..06c9032 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
                             int64_t sector_num, int nb_sectors, int *pnum);
 
 bool bdrv_is_read_only(BlockDriverState *bs);
+void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
 int bdrv_media_changed(BlockDriverState *bs);
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-2.10 1/9] block: add bdrv_set_read_only() helper function

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 589 bytes --]

On Wed, Apr 05, 2017 at 02:28:43PM -0400, Jeff Cody wrote:
> We have a helper wrapper for checking for the BDS read_only flag,
> add a helper wrapper to set the read_only flag as well.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c               | 5 +++++
>  block/bochs.c         | 2 +-
>  block/cloop.c         | 2 +-
>  block/dmg.c           | 2 +-
>  block/rbd.c           | 2 +-
>  block/vvfat.c         | 4 ++--
>  include/block/block.h | 1 +
>  7 files changed, 12 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled

[Jeff Cody]

A few block drivers will set the BDS read_only flag from their
.bdrv_open() function.  This means the bs->read_only flag could
be set after we enable copy_on_read, as the BDRV_O_COPY_ON_READ
flag check occurs prior to the call to bdrv->bdrv_open().

This adds an error return to bdrv_set_read_only(), and an error will be
return if we try to set the BDS to read_only while copy_on_read is
enabled.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c               | 10 +++++++++-
 block/bochs.c         |  5 ++++-
 block/cloop.c         |  5 ++++-
 block/dmg.c           |  6 +++++-
 block/rbd.c           |  6 +++++-
 block/vvfat.c         | 15 ++++++++++++---
 include/block/block.h |  2 +-
 7 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 7b4c7ef..f60d5ea 100644
--- a/block.c
+++ b/block.c
@@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size,
     }
 }
 
-void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
+int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
 {
+    /* Do not set read_only if copy_on_read is enabled */
+    if (bs->copy_on_read && read_only) {
+        error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",
+                   bdrv_get_device_or_node_name(bs));
+        return -EINVAL;
+    }
+
     bs->read_only = read_only;
+    return 0;
 }
 
 void bdrv_get_full_backing_filename_from_filename(const char *backed,
diff --git a/block/bochs.c b/block/bochs.c
index bdc2831..a759b6e 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -110,7 +110,10 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    bdrv_set_read_only(bs, true); /* no write support yet */
+    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));
     if (ret < 0) {
diff --git a/block/cloop.c b/block/cloop.c
index 11f17c8..d6597fc 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -72,7 +72,10 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    bdrv_set_read_only(bs, true);
+    ret = bdrv_set_read_only(bs, true, errp);
+    if (ret < 0) {
+        return ret;
+    }
 
     /* read header */
     ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
diff --git a/block/dmg.c b/block/dmg.c
index 27ce4a6..900ae5a 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -419,8 +419,12 @@ 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;
+    }
+
     block_module_load_one("dmg-bz2");
-    bdrv_set_read_only(bs, true);
 
     s->n_chunks = 0;
     s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
diff --git a/block/rbd.c b/block/rbd.c
index 6ad2904..328e4a9 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -641,7 +641,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_open;
     }
 
-    bdrv_set_read_only(bs, (s->snap != NULL));
+    r = bdrv_set_read_only(bs, (s->snap != NULL), &local_err);
+    if (r < 0) {
+        error_propagate(errp, local_err);
+        goto failed_open;
+    }
 
     qemu_opts_del(opts);
     return 0;
diff --git a/block/vvfat.c b/block/vvfat.c
index d4ce6d7..34a2854 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1156,8 +1156,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->current_cluster=0xffffffff;
 
-    /* read only is the default for safety */
-    bdrv_set_read_only(bs, true);
     s->qcow = NULL;
     s->qcow_filename = NULL;
     s->fat2 = NULL;
@@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         if (ret < 0) {
             goto fail;
         }
-        bdrv_set_read_only(bs, false);
+        ret = bdrv_set_read_only(bs, false, &local_err);
+        if (ret < 0) {
+            error_propagate(errp, local_err);
+            goto fail;
+        }
+    } else  {
+        /* read only is the default for safety */
+        ret = bdrv_set_read_only(bs, true, &local_err);
+        if (ret < 0) {
+            error_propagate(errp, local_err);
+            goto fail;
+        }
     }
 
     bs->total_sectors = cyls * heads * secs;
diff --git a/include/block/block.h b/include/block/block.h
index 06c9032..beb563a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -426,7 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
                             int64_t sector_num, int nb_sectors, int *pnum);
 
 bool bdrv_is_read_only(BlockDriverState *bs);
-void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
+int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
 int bdrv_media_changed(BlockDriverState *bs);
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled

[John Snow]

On 04/05/2017 02:28 PM, Jeff Cody wrote:
> A few block drivers will set the BDS read_only flag from their
> .bdrv_open() function.  This means the bs->read_only flag could
> be set after we enable copy_on_read, as the BDRV_O_COPY_ON_READ
> flag check occurs prior to the call to bdrv->bdrv_open().
> 
> This adds an error return to bdrv_set_read_only(), and an error will be
> return if we try to set the BDS to read_only while copy_on_read is
> enabled.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c               | 10 +++++++++-
>  block/bochs.c         |  5 ++++-
>  block/cloop.c         |  5 ++++-
>  block/dmg.c           |  6 +++++-
>  block/rbd.c           |  6 +++++-
>  block/vvfat.c         | 15 ++++++++++++---
>  include/block/block.h |  2 +-
>  7 files changed, 40 insertions(+), 9 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 7b4c7ef..f60d5ea 100644
> --- a/block.c
> +++ b/block.c
> @@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size,
>      }
>  }
>  
> -void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
> +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>  {
> +    /* Do not set read_only if copy_on_read is enabled */
> +    if (bs->copy_on_read && read_only) {
> +        error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",

COW?

> +                   bdrv_get_device_or_node_name(bs));
> +        return -EINVAL;
> +    }
> +
>      bs->read_only = read_only;
> +    return 0;
>  }
>  
>  void bdrv_get_full_backing_filename_from_filename(const char *backed,
> diff --git a/block/bochs.c b/block/bochs.c
> index bdc2831..a759b6e 100644
> --- a/block/bochs.c
> +++ b/block/bochs.c
> @@ -110,7 +110,10 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
>          return -EINVAL;
>      }
>  
> -    bdrv_set_read_only(bs, true); /* no write support yet */
> +    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));
>      if (ret < 0) {
> diff --git a/block/cloop.c b/block/cloop.c
> index 11f17c8..d6597fc 100644
> --- a/block/cloop.c
> +++ b/block/cloop.c
> @@ -72,7 +72,10 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
>          return -EINVAL;
>      }
>  
> -    bdrv_set_read_only(bs, true);
> +    ret = bdrv_set_read_only(bs, true, errp);
> +    if (ret < 0) {
> +        return ret;
> +    }
>  
>      /* read header */
>      ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
> diff --git a/block/dmg.c b/block/dmg.c
> index 27ce4a6..900ae5a 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -419,8 +419,12 @@ 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;
> +    }
> +
>      block_module_load_one("dmg-bz2");
> -    bdrv_set_read_only(bs, true);
>  
>      s->n_chunks = 0;
>      s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> diff --git a/block/rbd.c b/block/rbd.c
> index 6ad2904..328e4a9 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -641,7 +641,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          goto failed_open;
>      }
>  
> -    bdrv_set_read_only(bs, (s->snap != NULL));
> +    r = bdrv_set_read_only(bs, (s->snap != NULL), &local_err);
> +    if (r < 0) {
> +        error_propagate(errp, local_err);
> +        goto failed_open;
> +    }
>  
>      qemu_opts_del(opts);
>      return 0;
> diff --git a/block/vvfat.c b/block/vvfat.c
> index d4ce6d7..34a2854 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1156,8 +1156,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      s->current_cluster=0xffffffff;
>  
> -    /* read only is the default for safety */
> -    bdrv_set_read_only(bs, true);
>      s->qcow = NULL;
>      s->qcow_filename = NULL;
>      s->fat2 = NULL;
> @@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>          if (ret < 0) {
>              goto fail;
>          }
> -        bdrv_set_read_only(bs, false);
> +        ret = bdrv_set_read_only(bs, false, &local_err);
> +        if (ret < 0) {
> +            error_propagate(errp, local_err);
> +            goto fail;
> +        }
> +    } else  {
> +        /* read only is the default for safety */
> +        ret = bdrv_set_read_only(bs, true, &local_err);
> +        if (ret < 0) {
> +            error_propagate(errp, local_err);
> +            goto fail;
> +        }
>      }
>  
>      bs->total_sectors = cyls * heads * secs;
> diff --git a/include/block/block.h b/include/block/block.h
> index 06c9032..beb563a 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -426,7 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
>                              int64_t sector_num, int nb_sectors, int *pnum);
>  
>  bool bdrv_is_read_only(BlockDriverState *bs);
> -void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
> +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
>  bool bdrv_is_sg(BlockDriverState *bs);
>  bool bdrv_is_inserted(BlockDriverState *bs);
>  int bdrv_media_changed(BlockDriverState *bs);
> 

Re: [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled

[Jeff Cody]

On Wed, Apr 05, 2017 at 03:16:38PM -0400, John Snow wrote:
> 
> 
> On 04/05/2017 02:28 PM, Jeff Cody wrote:
> > A few block drivers will set the BDS read_only flag from their
> > .bdrv_open() function.  This means the bs->read_only flag could
> > be set after we enable copy_on_read, as the BDRV_O_COPY_ON_READ
> > flag check occurs prior to the call to bdrv->bdrv_open().
> > 
> > This adds an error return to bdrv_set_read_only(), and an error will be
> > return if we try to set the BDS to read_only while copy_on_read is
> > enabled.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c               | 10 +++++++++-
> >  block/bochs.c         |  5 ++++-
> >  block/cloop.c         |  5 ++++-
> >  block/dmg.c           |  6 +++++-
> >  block/rbd.c           |  6 +++++-
> >  block/vvfat.c         | 15 ++++++++++++---
> >  include/block/block.h |  2 +-
> >  7 files changed, 40 insertions(+), 9 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 7b4c7ef..f60d5ea 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size,
> >      }
> >  }
> >  
> > -void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
> > +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> >  {
> > +    /* Do not set read_only if copy_on_read is enabled */
> > +    if (bs->copy_on_read && read_only) {
> > +        error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",
> 
> COW?
>

Mooo! You are right, that should be COR (or better yet, I should just write
it out - copy on read).

> > +                   bdrv_get_device_or_node_name(bs));
> > +        return -EINVAL;
> > +    }
> > +
> >      bs->read_only = read_only;
> > +    return 0;
> >  }
> >  
> >  void bdrv_get_full_backing_filename_from_filename(const char *backed,
> > diff --git a/block/bochs.c b/block/bochs.c
> > index bdc2831..a759b6e 100644
> > --- a/block/bochs.c
> > +++ b/block/bochs.c
> > @@ -110,7 +110,10 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
> >          return -EINVAL;
> >      }
> >  
> > -    bdrv_set_read_only(bs, true); /* no write support yet */
> > +    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));
> >      if (ret < 0) {
> > diff --git a/block/cloop.c b/block/cloop.c
> > index 11f17c8..d6597fc 100644
> > --- a/block/cloop.c
> > +++ b/block/cloop.c
> > @@ -72,7 +72,10 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
> >          return -EINVAL;
> >      }
> >  
> > -    bdrv_set_read_only(bs, true);
> > +    ret = bdrv_set_read_only(bs, true, errp);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> >  
> >      /* read header */
> >      ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
> > diff --git a/block/dmg.c b/block/dmg.c
> > index 27ce4a6..900ae5a 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -419,8 +419,12 @@ 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;
> > +    }
> > +
> >      block_module_load_one("dmg-bz2");
> > -    bdrv_set_read_only(bs, true);
> >  
> >      s->n_chunks = 0;
> >      s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> > diff --git a/block/rbd.c b/block/rbd.c
> > index 6ad2904..328e4a9 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -641,7 +641,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> >          goto failed_open;
> >      }
> >  
> > -    bdrv_set_read_only(bs, (s->snap != NULL));
> > +    r = bdrv_set_read_only(bs, (s->snap != NULL), &local_err);
> > +    if (r < 0) {
> > +        error_propagate(errp, local_err);
> > +        goto failed_open;
> > +    }
> >  
> >      qemu_opts_del(opts);
> >      return 0;
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index d4ce6d7..34a2854 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvfat.c
> > @@ -1156,8 +1156,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> >  
> >      s->current_cluster=0xffffffff;
> >  
> > -    /* read only is the default for safety */
> > -    bdrv_set_read_only(bs, true);
> >      s->qcow = NULL;
> >      s->qcow_filename = NULL;
> >      s->fat2 = NULL;
> > @@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> >          if (ret < 0) {
> >              goto fail;
> >          }
> > -        bdrv_set_read_only(bs, false);
> > +        ret = bdrv_set_read_only(bs, false, &local_err);
> > +        if (ret < 0) {
> > +            error_propagate(errp, local_err);
> > +            goto fail;
> > +        }
> > +    } else  {
> > +        /* read only is the default for safety */
> > +        ret = bdrv_set_read_only(bs, true, &local_err);
> > +        if (ret < 0) {
> > +            error_propagate(errp, local_err);
> > +            goto fail;
> > +        }
> >      }
> >  
> >      bs->total_sectors = cyls * heads * secs;
> > diff --git a/include/block/block.h b/include/block/block.h
> > index 06c9032..beb563a 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -426,7 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> >                              int64_t sector_num, int nb_sectors, int *pnum);
> >  
> >  bool bdrv_is_read_only(BlockDriverState *bs);
> > -void bdrv_set_read_only(BlockDriverState *bs, bool read_only);
> > +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> >  bool bdrv_is_sg(BlockDriverState *bs);
> >  bool bdrv_is_inserted(BlockDriverState *bs);
> >  int bdrv_media_changed(BlockDriverState *bs);
> > 

Re: [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

On Wed, Apr 05, 2017 at 02:28:44PM -0400, Jeff Cody wrote:

Minor comments but:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

> diff --git a/block.c b/block.c
> index 7b4c7ef..f60d5ea 100644
> --- a/block.c
> +++ b/block.c
> @@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size,
>      }
>  }
>  
> -void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
> +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>  {
> +    /* Do not set read_only if copy_on_read is enabled */
> +    if (bs->copy_on_read && read_only) {
> +        error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",

Users might be puzzled by "COR".  The -drive option is called
"copy-on-read" so spelling it out is clearer than using an acronym.

> @@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>          if (ret < 0) {
>              goto fail;
>          }
> -        bdrv_set_read_only(bs, false);
> +        ret = bdrv_set_read_only(bs, false, &local_err);
> +        if (ret < 0) {
> +            error_propagate(errp, local_err);
> +            goto fail;
> +        }

read_only = false by default.  There's no need to set it now that you've
moved the bdrv_set_read_only(bs, true) call.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

On Wed, Apr 05, 2017 at 02:28:44PM -0400, Jeff Cody wrote:
> @@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>          if (ret < 0) {
>              goto fail;
>          }
> -        bdrv_set_read_only(bs, false);
> +        ret = bdrv_set_read_only(bs, false, &local_err);
> +        if (ret < 0) {
> +            error_propagate(errp, local_err);
> +            goto fail;
> +        }

I realized later in the series why you are doing this.

The error code path introduces a resource leak: enable_write_target()
has already been called and isn't cleaned up by the fail label.

It would be cleaner to check that bs is writable before calling
enable_write_target().

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only

[Jeff Cody]

The BDRV_O_ALLOW_RDWR flag allows / prohibits the changing of
the BDS 'read_only' state, but there are a few places where it
is ignored.  In the bdrv_set_read_only() helper, make sure to
honor the flag.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block.c b/block.c
index f60d5ea..4a61ff0 100644
--- a/block.c
+++ b/block.c
@@ -201,6 +201,13 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
         return -EINVAL;
     }
 
+    /* Do not clear read_only if it is prohibited */
+    if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) {
+        error_setg(errp, "Node '%s' is read only",
+                   bdrv_get_device_or_node_name(bs));
+        return -EPERM;
+    }
+
     bs->read_only = read_only;
     return 0;
 }
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only

[John Snow]

On 04/05/2017 02:28 PM, Jeff Cody wrote:
> The BDRV_O_ALLOW_RDWR flag allows / prohibits the changing of
> the BDS 'read_only' state, but there are a few places where it
> is ignored.  In the bdrv_set_read_only() helper, make sure to
> honor the flag.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block.c b/block.c
> index f60d5ea..4a61ff0 100644
> --- a/block.c
> +++ b/block.c
> @@ -201,6 +201,13 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>          return -EINVAL;
>      }
>  
> +    /* Do not clear read_only if it is prohibited */
> +    if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) {
> +        error_setg(errp, "Node '%s' is read only",
> +                   bdrv_get_device_or_node_name(bs));
> +        return -EPERM;
> +    }
> +
>      bs->read_only = read_only;
>      return 0;
>  }
> 

Conceptually straightforward.

looks like this might change behavior for... RBD and vvfat, right?
RBD is the subject of this series so we'll just assume that was broken
and stupid.

What's vvfat's story? It always set the read-only property to false
regardless of what you asked for?

Re: [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 646 bytes --]

On 04/05/2017 02:20 PM, John Snow wrote:

> Conceptually straightforward.
> 
> looks like this might change behavior for... RBD and vvfat, right?
> RBD is the subject of this series so we'll just assume that was broken
> and stupid.
> 
> What's vvfat's story? It always set the read-only property to false
> regardless of what you asked for?

vvfat is even stupider than that - it has its own independent property
'rw' that determines whether to allow write operations, separate from
the inherited BDS readonly property.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only

[Jeff Cody]

On Wed, Apr 05, 2017 at 02:26:53PM -0500, Eric Blake wrote:
> On 04/05/2017 02:20 PM, John Snow wrote:
> 
> > Conceptually straightforward.
> > 
> > looks like this might change behavior for... RBD and vvfat, right?
> > RBD is the subject of this series so we'll just assume that was broken
> > and stupid.
> > 

Yes on RBD, and that change is intentional.

> > What's vvfat's story? It always set the read-only property to false
> > regardless of what you asked for?
> 
> vvfat is even stupider than that - it has its own independent property
> 'rw' that determines whether to allow write operations, separate from
> the inherited BDS readonly property.
>

Yes, it is very odd.  But if we have copy_on_read enabled, or explicitly set
the block device to read-only via QAPI or -drive, I think that those should
take precedence.

Re: [Qemu-devel] [PATCH for-2.10 3/9] block: honor BDRV_O_ALLOW_RDWR when clearing bs->read_only

[Jeff Cody]

On Wed, Apr 05, 2017 at 08:20:28PM -0400, Jeff Cody wrote:
> On Wed, Apr 05, 2017 at 02:26:53PM -0500, Eric Blake wrote:
> > On 04/05/2017 02:20 PM, John Snow wrote:
> > 
> > > Conceptually straightforward.
> > > 
> > > looks like this might change behavior for... RBD and vvfat, right?
> > > RBD is the subject of this series so we'll just assume that was broken
> > > and stupid.
> > > 
> 
> Yes on RBD, and that change is intentional.
> 
> > > What's vvfat's story? It always set the read-only property to false
> > > regardless of what you asked for?
> > 
> > vvfat is even stupider than that - it has its own independent property
> > 'rw' that determines whether to allow write operations, separate from
> > the inherited BDS readonly property.
> >
> 
> Yes, it is very odd.  But if we have copy_on_read enabled, or explicitly set
> the block device to read-only via QAPI or -drive, I think that those should
> take precedence.
>

I meant to add an example - currently, with this drive commandline:

"-drive format=vvfat,dir=/tmp/vvfat,rw,if=virtio,readonly=on"

The drive is not read-only, but writable.  That seems broken.

After this patch, this ends up throwing an error now (which I think is a
logical thing to do):

qemu-system-x86_64: -drive format=vvfat,dir=/tmp/vvfat,rw,if=virtio,readonly=on: Node '#block238' is read only

How this affects vvfat (and rbd) should be documented in the commit message,
however, so I should ammend that if we keep this behavior.

One other possible option is to treat the vvfat 'rw' option as meaning
"enable writes, iff the block device is writable".  With that
interpretation, we could do something different in the above scenario:
silently fail to set bs->read_only to 'true' in the vvfat driver, and keep
it r/o.

[Qemu-devel] [PATCH for-2.10 4/9] block: code movement

[Jeff Cody]

Move bdrv_is_read_only() up with its friends.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 4a61ff0..8bfe7f4 100644
--- a/block.c
+++ b/block.c
@@ -192,6 +192,11 @@ void path_combine(char *dest, int dest_size,
     }
 }
 
+bool bdrv_is_read_only(BlockDriverState *bs)
+{
+    return bs->read_only;
+}
+
 int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
 {
     /* Do not set read_only if copy_on_read is enabled */
@@ -3367,11 +3372,6 @@ void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr)
     *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
 }
 
-bool bdrv_is_read_only(BlockDriverState *bs)
-{
-    return bs->read_only;
-}
-
 bool bdrv_is_sg(BlockDriverState *bs)
 {
     return bs->sg;
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-2.10 4/9] block: code movement

[John Snow]

On 04/05/2017 02:28 PM, Jeff Cody wrote:
> Move bdrv_is_read_only() up with its friends.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 4a61ff0..8bfe7f4 100644
> --- a/block.c
> +++ b/block.c
> @@ -192,6 +192,11 @@ void path_combine(char *dest, int dest_size,
>      }
>  }
>  
> +bool bdrv_is_read_only(BlockDriverState *bs)
> +{
> +    return bs->read_only;
> +}
> +
>  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>  {
>      /* Do not set read_only if copy_on_read is enabled */
> @@ -3367,11 +3372,6 @@ void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr)
>      *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
>  }
>  
> -bool bdrv_is_read_only(BlockDriverState *bs)
> -{
> -    return bs->read_only;
> -}
> -
>  bool bdrv_is_sg(BlockDriverState *bs)
>  {
>      return bs->sg;
> 

http://i.imgur.com/BrkMeS5.jpg

Re: [Qemu-devel] [PATCH for-2.10 4/9] block: code movement

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

On Wed, Apr 05, 2017 at 02:28:46PM -0400, Jeff Cody wrote:
> Move bdrv_is_read_only() up with its friends.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only()

[Jeff Cody]

Introduce try function for setting read_only flags.  Will return < 0 on
error, with appropriate Error value set.  Does not alter any flags.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c               | 14 +++++++++++++-
 include/block/block.h |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 8bfe7f4..ad958b9 100644
--- a/block.c
+++ b/block.c
@@ -197,7 +197,7 @@ bool bdrv_is_read_only(BlockDriverState *bs)
     return bs->read_only;
 }
 
-int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
 {
     /* Do not set read_only if copy_on_read is enabled */
     if (bs->copy_on_read && read_only) {
@@ -213,6 +213,18 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
         return -EPERM;
     }
 
+    return 0;
+}
+
+int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+{
+    int ret = 0;
+
+    ret = bdrv_try_set_read_only(bs, read_only, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
     bs->read_only = read_only;
     return 0;
 }
diff --git a/include/block/block.h b/include/block/block.h
index beb563a..0049b57 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
                             int64_t sector_num, int nb_sectors, int *pnum);
 
 bool bdrv_is_read_only(BlockDriverState *bs);
+int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only()

[John Snow]

On 04/05/2017 02:28 PM, Jeff Cody wrote:
> Introduce try function for setting read_only flags.  Will return < 0 on
> error, with appropriate Error value set.  Does not alter any flags.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c               | 14 +++++++++++++-
>  include/block/block.h |  1 +
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 8bfe7f4..ad958b9 100644
> --- a/block.c
> +++ b/block.c
> @@ -197,7 +197,7 @@ bool bdrv_is_read_only(BlockDriverState *bs)
>      return bs->read_only;
>  }
>  
> -int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> +int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>  {
>      /* Do not set read_only if copy_on_read is enabled */
>      if (bs->copy_on_read && read_only) {
> @@ -213,6 +213,18 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>          return -EPERM;
>      }
>  
> +    return 0;
> +}
> +
> +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> +{
> +    int ret = 0;
> +
> +    ret = bdrv_try_set_read_only(bs, read_only, errp);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
>      bs->read_only = read_only;
>      return 0;
>  }
> diff --git a/include/block/block.h b/include/block/block.h
> index beb563a..0049b57 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
>                              int64_t sector_num, int nb_sectors, int *pnum);
>  
>  bool bdrv_is_read_only(BlockDriverState *bs);
> +int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
>  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
>  bool bdrv_is_sg(BlockDriverState *bs);
>  bool bdrv_is_inserted(BlockDriverState *bs);
> 

Maybe I'm just green, but I find it weird that the function is named
try_x when we aren't actually trying to do anything, we're checking to
see if we can.

Re: [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only()

[Jeff Cody]

On Wed, Apr 05, 2017 at 04:28:44PM -0400, John Snow wrote:
> 
> 
> On 04/05/2017 02:28 PM, Jeff Cody wrote:
> > Introduce try function for setting read_only flags.  Will return < 0 on
> > error, with appropriate Error value set.  Does not alter any flags.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c               | 14 +++++++++++++-
> >  include/block/block.h |  1 +
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block.c b/block.c
> > index 8bfe7f4..ad958b9 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -197,7 +197,7 @@ bool bdrv_is_read_only(BlockDriverState *bs)
> >      return bs->read_only;
> >  }
> >  
> > -int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> > +int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> >  {
> >      /* Do not set read_only if copy_on_read is enabled */
> >      if (bs->copy_on_read && read_only) {
> > @@ -213,6 +213,18 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> >          return -EPERM;
> >      }
> >  
> > +    return 0;
> > +}
> > +
> > +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> > +{
> > +    int ret = 0;
> > +
> > +    ret = bdrv_try_set_read_only(bs, read_only, errp);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> >      bs->read_only = read_only;
> >      return 0;
> >  }
> > diff --git a/include/block/block.h b/include/block/block.h
> > index beb563a..0049b57 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> >                              int64_t sector_num, int nb_sectors, int *pnum);
> >  
> >  bool bdrv_is_read_only(BlockDriverState *bs);
> > +int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> >  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> >  bool bdrv_is_sg(BlockDriverState *bs);
> >  bool bdrv_is_inserted(BlockDriverState *bs);
> > 
> 
> Maybe I'm just green, but I find it weird that the function is named
> try_x when we aren't actually trying to do anything, we're checking to
> see if we can.

Yeah, not a good name.  Maybe bdrv_set_read_only_test() or
bdrv_test_set_read_only()?

Re: [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]

On Wed, Apr 05, 2017 at 08:23:12PM -0400, Jeff Cody wrote:
> On Wed, Apr 05, 2017 at 04:28:44PM -0400, John Snow wrote:
> > On 04/05/2017 02:28 PM, Jeff Cody wrote:
> > > diff --git a/include/block/block.h b/include/block/block.h
> > > index beb563a..0049b57 100644
> > > --- a/include/block/block.h
> > > +++ b/include/block/block.h
> > > @@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> > >                              int64_t sector_num, int nb_sectors, int *pnum);
> > >  
> > >  bool bdrv_is_read_only(BlockDriverState *bs);
> > > +int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> > >  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> > >  bool bdrv_is_sg(BlockDriverState *bs);
> > >  bool bdrv_is_inserted(BlockDriverState *bs);
> > > 
> > 
> > Maybe I'm just green, but I find it weird that the function is named
> > try_x when we aren't actually trying to do anything, we're checking to
> > see if we can.
> 
> Yeah, not a good name.  Maybe bdrv_set_read_only_test() or
> bdrv_test_set_read_only()?

This one reads nicely:

  if (bdrv_can_set_read_only(...)) {
      ...
  }

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_set_read_only() during reopen

[Jeff Cody]

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index ad958b9..3245fae 100644
--- a/block.c
+++ b/block.c
@@ -2785,6 +2785,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
     BlockDriver *drv;
     QemuOpts *opts;
     const char *value;
+    bool read_only;
 
     assert(reopen_state != NULL);
     assert(reopen_state->bs->drv != NULL);
@@ -2813,12 +2814,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         qdict_put(reopen_state->options, "driver", qstring_from_str(value));
     }
 
-    /* if we are to stay read-only, do not allow permission change
-     * to r/w */
-    if (!(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR) &&
-        reopen_state->flags & BDRV_O_RDWR) {
-        error_setg(errp, "Node '%s' is read only",
-                   bdrv_get_device_or_node_name(reopen_state->bs));
+    /* If we are to stay read-only, do not allow permission change
+     * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
+     * not set, or if the BDS still has copy_on_read enabled */
+    read_only = !(reopen_state->bs->open_flags & BDRV_O_RDWR);
+    ret = bdrv_try_set_read_only(reopen_state->bs, read_only, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
         goto error;
     }
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_set_read_only() during reopen

[John Snow]

On 04/05/2017 02:28 PM, Jeff Cody wrote:
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ad958b9..3245fae 100644
> --- a/block.c
> +++ b/block.c
> @@ -2785,6 +2785,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>      BlockDriver *drv;
>      QemuOpts *opts;
>      const char *value;
> +    bool read_only;
>  
>      assert(reopen_state != NULL);
>      assert(reopen_state->bs->drv != NULL);
> @@ -2813,12 +2814,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>          qdict_put(reopen_state->options, "driver", qstring_from_str(value));
>      }
>  
> -    /* if we are to stay read-only, do not allow permission change
> -     * to r/w */
> -    if (!(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR) &&
> -        reopen_state->flags & BDRV_O_RDWR) {

So the current code checks reopen_state->flags & BDRV_O_RDWR;

> -        error_setg(errp, "Node '%s' is read only",
> -                   bdrv_get_device_or_node_name(reopen_state->bs));
> +    /* If we are to stay read-only, do not allow permission change
> +     * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
> +     * not set, or if the BDS still has copy_on_read enabled */
> +    read_only = !(reopen_state->bs->open_flags & BDRV_O_RDWR);

And the proposed change checks reopen_state->bs->open_flags &
BDRV_O_RDWR. (It's negated again inside of bdrv_try_set_read_only.)

Both check against !(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR).

What's the functional difference of doing so, and is it intentional?

> +    ret = bdrv_try_set_read_only(reopen_state->bs, read_only, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
>          goto error;
>      }
>  
> 

Re: [Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_set_read_only() during reopen

[Jeff Cody]

On Wed, Apr 05, 2017 at 04:25:39PM -0400, John Snow wrote:
> 
> 
> On 04/05/2017 02:28 PM, Jeff Cody wrote:
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index ad958b9..3245fae 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2785,6 +2785,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
> >      BlockDriver *drv;
> >      QemuOpts *opts;
> >      const char *value;
> > +    bool read_only;
> >  
> >      assert(reopen_state != NULL);
> >      assert(reopen_state->bs->drv != NULL);
> > @@ -2813,12 +2814,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
> >          qdict_put(reopen_state->options, "driver", qstring_from_str(value));
> >      }
> >  
> > -    /* if we are to stay read-only, do not allow permission change
> > -     * to r/w */
> > -    if (!(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR) &&
> > -        reopen_state->flags & BDRV_O_RDWR) {
> 
> So the current code checks reopen_state->flags & BDRV_O_RDWR;
> 
> > -        error_setg(errp, "Node '%s' is read only",
> > -                   bdrv_get_device_or_node_name(reopen_state->bs));
> > +    /* If we are to stay read-only, do not allow permission change
> > +     * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
> > +     * not set, or if the BDS still has copy_on_read enabled */
> > +    read_only = !(reopen_state->bs->open_flags & BDRV_O_RDWR);
> 
> And the proposed change checks reopen_state->bs->open_flags &
> BDRV_O_RDWR. (It's negated again inside of bdrv_try_set_read_only.)
> 
> Both check against !(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR).
> 
> What's the functional difference of doing so, and is it intentional?
>

Good catch; unintentional.  That line should read:

+    read_only = !(reopen_state->flags & BDRV_O_RDWR);

The functional difference is we would be testing against the open_flags of
the current BS to see if we are requesting r/w or r/o, rather than checking
against the reopen requested flags.

> > +    ret = bdrv_try_set_read_only(reopen_state->bs, read_only, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> >          goto error;
> >      }
> >  
> > 

[Qemu-devel] [PATCH for-2.10 7/9] block/rbd - update variable names to more apt names

[Jeff Cody]

Update 'clientname' to be 'user', which tracks better with both
the QAPI and rados viarable naming.

Update 'name' to be 'image_name', as it indicates the rbd image.
Naming it 'image' would have been ideal, but we are using that for
the rados_image_t value returned by rbd_open().

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/rbd.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 328e4a9..c5e1aeb 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -94,7 +94,7 @@ typedef struct BDRVRBDState {
     rados_t cluster;
     rados_ioctx_t io_ctx;
     rbd_image_t image;
-    char *name;
+    char *image_name;
     char *snap;
 } BDRVRBDState;
 
@@ -350,7 +350,7 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
     int64_t bytes = 0;
     int64_t objsize;
     int obj_order = 0;
-    const char *pool, *name, *conf, *clientname, *keypairs;
+    const char *pool, *image_name, *conf, *user, *keypairs;
     const char *secretid;
     rados_t cluster;
     rados_ioctx_t io_ctx;
@@ -393,11 +393,11 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
      */
     pool       = qdict_get_try_str(options, "pool");
     conf       = qdict_get_try_str(options, "conf");
-    clientname = qdict_get_try_str(options, "user");
-    name       = qdict_get_try_str(options, "image");
+    user       = qdict_get_try_str(options, "user");
+    image_name = qdict_get_try_str(options, "image");
     keypairs   = qdict_get_try_str(options, "=keyvalue-pairs");
 
-    ret = rados_create(&cluster, clientname);
+    ret = rados_create(&cluster, user);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "error initializing");
         goto exit;
@@ -434,7 +434,7 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
         goto shutdown;
     }
 
-    ret = rbd_create(io_ctx, name, bytes, &obj_order);
+    ret = rbd_create(io_ctx, image_name, bytes, &obj_order);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "error rbd create");
     }
@@ -540,7 +540,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
-    const char *pool, *snap, *conf, *clientname, *name, *keypairs;
+    const char *pool, *snap, *conf, *user, *image_name, *keypairs;
     const char *secretid;
     QemuOpts *opts;
     Error *local_err = NULL;
@@ -567,24 +567,24 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     pool           = qemu_opt_get(opts, "pool");
     conf           = qemu_opt_get(opts, "conf");
     snap           = qemu_opt_get(opts, "snapshot");
-    clientname     = qemu_opt_get(opts, "user");
-    name           = qemu_opt_get(opts, "image");
+    user           = qemu_opt_get(opts, "user");
+    image_name     = qemu_opt_get(opts, "image");
     keypairs       = qemu_opt_get(opts, "=keyvalue-pairs");
 
-    if (!pool || !name) {
+    if (!pool || !image_name) {
         error_setg(errp, "Parameters 'pool' and 'image' are required");
         r = -EINVAL;
         goto failed_opts;
     }
 
-    r = rados_create(&s->cluster, clientname);
+    r = rados_create(&s->cluster, user);
     if (r < 0) {
         error_setg_errno(errp, -r, "error initializing");
         goto failed_opts;
     }
 
     s->snap = g_strdup(snap);
-    s->name = g_strdup(name);
+    s->image_name = g_strdup(image_name);
 
     /* try default location when conf=NULL, but ignore failure */
     r = rados_conf_read_file(s->cluster, conf);
@@ -635,9 +635,10 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_shutdown;
     }
 
-    r = rbd_open(s->io_ctx, s->name, &s->image, s->snap);
+    r = rbd_open(s->io_ctx, s->image_name, &s->image, s->snap);
     if (r < 0) {
-        error_setg_errno(errp, -r, "error reading header from %s", s->name);
+        error_setg_errno(errp, -r, "error reading header from %s",
+                         s->image_name);
         goto failed_open;
     }
 
@@ -655,7 +656,7 @@ failed_open:
 failed_shutdown:
     rados_shutdown(s->cluster);
     g_free(s->snap);
-    g_free(s->name);
+    g_free(s->image_name);
 failed_opts:
     qemu_opts_del(opts);
     g_free(mon_host);
@@ -669,7 +670,7 @@ static void qemu_rbd_close(BlockDriverState *bs)
     rbd_close(s->image);
     rados_ioctx_destroy(s->io_ctx);
     g_free(s->snap);
-    g_free(s->name);
+    g_free(s->image_name);
     rados_shutdown(s->cluster);
 }
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-2.10 7/9] block/rbd - update variable names to more apt names

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 580 bytes --]

On Wed, Apr 05, 2017 at 02:28:49PM -0400, Jeff Cody wrote:
> Update 'clientname' to be 'user', which tracks better with both
> the QAPI and rados viarable naming.
> 
> Update 'name' to be 'image_name', as it indicates the rbd image.
> Naming it 'image' would have been ideal, but we are using that for
> the rados_image_t value returned by rbd_open().
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH for-2.10 8/9] block/rbd: do not blindly set bs->read_only

[Jeff Cody]

We do not want to just blindly set bs->read_only.  The only time
we need to set it with the current rbd driver, is if we are using an
rbd snapshot.  In that case, we must set it to read-only.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/rbd.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index c5e1aeb..35853c9 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -635,6 +635,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_shutdown;
     }
 
+    /* rbd_open is always r/w */
     r = rbd_open(s->io_ctx, s->image_name, &s->image, s->snap);
     if (r < 0) {
         error_setg_errno(errp, -r, "error reading header from %s",
@@ -642,10 +643,14 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto failed_open;
     }
 
-    r = bdrv_set_read_only(bs, (s->snap != NULL), &local_err);
-    if (r < 0) {
-        error_propagate(errp, local_err);
-        goto failed_open;
+    /* 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;
+        }
     }
 
     qemu_opts_del(opts);
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-2.10 8/9] block/rbd: do not blindly set bs->read_only

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1715 bytes --]

On Wed, Apr 05, 2017 at 02:28:50PM -0400, Jeff Cody wrote:
> We do not want to just blindly set bs->read_only.  The only time
> we need to set it with the current rbd driver, is if we are using an
> rbd snapshot.  In that case, we must set it to read-only.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index c5e1aeb..35853c9 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -635,6 +635,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          goto failed_shutdown;
>      }
>  
> +    /* rbd_open is always r/w */
>      r = rbd_open(s->io_ctx, s->image_name, &s->image, s->snap);
>      if (r < 0) {
>          error_setg_errno(errp, -r, "error reading header from %s",
> @@ -642,10 +643,14 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>          goto failed_open;
>      }
>  
> -    r = bdrv_set_read_only(bs, (s->snap != NULL), &local_err);
> -    if (r < 0) {
> -        error_propagate(errp, local_err);
> -        goto failed_open;
> +    /* 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;
> +        }

This patch series breaks git-bisect(1) for -drive
file.driver=rbd,file.read-only=on since bdrv_set_read_only(bs, false,
&local_err) fails.

Please squash this patch where you added the bdrv_set_read_only() errp
argument.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH for-2.10 9/9] block/rbd: Add support for reopen()

[Jeff Cody]

This adds support for reopen in rbd, for changing between r/w and r/o.

Note, that this is only a flag change, but we will block a change from
r/o to r/w if we are using an RBD internal snapshot.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/rbd.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/block/rbd.c b/block/rbd.c
index 35853c9..6471f4f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -668,6 +668,26 @@ failed_opts:
     return r;
 }
 
+
+/* Since RBD is currently always opened R/W via the API,
+ * we just need to check if we are using a snapshot or not, in
+ * order to determine if we will allow it to be R/W */
+static int qemu_rbd_reopen_prepare(BDRVReopenState *state,
+                                   BlockReopenQueue *queue, Error **errp)
+{
+    BDRVRBDState *s = state->bs->opaque;
+    int ret = 0;
+
+    if (s->snap && state->flags & BDRV_O_RDWR) {
+        error_setg(errp,
+                   "Cannot change node '%s' to r/w when using RBD snapshot",
+                   bdrv_get_device_or_node_name(state->bs));
+        ret = -EINVAL;
+    }
+
+    return ret;
+}
+
 static void qemu_rbd_close(BlockDriverState *bs)
 {
     BDRVRBDState *s = bs->opaque;
@@ -1074,6 +1094,7 @@ static BlockDriver bdrv_rbd = {
     .bdrv_parse_filename    = qemu_rbd_parse_filename,
     .bdrv_file_open         = qemu_rbd_open,
     .bdrv_close             = qemu_rbd_close,
+    .bdrv_reopen_prepare    = qemu_rbd_reopen_prepare,
     .bdrv_create            = qemu_rbd_create,
     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
     .bdrv_get_info          = qemu_rbd_getinfo,
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-2.10 9/9] block/rbd: Add support for reopen()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 460 bytes --]

On Wed, Apr 05, 2017 at 02:28:51PM -0400, Jeff Cody wrote:
> This adds support for reopen in rbd, for changing between r/w and r/o.
> 
> Note, that this is only a flag change, but we will block a change from
> r/o to r/w if we are using an RBD internal snapshot.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/rbd.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]