Re: [PATCH v2] block: drop BLK_PERM_GRAPH_MOD

[Eric Blake <eblake@redhat.com>]

On Thu, Sep 02, 2021 at 12:37:54PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> First, this permission never protected a node from being changed, as
> generic child-replacing functions don't check it.
> 
> Second, it's a strange thing: it presents a permission of parent node
> to change its child. But generally, children are replaced by different
> mechanisms, like jobs or qmp commands, not by nodes.
> 
> Graph-mod permission is hard to understand. All other permissions
> describe operations which done by parent node on its child: read,
> write, resize. Graph modification operations are something completely
> different.
> 
> The only place where BLK_PERM_GRAPH_MOD is used as "perm" (not shared
> perm) is mirror_start_job, for s->target. Still modern code should use
> bdrv_freeze_backing_chain() to protect from graph modification, if we
> don't do it somewhere it may be considered as a bug. So, it's a bit
> risky to drop GRAPH_MOD, and analyzing of possible loss of protection
> is hard. But one day we should do it, let's do it now.
> 
> One more bit of information is that locking the corresponding byte in
> file-posix doesn't make sense at all.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> v2: fix grammar
> 
>  qapi/block-core.json          |  7 ++-----
>  include/block/block.h         |  9 +++++----
>  block.c                       |  7 +------
>  block/commit.c                |  1 -
>  block/mirror.c                | 15 +++------------
>  hw/block/block.c              |  3 +--
>  scripts/render_block_graph.py |  1 -
>  tests/qemu-iotests/273.out    |  4 ----
>  8 files changed, 12 insertions(+), 35 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 06674c25c9..6fa2c4ab82 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1825,14 +1825,11 @@
>  #
>  # @resize: This permission is required to change the size of a block node.
>  #
> -# @graph-mod: This permission is required to change the node that this
> -#             BdrvChild points to.
> -#

Do we need to mention that graph-mod was removed in 6.2?

>  # Since: 4.0
>  ##
>  { 'enum': 'BlockPermission',
> -  'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize',
> -            'graph-mod' ] }
> +  'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize' ] }
> +
>  ##
>  # @XDbgBlockGraphEdge:
>  #

Otherwise the patch makes sense to me, but I'd rather that Kevin chime in.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org