| Message ID | 20210504094510.25032-6-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block permission updated follow-up | expand |
Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben: > Now permissions are updated as follows: > 1. do graph modifications ignoring permissions > 2. do permission update > > (of course, we rollback [1] if [2] fails) > > So, on stage [2] we can't say which users are "old" and which are > "new" and exist only since [1]. And current error message is a bit > outdated. Let's improve it, to make everything clean. > > While being here, add also a comment and some good assertions. > > iotests 283, 307, qsd-jobs outputs are updated. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block.c | 29 ++++++++++++++++++++------- > tests/qemu-iotests/283.out | 2 +- > tests/qemu-iotests/307.out | 2 +- > tests/qemu-iotests/tests/qsd-jobs.out | 2 +- > 4 files changed, 25 insertions(+), 10 deletions(-) > > diff --git a/block.c b/block.c > index 2f73523285..354438d918 100644 > --- a/block.c > +++ b/block.c > @@ -2032,20 +2032,35 @@ static char *bdrv_child_user_desc(BdrvChild *c) > return c->klass->get_parent_desc(c); > } > > +/* > + * Check that @a allows everything that @b needs. @a and @b must reference same > + * child node. > + */ > static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp) > { > - g_autofree char *user = NULL; > - g_autofree char *perm_names = NULL; > + g_autofree char *a_user = NULL; > + g_autofree char *a_against = NULL; > + g_autofree char *b_user = NULL; > + g_autofree char *b_perm = NULL; > + > + assert(a->bs); > + assert(a->bs == b->bs); > > if ((b->perm & a->shared_perm) == b->perm) { > return true; > } > > - perm_names = bdrv_perm_names(b->perm & ~a->shared_perm); > - user = bdrv_child_user_desc(a); > - error_setg(errp, "Conflicts with use by %s as '%s', which does not " > - "allow '%s' on %s", > - user, a->name, perm_names, bdrv_get_node_name(b->bs)); > + a_user = bdrv_child_user_desc(a); > + a_against = bdrv_perm_names(b->perm & ~a->shared_perm); > + > + b_user = bdrv_child_user_desc(b); > + b_perm = bdrv_perm_names(b->perm); > + error_setg(errp, "Permission conflict on node '%s': %s wants to use it as " > + "'%s', which requires these permissions: %s. On the other hand %s " > + "wants to use it as '%s', which doesn't share: %s", > + bdrv_get_node_name(b->bs), > + b_user, b->name, b_perm, a_user, a->name, a_against); I think the combination of a_against and b_perm is confusing to report because one is the intersection of permissions (i.e. only the permissions that actually conflict) and the other the full list of unshared permissions. We could report both the full list of required permissions (which is what your current error message claims to report) and of unshared permissions. I'm not sure if there is actually any use for this information. The other option that would feel consistent is to report only the conflicting permissions, and report them only once because they are the same for both sides. Kevin > > return false; > } > diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out > index c9397bfc44..92f3cc1ed5 100644 > --- a/tests/qemu-iotests/283.out > +++ b/tests/qemu-iotests/283.out > @@ -5,7 +5,7 @@ > {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}} > {"return": {}} > {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}} > -{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Conflicts with use by node 'source' as 'image', which does not allow 'write' on base"}} > +{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Permission conflict on node 'base': node 'other' wants to use it as 'image', which requires these permissions: write. On the other hand node 'source' wants to use it as 'image', which doesn't share: write"}} > > === backup-top should be gone after job-finalize === > > diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out > index 66bf2ddb74..e03932ba4f 100644 > --- a/tests/qemu-iotests/307.out > +++ b/tests/qemu-iotests/307.out > @@ -53,7 +53,7 @@ exports available: 1 > > === Add a writable export === > {"execute": "block-export-add", "arguments": {"description": "This is the writable second export", "id": "export1", "name": "export1", "node-name": "fmt", "type": "nbd", "writable": true, "writethrough": true}} > -{"error": {"class": "GenericError", "desc": "Conflicts with use by block device 'sda' as 'root', which does not allow 'write' on fmt"}} > +{"error": {"class": "GenericError", "desc": "Permission conflict on node 'fmt': unnamed block device wants to use it as 'root', which requires these permissions: consistent read, write. On the other hand block device 'sda' wants to use it as 'root', which doesn't share: write"}} > {"execute": "device_del", "arguments": {"id": "sda"}} > {"return": {}} > {"data": {"device": "sda", "path": "/machine/peripheral/sda"}, "event": "DEVICE_DELETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} > diff --git a/tests/qemu-iotests/tests/qsd-jobs.out b/tests/qemu-iotests/tests/qsd-jobs.out > index 9f52255da8..b0596d2c95 100644 > --- a/tests/qemu-iotests/tests/qsd-jobs.out > +++ b/tests/qemu-iotests/tests/qsd-jobs.out > @@ -16,7 +16,7 @@ QMP_VERSION > {"return": {}} > {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}} > {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}} > -{"error": {"class": "GenericError", "desc": "Conflicts with use by stream job 'job0' as 'intermediate node', which does not allow 'write' on fmt_base"}} > +{"error": {"class": "GenericError", "desc": "Permission conflict on node 'fmt_base': unnamed block device wants to use it as 'root', which requires these permissions: consistent read, write. On the other hand stream job 'job0' wants to use it as 'intermediate node', which doesn't share: write"}} > {"return": {}} > {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "export1"}} > *** done > -- > 2.29.2 >
31.05.2021 19:07, Kevin Wolf wrote: > Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben: >> Now permissions are updated as follows: >> 1. do graph modifications ignoring permissions >> 2. do permission update >> >> (of course, we rollback [1] if [2] fails) >> >> So, on stage [2] we can't say which users are "old" and which are >> "new" and exist only since [1]. And current error message is a bit >> outdated. Let's improve it, to make everything clean. >> >> While being here, add also a comment and some good assertions. >> >> iotests 283, 307, qsd-jobs outputs are updated. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block.c | 29 ++++++++++++++++++++------- >> tests/qemu-iotests/283.out | 2 +- >> tests/qemu-iotests/307.out | 2 +- >> tests/qemu-iotests/tests/qsd-jobs.out | 2 +- >> 4 files changed, 25 insertions(+), 10 deletions(-) >> >> diff --git a/block.c b/block.c >> index 2f73523285..354438d918 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2032,20 +2032,35 @@ static char *bdrv_child_user_desc(BdrvChild *c) >> return c->klass->get_parent_desc(c); >> } >> >> +/* >> + * Check that @a allows everything that @b needs. @a and @b must reference same >> + * child node. >> + */ >> static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp) >> { >> - g_autofree char *user = NULL; >> - g_autofree char *perm_names = NULL; >> + g_autofree char *a_user = NULL; >> + g_autofree char *a_against = NULL; >> + g_autofree char *b_user = NULL; >> + g_autofree char *b_perm = NULL; >> + >> + assert(a->bs); >> + assert(a->bs == b->bs); >> >> if ((b->perm & a->shared_perm) == b->perm) { >> return true; >> } >> >> - perm_names = bdrv_perm_names(b->perm & ~a->shared_perm); >> - user = bdrv_child_user_desc(a); >> - error_setg(errp, "Conflicts with use by %s as '%s', which does not " >> - "allow '%s' on %s", >> - user, a->name, perm_names, bdrv_get_node_name(b->bs)); >> + a_user = bdrv_child_user_desc(a); >> + a_against = bdrv_perm_names(b->perm & ~a->shared_perm); >> + >> + b_user = bdrv_child_user_desc(b); >> + b_perm = bdrv_perm_names(b->perm); >> + error_setg(errp, "Permission conflict on node '%s': %s wants to use it as " >> + "'%s', which requires these permissions: %s. On the other hand %s " >> + "wants to use it as '%s', which doesn't share: %s", >> + bdrv_get_node_name(b->bs), >> + b_user, b->name, b_perm, a_user, a->name, a_against); > > I think the combination of a_against and b_perm is confusing to report > because one is the intersection of permissions (i.e. only the > permissions that actually conflict) and the other the full list of > unshared permissions. > > We could report both the full list of required permissions (which is > what your current error message claims to report) and of unshared > permissions. I'm not sure if there is actually any use for this > information. > > The other option that would feel consistent is to report only the > conflicting permissions, and report them only once because they are the > same for both sides. > Agreed. So, what about: error_setg(errp, "Permission conflict on node '%s": permissions %s are both required by %s (%s) and unshared by %s (%s).", bdrv_get_node_name(b->bs), a_against, b_user, b->name, a_user, a->name); ? > >> >> return false; >> } >> diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out >> index c9397bfc44..92f3cc1ed5 100644 >> --- a/tests/qemu-iotests/283.out >> +++ b/tests/qemu-iotests/283.out >> @@ -5,7 +5,7 @@ >> {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}} >> {"return": {}} >> {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}} >> -{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Conflicts with use by node 'source' as 'image', which does not allow 'write' on base"}} >> +{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Permission conflict on node 'base': node 'other' wants to use it as 'image', which requires these permissions: write. On the other hand node 'source' wants to use it as 'image', which doesn't share: write"}} >> >> === backup-top should be gone after job-finalize === >> >> diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out >> index 66bf2ddb74..e03932ba4f 100644 >> --- a/tests/qemu-iotests/307.out >> +++ b/tests/qemu-iotests/307.out >> @@ -53,7 +53,7 @@ exports available: 1 >> >> === Add a writable export === >> {"execute": "block-export-add", "arguments": {"description": "This is the writable second export", "id": "export1", "name": "export1", "node-name": "fmt", "type": "nbd", "writable": true, "writethrough": true}} >> -{"error": {"class": "GenericError", "desc": "Conflicts with use by block device 'sda' as 'root', which does not allow 'write' on fmt"}} >> +{"error": {"class": "GenericError", "desc": "Permission conflict on node 'fmt': unnamed block device wants to use it as 'root', which requires these permissions: consistent read, write. On the other hand block device 'sda' wants to use it as 'root', which doesn't share: write"}} >> {"execute": "device_del", "arguments": {"id": "sda"}} >> {"return": {}} >> {"data": {"device": "sda", "path": "/machine/peripheral/sda"}, "event": "DEVICE_DELETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} >> diff --git a/tests/qemu-iotests/tests/qsd-jobs.out b/tests/qemu-iotests/tests/qsd-jobs.out >> index 9f52255da8..b0596d2c95 100644 >> --- a/tests/qemu-iotests/tests/qsd-jobs.out >> +++ b/tests/qemu-iotests/tests/qsd-jobs.out >> @@ -16,7 +16,7 @@ QMP_VERSION >> {"return": {}} >> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}} >> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}} >> -{"error": {"class": "GenericError", "desc": "Conflicts with use by stream job 'job0' as 'intermediate node', which does not allow 'write' on fmt_base"}} >> +{"error": {"class": "GenericError", "desc": "Permission conflict on node 'fmt_base': unnamed block device wants to use it as 'root', which requires these permissions: consistent read, write. On the other hand stream job 'job0' wants to use it as 'intermediate node', which doesn't share: write"}} >> {"return": {}} >> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "export1"}} >> *** done >> -- >> 2.29.2 >> >
Am 31.05.2021 um 18:18 hat Vladimir Sementsov-Ogievskiy geschrieben: > 31.05.2021 19:07, Kevin Wolf wrote: > > Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > Now permissions are updated as follows: > > > 1. do graph modifications ignoring permissions > > > 2. do permission update > > > > > > (of course, we rollback [1] if [2] fails) > > > > > > So, on stage [2] we can't say which users are "old" and which are > > > "new" and exist only since [1]. And current error message is a bit > > > outdated. Let's improve it, to make everything clean. > > > > > > While being here, add also a comment and some good assertions. > > > > > > iotests 283, 307, qsd-jobs outputs are updated. > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > > --- > > > block.c | 29 ++++++++++++++++++++------- > > > tests/qemu-iotests/283.out | 2 +- > > > tests/qemu-iotests/307.out | 2 +- > > > tests/qemu-iotests/tests/qsd-jobs.out | 2 +- > > > 4 files changed, 25 insertions(+), 10 deletions(-) > > > > > > diff --git a/block.c b/block.c > > > index 2f73523285..354438d918 100644 > > > --- a/block.c > > > +++ b/block.c > > > @@ -2032,20 +2032,35 @@ static char *bdrv_child_user_desc(BdrvChild *c) > > > return c->klass->get_parent_desc(c); > > > } > > > +/* > > > + * Check that @a allows everything that @b needs. @a and @b must reference same > > > + * child node. > > > + */ > > > static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp) > > > { > > > - g_autofree char *user = NULL; > > > - g_autofree char *perm_names = NULL; > > > + g_autofree char *a_user = NULL; > > > + g_autofree char *a_against = NULL; > > > + g_autofree char *b_user = NULL; > > > + g_autofree char *b_perm = NULL; > > > + > > > + assert(a->bs); > > > + assert(a->bs == b->bs); > > > if ((b->perm & a->shared_perm) == b->perm) { > > > return true; > > > } > > > - perm_names = bdrv_perm_names(b->perm & ~a->shared_perm); > > > - user = bdrv_child_user_desc(a); > > > - error_setg(errp, "Conflicts with use by %s as '%s', which does not " > > > - "allow '%s' on %s", > > > - user, a->name, perm_names, bdrv_get_node_name(b->bs)); > > > + a_user = bdrv_child_user_desc(a); > > > + a_against = bdrv_perm_names(b->perm & ~a->shared_perm); > > > + > > > + b_user = bdrv_child_user_desc(b); > > > + b_perm = bdrv_perm_names(b->perm); > > > + error_setg(errp, "Permission conflict on node '%s': %s wants to use it as " > > > + "'%s', which requires these permissions: %s. On the other hand %s " > > > + "wants to use it as '%s', which doesn't share: %s", > > > + bdrv_get_node_name(b->bs), > > > + b_user, b->name, b_perm, a_user, a->name, a_against); > > > > I think the combination of a_against and b_perm is confusing to report > > because one is the intersection of permissions (i.e. only the > > permissions that actually conflict) and the other the full list of > > unshared permissions. > > > > We could report both the full list of required permissions (which is > > what your current error message claims to report) and of unshared > > permissions. I'm not sure if there is actually any use for this > > information. > > > > The other option that would feel consistent is to report only the > > conflicting permissions, and report them only once because they are the > > same for both sides. > > > > Agreed. > > So, what about: > > error_setg(errp, "Permission conflict on node '%s": permissions %s are both required by %s (%s) and unshared by %s (%s).", bdrv_get_node_name(b->bs), a_against, b_user, b->name, a_user, a->name); I'm not sure if I'm happy with the child names simply in parentheses, but I don't have a good alternative. I was thinking something like "(node used as %s)", but while writing down the example below, that turned out confusing because a_user and b_user can refer to nodes, too. "permissions '%s'" with single quotes might be preferable, too. So a real error message from the current version of the patch is: Permission conflict on node 'base': node 'other' wants to use it as 'image', which requires these permissions: write. On the other hand node 'source' wants to use it as 'image', which doesn't share: write It would then become: Permission conflict on node 'base': permissions 'write' are both required by node 'other' (image) and unshared by 'source' (image). Looks like an improvement to me, but if anyone has a good idea what to do about the unclear meaning of the parentheses, I would be happy to hear suggestions. Kevin > > > return false; > > > } > > > diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out > > > index c9397bfc44..92f3cc1ed5 100644 > > > --- a/tests/qemu-iotests/283.out > > > +++ b/tests/qemu-iotests/283.out > > > @@ -5,7 +5,7 @@ > > > {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}} > > > {"return": {}} > > > {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}} > > > -{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Conflicts with use by node 'source' as 'image', which does not allow 'write' on base"}} > > > +{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Permission conflict on node 'base': node 'other' wants to use it as 'image', which requires these permissions: write. On the other hand node 'source' wants to use it as 'image', which doesn't share: write"}} > > > === backup-top should be gone after job-finalize === > > > diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out > > > index 66bf2ddb74..e03932ba4f 100644 > > > --- a/tests/qemu-iotests/307.out > > > +++ b/tests/qemu-iotests/307.out > > > @@ -53,7 +53,7 @@ exports available: 1 > > > === Add a writable export === > > > {"execute": "block-export-add", "arguments": {"description": "This is the writable second export", "id": "export1", "name": "export1", "node-name": "fmt", "type": "nbd", "writable": true, "writethrough": true}} > > > -{"error": {"class": "GenericError", "desc": "Conflicts with use by block device 'sda' as 'root', which does not allow 'write' on fmt"}} > > > +{"error": {"class": "GenericError", "desc": "Permission conflict on node 'fmt': unnamed block device wants to use it as 'root', which requires these permissions: consistent read, write. On the other hand block device 'sda' wants to use it as 'root', which doesn't share: write"}} > > > {"execute": "device_del", "arguments": {"id": "sda"}} > > > {"return": {}} > > > {"data": {"device": "sda", "path": "/machine/peripheral/sda"}, "event": "DEVICE_DELETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} > > > diff --git a/tests/qemu-iotests/tests/qsd-jobs.out b/tests/qemu-iotests/tests/qsd-jobs.out > > > index 9f52255da8..b0596d2c95 100644 > > > --- a/tests/qemu-iotests/tests/qsd-jobs.out > > > +++ b/tests/qemu-iotests/tests/qsd-jobs.out > > > @@ -16,7 +16,7 @@ QMP_VERSION > > > {"return": {}} > > > {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}} > > > {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}} > > > -{"error": {"class": "GenericError", "desc": "Conflicts with use by stream job 'job0' as 'intermediate node', which does not allow 'write' on fmt_base"}} > > > +{"error": {"class": "GenericError", "desc": "Permission conflict on node 'fmt_base': unnamed block device wants to use it as 'root', which requires these permissions: consistent read, write. On the other hand stream job 'job0' wants to use it as 'intermediate node', which doesn't share: write"}} > > > {"return": {}} > > > {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "export1"}} > > > *** done > > > -- > > > 2.29.2 > > > > > > > > -- > Best regards, > Vladimir >
31.05.2021 19:35, Kevin Wolf wrote: > Am 31.05.2021 um 18:18 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 31.05.2021 19:07, Kevin Wolf wrote: >>> Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben: >>>> Now permissions are updated as follows: >>>> 1. do graph modifications ignoring permissions >>>> 2. do permission update >>>> >>>> (of course, we rollback [1] if [2] fails) >>>> >>>> So, on stage [2] we can't say which users are "old" and which are >>>> "new" and exist only since [1]. And current error message is a bit >>>> outdated. Let's improve it, to make everything clean. >>>> >>>> While being here, add also a comment and some good assertions. >>>> >>>> iotests 283, 307, qsd-jobs outputs are updated. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> block.c | 29 ++++++++++++++++++++------- >>>> tests/qemu-iotests/283.out | 2 +- >>>> tests/qemu-iotests/307.out | 2 +- >>>> tests/qemu-iotests/tests/qsd-jobs.out | 2 +- >>>> 4 files changed, 25 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/block.c b/block.c >>>> index 2f73523285..354438d918 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -2032,20 +2032,35 @@ static char *bdrv_child_user_desc(BdrvChild *c) >>>> return c->klass->get_parent_desc(c); >>>> } >>>> +/* >>>> + * Check that @a allows everything that @b needs. @a and @b must reference same >>>> + * child node. >>>> + */ >>>> static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp) >>>> { >>>> - g_autofree char *user = NULL; >>>> - g_autofree char *perm_names = NULL; >>>> + g_autofree char *a_user = NULL; >>>> + g_autofree char *a_against = NULL; >>>> + g_autofree char *b_user = NULL; >>>> + g_autofree char *b_perm = NULL; >>>> + >>>> + assert(a->bs); >>>> + assert(a->bs == b->bs); >>>> if ((b->perm & a->shared_perm) == b->perm) { >>>> return true; >>>> } >>>> - perm_names = bdrv_perm_names(b->perm & ~a->shared_perm); >>>> - user = bdrv_child_user_desc(a); >>>> - error_setg(errp, "Conflicts with use by %s as '%s', which does not " >>>> - "allow '%s' on %s", >>>> - user, a->name, perm_names, bdrv_get_node_name(b->bs)); >>>> + a_user = bdrv_child_user_desc(a); >>>> + a_against = bdrv_perm_names(b->perm & ~a->shared_perm); >>>> + >>>> + b_user = bdrv_child_user_desc(b); >>>> + b_perm = bdrv_perm_names(b->perm); >>>> + error_setg(errp, "Permission conflict on node '%s': %s wants to use it as " >>>> + "'%s', which requires these permissions: %s. On the other hand %s " >>>> + "wants to use it as '%s', which doesn't share: %s", >>>> + bdrv_get_node_name(b->bs), >>>> + b_user, b->name, b_perm, a_user, a->name, a_against); >>> >>> I think the combination of a_against and b_perm is confusing to report >>> because one is the intersection of permissions (i.e. only the >>> permissions that actually conflict) and the other the full list of >>> unshared permissions. >>> >>> We could report both the full list of required permissions (which is >>> what your current error message claims to report) and of unshared >>> permissions. I'm not sure if there is actually any use for this >>> information. >>> >>> The other option that would feel consistent is to report only the >>> conflicting permissions, and report them only once because they are the >>> same for both sides. >>> >> >> Agreed. >> >> So, what about: >> >> error_setg(errp, "Permission conflict on node '%s": permissions %s are both required by %s (%s) and unshared by %s (%s).", bdrv_get_node_name(b->bs), a_against, b_user, b->name, a_user, a->name); > > I'm not sure if I'm happy with the child names simply in parentheses, > but I don't have a good alternative. I was thinking something like > "(node used as %s)", but while writing down the example below, that > turned out confusing because a_user and b_user can refer to nodes, too. > > "permissions '%s'" with single quotes might be preferable, too. > > So a real error message from the current version of the patch is: > > Permission conflict on node 'base': node 'other' wants to use it as > 'image', which requires these permissions: write. On the other hand > node 'source' wants to use it as 'image', which doesn't share: write > > It would then become: > > Permission conflict on node 'base': permissions 'write' are both > required by node 'other' (image) and unshared by 'source' (image). > > Looks like an improvement to me, but if anyone has a good idea what to > do about the unclear meaning of the parentheses, I would be happy to > hear suggestions. > The only idea I have is duplicating (hmm, "triplicating" is an existing word?) the node of conflict: bs_n = bdrv_get_node_name(b->bs); error_setg(errp, "Permission conflict on node '%s": permissions %s are both required by %s (uses node '%s' as '%s' child) and unshared by %s (uses node '%s' as '%s' child).", bs_n, a_against, b_user, bs_n, b->name, a_user, bs_n, a->name);
diff --git a/block.c b/block.c index 2f73523285..354438d918 100644 --- a/block.c +++ b/block.c @@ -2032,20 +2032,35 @@ static char *bdrv_child_user_desc(BdrvChild *c) return c->klass->get_parent_desc(c); } +/* + * Check that @a allows everything that @b needs. @a and @b must reference same + * child node. + */ static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp) { - g_autofree char *user = NULL; - g_autofree char *perm_names = NULL; + g_autofree char *a_user = NULL; + g_autofree char *a_against = NULL; + g_autofree char *b_user = NULL; + g_autofree char *b_perm = NULL; + + assert(a->bs); + assert(a->bs == b->bs); if ((b->perm & a->shared_perm) == b->perm) { return true; } - perm_names = bdrv_perm_names(b->perm & ~a->shared_perm); - user = bdrv_child_user_desc(a); - error_setg(errp, "Conflicts with use by %s as '%s', which does not " - "allow '%s' on %s", - user, a->name, perm_names, bdrv_get_node_name(b->bs)); + a_user = bdrv_child_user_desc(a); + a_against = bdrv_perm_names(b->perm & ~a->shared_perm); + + b_user = bdrv_child_user_desc(b); + b_perm = bdrv_perm_names(b->perm); + + error_setg(errp, "Permission conflict on node '%s': %s wants to use it as " + "'%s', which requires these permissions: %s. On the other hand %s " + "wants to use it as '%s', which doesn't share: %s", + bdrv_get_node_name(b->bs), + b_user, b->name, b_perm, a_user, a->name, a_against); return false; } diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out index c9397bfc44..92f3cc1ed5 100644 --- a/tests/qemu-iotests/283.out +++ b/tests/qemu-iotests/283.out @@ -5,7 +5,7 @@ {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}} {"return": {}} {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}} -{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Conflicts with use by node 'source' as 'image', which does not allow 'write' on base"}} +{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Permission conflict on node 'base': node 'other' wants to use it as 'image', which requires these permissions: write. On the other hand node 'source' wants to use it as 'image', which doesn't share: write"}} === backup-top should be gone after job-finalize === diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out index 66bf2ddb74..e03932ba4f 100644 --- a/tests/qemu-iotests/307.out +++ b/tests/qemu-iotests/307.out @@ -53,7 +53,7 @@ exports available: 1 === Add a writable export === {"execute": "block-export-add", "arguments": {"description": "This is the writable second export", "id": "export1", "name": "export1", "node-name": "fmt", "type": "nbd", "writable": true, "writethrough": true}} -{"error": {"class": "GenericError", "desc": "Conflicts with use by block device 'sda' as 'root', which does not allow 'write' on fmt"}} +{"error": {"class": "GenericError", "desc": "Permission conflict on node 'fmt': unnamed block device wants to use it as 'root', which requires these permissions: consistent read, write. On the other hand block device 'sda' wants to use it as 'root', which doesn't share: write"}} {"execute": "device_del", "arguments": {"id": "sda"}} {"return": {}} {"data": {"device": "sda", "path": "/machine/peripheral/sda"}, "event": "DEVICE_DELETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} diff --git a/tests/qemu-iotests/tests/qsd-jobs.out b/tests/qemu-iotests/tests/qsd-jobs.out index 9f52255da8..b0596d2c95 100644 --- a/tests/qemu-iotests/tests/qsd-jobs.out +++ b/tests/qemu-iotests/tests/qsd-jobs.out @@ -16,7 +16,7 @@ QMP_VERSION {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}} -{"error": {"class": "GenericError", "desc": "Conflicts with use by stream job 'job0' as 'intermediate node', which does not allow 'write' on fmt_base"}} +{"error": {"class": "GenericError", "desc": "Permission conflict on node 'fmt_base': unnamed block device wants to use it as 'root', which requires these permissions: consistent read, write. On the other hand stream job 'job0' wants to use it as 'intermediate node', which doesn't share: write"}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "export1"}} *** done
Now permissions are updated as follows: 1. do graph modifications ignoring permissions 2. do permission update (of course, we rollback [1] if [2] fails) So, on stage [2] we can't say which users are "old" and which are "new" and exist only since [1]. And current error message is a bit outdated. Let's improve it, to make everything clean. While being here, add also a comment and some good assertions. iotests 283, 307, qsd-jobs outputs are updated. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block.c | 29 ++++++++++++++++++++------- tests/qemu-iotests/283.out | 2 +- tests/qemu-iotests/307.out | 2 +- tests/qemu-iotests/tests/qsd-jobs.out | 2 +- 4 files changed, 25 insertions(+), 10 deletions(-)