[Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first

[Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first

[Fam Zheng]

bdrv_child_set_perm alone is not very usable because the caller must
call bdrv_child_check_perm first. This is already encapsulated
conveniently in bdrv_child_try_set_perm, so remove the other prototypes
from the header and fix the one wrong caller, block/mirror.c.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c                   | 13 +++++++++----
 block/mirror.c            |  6 ++++--
 include/block/block_int.h |  4 ----
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index cb57370..a77e8a0 100644
--- a/block.c
+++ b/block.c
@@ -1393,6 +1393,11 @@ static int bdrv_fill_options(QDict **options, const char *filename,
     return 0;
 }
 
+static int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
+                                 GSList *ignore_children, Error **errp);
+static void bdrv_child_abort_perm_update(BdrvChild *c);
+static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
+
 /*
  * Check whether permissions on this node can be changed in a way that
  * @cumulative_perms and @cumulative_shared_perms are the new cumulative
@@ -1615,8 +1620,8 @@ static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t new_used_perm,
 
 /* Needs to be followed by a call to either bdrv_child_set_perm() or
  * bdrv_child_abort_perm_update(). */
-int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
-                          GSList *ignore_children, Error **errp)
+static int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
+                                 GSList *ignore_children, Error **errp)
 {
     int ret;
 
@@ -1627,7 +1632,7 @@ int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
     return ret;
 }
 
-void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
+static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
 {
     uint64_t cumulative_perms, cumulative_shared_perms;
 
@@ -1639,7 +1644,7 @@ void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
     bdrv_set_perm(c->bs, cumulative_perms, cumulative_shared_perms);
 }
 
-void bdrv_child_abort_perm_update(BdrvChild *c)
+static void bdrv_child_abort_perm_update(BdrvChild *c)
 {
     bdrv_abort_perm_update(c->bs);
 }
diff --git a/block/mirror.c b/block/mirror.c
index 4f3a5cb..ca4baa5 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -574,7 +574,8 @@ static void mirror_exit(BlockJob *job, void *opaque)
      * valid. Also give up permissions on mirror_top_bs->backing, which might
      * block the removal. */
     block_job_remove_all_bdrv(job);
-    bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);
+    bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
+                            &error_abort);
     bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 
     /* We just changed the BDS the job BB refers to (with either or both of the
@@ -1245,7 +1246,8 @@ fail:
         block_job_unref(&s->common);
     }
 
-    bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);
+    bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
+                            &error_abort);
     bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6c699ac..59400bd 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -889,10 +889,6 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
                                   void *opaque, Error **errp);
 void bdrv_root_unref_child(BdrvChild *child);
 
-int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
-                          GSList *ignore_children, Error **errp);
-void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
-void bdrv_child_abort_perm_update(BdrvChild *c);
 int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
                             Error **errp);
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first

[Eric Blake]

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

On 03/13/2017 09:30 PM, Fam Zheng wrote:
> bdrv_child_set_perm alone is not very usable because the caller must
> call bdrv_child_check_perm first. This is already encapsulated
> conveniently in bdrv_child_try_set_perm, so remove the other prototypes
> from the header and fix the one wrong caller, block/mirror.c.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c                   | 13 +++++++++----
>  block/mirror.c            |  6 ++++--
>  include/block/block_int.h |  4 ----
>  3 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/block.c b/block.c
> index cb57370..a77e8a0 100644
> --- a/block.c
> +++ b/block.c
> @@ -1393,6 +1393,11 @@ static int bdrv_fill_options(QDict **options, const char *filename,
>      return 0;
>  }
>  
> +static int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
> +                                 GSList *ignore_children, Error **errp);
> +static void bdrv_child_abort_perm_update(BdrvChild *c);
> +static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
> +

Now that you have static prototypes, is it worth rearranging the file
(in a followup) to sort the function implementations into topological
order so that a prototype is not necessary?  [In general, I try to code
so that static prototypes are only necessary if I am implementing
mutually-referencing recursive code. But it's not a strict requirement]

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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] block: Always call bdrv_child_check_perm first

[Fam Zheng]

On Tue, 03/14 08:28, Eric Blake wrote:
> On 03/13/2017 09:30 PM, Fam Zheng wrote:
> > bdrv_child_set_perm alone is not very usable because the caller must
> > call bdrv_child_check_perm first. This is already encapsulated
> > conveniently in bdrv_child_try_set_perm, so remove the other prototypes
> > from the header and fix the one wrong caller, block/mirror.c.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block.c                   | 13 +++++++++----
> >  block/mirror.c            |  6 ++++--
> >  include/block/block_int.h |  4 ----
> >  3 files changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index cb57370..a77e8a0 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1393,6 +1393,11 @@ static int bdrv_fill_options(QDict **options, const char *filename,
> >      return 0;
> >  }
> >  
> > +static int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
> > +                                 GSList *ignore_children, Error **errp);
> > +static void bdrv_child_abort_perm_update(BdrvChild *c);
> > +static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
> > +
> 
> Now that you have static prototypes, is it worth rearranging the file
> (in a followup) to sort the function implementations into topological
> order so that a prototype is not necessary?  [In general, I try to code
> so that static prototypes are only necessary if I am implementing
> mutually-referencing recursive code. But it's not a strict requirement]

Yes, thanks for pointing out, but it does have a recursion here:

bdrv_check_update_perm
    -> bdrv_check_perm
        -> bdrv_child_check_perm
            -> bdrv_check_update_perm

Fam

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

Re: [Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first

[Kevin Wolf]

Am 14.03.2017 um 03:30 hat Fam Zheng geschrieben:
> bdrv_child_set_perm alone is not very usable because the caller must
> call bdrv_child_check_perm first.

Well, you can imagine use cases where you want to check multiple
children first and then set or abort all of them, but apparently we
haven't found such a case yet, so I'm fine with making the functions
private for now.  If we ever need it, making them public again is
trivial.

> This is already encapsulated
> conveniently in bdrv_child_try_set_perm, so remove the other prototypes
> from the header and fix the one wrong caller, block/mirror.c.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first

[Fam Zheng]

On Wed, 03/15 11:52, Kevin Wolf wrote:
> Am 14.03.2017 um 03:30 hat Fam Zheng geschrieben:
> > bdrv_child_set_perm alone is not very usable because the caller must
> > call bdrv_child_check_perm first.
> 
> Well, you can imagine use cases where you want to check multiple
> children first and then set or abort all of them, but apparently we
> haven't found such a case yet, so I'm fine with making the functions
> private for now.  If we ever need it, making them public again is
> trivial.

Yes, no problem with that use case but by then I suppose we should also add an
assertion about the calling sequence: e.g. in image locking, raw_set_perm goes
nut if not preceded by `raw_check_perm.

> 
> > This is already encapsulated
> > conveniently in bdrv_child_try_set_perm, so remove the other prototypes
> > from the header and fix the one wrong caller, block/mirror.c.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> Thanks, applied to the block branch.

Thanks!

Fam

> 
> Kevin

Re: [Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first

[Eric Blake]

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

On 03/14/2017 10:06 PM, Fam Zheng wrote:

>>> +static int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
>>> +                                 GSList *ignore_children, Error **errp);
>>> +static void bdrv_child_abort_perm_update(BdrvChild *c);
>>> +static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
>>> +
>>
>> Now that you have static prototypes, is it worth rearranging the file
>> (in a followup) to sort the function implementations into topological
>> order so that a prototype is not necessary?  [In general, I try to code
>> so that static prototypes are only necessary if I am implementing
>> mutually-referencing recursive code. But it's not a strict requirement]
> 
> Yes, thanks for pointing out, but it does have a recursion here:
> 
> bdrv_check_update_perm
>     -> bdrv_check_perm
>         -> bdrv_child_check_perm
>             -> bdrv_check_update_perm
> 

So you're right, topological sorting is not possible.  Carry on, nothing
to see here :)

-- 
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] block: Always call bdrv_child_check_perm first

[Kevin Wolf]

Am 15.03.2017 um 12:09 hat Fam Zheng geschrieben:
> On Wed, 03/15 11:52, Kevin Wolf wrote:
> > Am 14.03.2017 um 03:30 hat Fam Zheng geschrieben:
> > > bdrv_child_set_perm alone is not very usable because the caller must
> > > call bdrv_child_check_perm first.
> > 
> > Well, you can imagine use cases where you want to check multiple
> > children first and then set or abort all of them, but apparently we
> > haven't found such a case yet, so I'm fine with making the functions
> > private for now.  If we ever need it, making them public again is
> > trivial.
> 
> Yes, no problem with that use case but by then I suppose we should also add an
> assertion about the calling sequence: e.g. in image locking, raw_set_perm goes
> nut if not preceded by `raw_check_perm.

Yes, that makes sense.

Kevin