[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 22/33] block: Make backing files child_of_bds children

From: Eric Blake
Subject: Re: [PATCH v2 22/33] block: Make backing files child_of_bds children
Date: Wed, 5 Feb 2020 16:45:09 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1

On 2/4/20 11:08 AM, Max Reitz wrote:
Signed-off-by: Max Reitz <address@hidden>

Another sparse commit message (a recurring theme of this series). The subject line says 'what', and the patch appears to be faithful to that, but if a future bisection lands here, even a one-sentence 'why' would be handy; maybe:

This is part of a larger series of unifying block device relationships via child_of_bds.

to at least hint that searching nearby commits gives a better why.

  block.c                 | 26 ++++++++++++++++++++------
  block/backup-top.c      |  2 +-
  block/vvfat.c           |  3 ++-
  tests/test-bdrv-drain.c | 13 +++++++------
  4 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/block.c b/block.c
index 77755f0c6c..6b705ee23a 100644
--- a/block.c
+++ b/block.c
@@ -2748,6 +2748,20 @@ static bool 
bdrv_inherits_from_recursive(BlockDriverState *child,
      return child != NULL;
+ * Return the BdrvChildRole for @bs's backing child.  bs->backing is
+ * mostly used for COW backing children (role = COW), but also for
+ * filtered children (role = FILTERED | PRIMARY).
+ */
+static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)
+    if (bs->drv && bs->drv->is_filter) {

And here's the first point (that I've spotted at least) in this series where you are definitely returning a non-BdrvChildRole through a return type of BdrvChildRole, rather than me just guessing you might (the integer formed by bitwise-or of two enum values is not itself an enum value). Repeating what I said earlier, the C language is loose enough to allow your usage, and your usage is somewhat better self-documenting than using an unsigned int; but it would not fly in other languages.

So I won't insist you change it, but at least think about it. (And the latter has already happened if you read my paragraph - so can we call that enough thought on the matter? ;)

+    } else {
+        return BDRV_CHILD_COW;
+    }
Reviewed-by: Eric Blake <address@hidden>

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]