On Mon, Sep 15, 2014 at 04:21:18PM +0200, Benoît Canet wrote: > We do not want to try to stream or commit with a base argument through > a multiple children driver. > > Handle this case. > > Benoît Canet (2): > block: Introduce a BlockDriver field to flag drivers supporting > multiple children > block: commit and stream return an error when a subtree is found > > block.c | 17 +++++++++++++++-- > block/blkverify.c | 1 + > block/quorum.c | 1 + > block/vmdk.c | 1 + > blockdev.c | 18 ++++++++++++------ > include/block/block.h | 3 ++- > include/block/block_int.h | 6 ++++++ > 7 files changed, 38 insertions(+), 9 deletions(-) > > -- > 2.1.0 >
I had some specific comments on the implementation, but once I stepped back some I am not sure this series is needed. When we talked on irc, you mentioned it was needed because your recursive blocker code would assert in quorum/blkverify/vmdk - but I think that might mean the recursive code needs refactoring. Here is why I am not sure this is needed: we should be able to commit through/to an image that has multiple child nodes, if we can treat that image as an actual BDS. I view these as a black box, like so: (imageE) ------------ | [childA] | | [childB] | [base] <---- | | <----- [imageF] <--- [active] | [childC] | ^ | [childD] | | ------------ | | (Perhaps base isn't support by the format, in which case it will always be NULL anyway) ImageE may have multiple children (and perhaps each of those children are an entire chain themselves), but ultimately imageE is a 'BDS' entry. Perhaps the format, like quorum, will not have a backing_hd pointer. But in that case, backing_hd will be NULL. Thinking about your recursive blocker code, why assert when the 'base' termination argument is NULL? If the multi-child format does not support a backing_hd, then bdrv_find_base_image() will just find it as the end. The .bdrv_op_recursive_block() in that case could still navigate down each private child node, because if the original 'base' blocker applied to the multi-child parent (imageE), then that same blocker should apply to each private child (and their children), regardless of 'base' - because they are essentially part of the pseudo-atomic entity of (imageE). Does this make sense, or am I missing a piece? Thanks, Jeff