* [Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first
@ 2017-03-14 2:30 Fam Zheng
2017-03-14 13:28 ` Eric Blake
2017-03-15 10:52 ` Kevin Wolf
0 siblings, 2 replies; 7+ messages in thread
From: Fam Zheng @ 2017-03-14 2:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Max Reitz, qemu-block
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first
2017-03-14 2:30 [Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first Fam Zheng
@ 2017-03-14 13:28 ` Eric Blake
2017-03-15 3:06 ` Fam Zheng
2017-03-15 10:52 ` Kevin Wolf
1 sibling, 1 reply; 7+ messages in thread
From: Eric Blake @ 2017-03-14 13:28 UTC (permalink / raw)
To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first
2017-03-14 13:28 ` Eric Blake
@ 2017-03-15 3:06 ` Fam Zheng
2017-03-15 11:47 ` Eric Blake
0 siblings, 1 reply; 7+ messages in thread
From: Fam Zheng @ 2017-03-15 3:06 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Kevin Wolf, qemu-block, Max Reitz
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first
2017-03-15 3:06 ` Fam Zheng
@ 2017-03-15 11:47 ` Eric Blake
0 siblings, 0 replies; 7+ messages in thread
From: Eric Blake @ 2017-03-15 11:47 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel, Kevin Wolf, qemu-block, Max Reitz
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first
2017-03-14 2:30 [Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first Fam Zheng
2017-03-14 13:28 ` Eric Blake
@ 2017-03-15 10:52 ` Kevin Wolf
2017-03-15 11:09 ` Fam Zheng
1 sibling, 1 reply; 7+ messages in thread
From: Kevin Wolf @ 2017-03-15 10:52 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel, Max Reitz, qemu-block
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first
2017-03-15 10:52 ` Kevin Wolf
@ 2017-03-15 11:09 ` Fam Zheng
2017-03-15 12:02 ` Kevin Wolf
0 siblings, 1 reply; 7+ messages in thread
From: Fam Zheng @ 2017-03-15 11:09 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, Max Reitz, qemu-block
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first
2017-03-15 11:09 ` Fam Zheng
@ 2017-03-15 12:02 ` Kevin Wolf
0 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2017-03-15 12:02 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel, Max Reitz, qemu-block
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-15 12:02 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-14 2:30 [Qemu-devel] [PATCH] block: Always call bdrv_child_check_perm first Fam Zheng
2017-03-14 13:28 ` Eric Blake
2017-03-15 3:06 ` Fam Zheng
2017-03-15 11:47 ` Eric Blake
2017-03-15 10:52 ` Kevin Wolf
2017-03-15 11:09 ` Fam Zheng
2017-03-15 12:02 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.