Now that all block drivers with children tell us what permissions they
need from each of their children, bdrv_attach_child() can use this
information and make the right requirements while trying to attach new
children.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 1c5f211..054e6f0 100644
--- a/block.c
+++ b/block.c
@@ -1659,10 +1659,14 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
                              Error **errp)
 {
     BdrvChild *child;
+    uint64_t perm, shared_perm;
+
+    assert(parent_bs->drv);
+    parent_bs->drv->bdrv_child_perm(parent_bs, NULL, child_role,
+                                    0, BLK_PERM_ALL, &perm, &shared_perm);
 
-    /* FIXME Use real permissions */
     child = bdrv_root_attach_child(child_bs, child_name, child_role,
-                                   0, BLK_PERM_ALL, parent_bs, errp);
+                                   perm, shared_perm, parent_bs, errp);
     if (child == NULL) {
         return NULL;
     }
-- 
1.8.3.1

On 21.02.2017 15:58, Kevin Wolf wrote:
> Now that all block drivers with children tell us what permissions they
> need from each of their children, bdrv_attach_child() can use this
> information and make the right requirements while trying to attach new
> children.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Not sure whether it was intended how the assertion in vvfat still passes
(@c == s->qcow because both are NULL...), but it works, so:

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

On 21.02.2017 15:58, Kevin Wolf wrote:
> Now that all block drivers with children tell us what permissions they
> need from each of their children, bdrv_attach_child() can use this
> information and make the right requirements while trying to attach new
> children.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1c5f211..054e6f0 100644
> --- a/block.c
> +++ b/block.c
> @@ -1659,10 +1659,14 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
>                               Error **errp)
>  {
>      BdrvChild *child;
> +    uint64_t perm, shared_perm;
> +
> +    assert(parent_bs->drv);
> +    parent_bs->drv->bdrv_child_perm(parent_bs, NULL, child_role,
> +                                    0, BLK_PERM_ALL, &perm, &shared_perm);

Another Second Thought™: Why do we request no permissions for the new
child here? Seems weird to me. Shouldn't the caller specify the
necessary permissions and what can be shared?

Max

>  
> -    /* FIXME Use real permissions */
>      child = bdrv_root_attach_child(child_bs, child_name, child_role,
> -                                   0, BLK_PERM_ALL, parent_bs, errp);
> +                                   perm, shared_perm, parent_bs, errp);
>      if (child == NULL) {
>          return NULL;
>      }
> 

Am 22.02.2017 um 15:31 hat Max Reitz geschrieben:
> On 21.02.2017 15:58, Kevin Wolf wrote:
> > Now that all block drivers with children tell us what permissions they
> > need from each of their children, bdrv_attach_child() can use this
> > information and make the right requirements while trying to attach new
> > children.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 1c5f211..054e6f0 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1659,10 +1659,14 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
> >                               Error **errp)
> >  {
> >      BdrvChild *child;
> > +    uint64_t perm, shared_perm;
> > +
> > +    assert(parent_bs->drv);
> > +    parent_bs->drv->bdrv_child_perm(parent_bs, NULL, child_role,
> > +                                    0, BLK_PERM_ALL, &perm, &shared_perm);
> 
> Another Second Thought™: Why do we request no permissions for the new
> child here? Seems weird to me. Shouldn't the caller specify the
> necessary permissions and what can be shared?

Actually not the caller, but we should calculate the cumulative
permissions of parent_bs, like in bdrv_update_perm().

Kevin