On 21.06.2016 11:21, Kevin Wolf wrote: > Signed-off-by: Kevin Wolf > --- > block.c | 7 ++++--- > block/bochs.c | 6 +++--- > block/cloop.c | 8 ++++---- > block/crypto.c | 2 +- > block/dmg.c | 21 +++++++++++---------- > block/io.c | 8 ++++---- > block/parallels.c | 4 ++-- > block/qcow.c | 10 +++++----- > block/qcow2-cache.c | 2 +- > block/qcow2-refcount.c | 12 ++++++------ > block/qcow2-snapshot.c | 12 ++++++------ > block/qcow2.c | 16 ++++++++-------- > block/qed.c | 6 +++--- > block/vhdx-log.c | 8 ++++---- > block/vhdx.c | 38 +++++++++++++++++++++++--------------- > block/vmdk.c | 36 +++++++++++++++++------------------- > block/vpc.c | 8 ++++---- > include/block/block.h | 5 ++--- > 18 files changed, 108 insertions(+), 101 deletions(-) Reviewed-by: Max Reitz But I do take issue with a comment in this patch, see below. [...] > diff --git a/block/vhdx.c b/block/vhdx.c > index b0f66de..c5ec608 100644 > --- a/block/vhdx.c > +++ b/block/vhdx.c [...] > @@ -1405,12 +1407,18 @@ static int vhdx_create_new_headers(BlockBackend *blk, uint64_t image_size, > vhdx_guid_generate(&hdr->file_write_guid); > vhdx_guid_generate(&hdr->data_write_guid); > > - ret = vhdx_write_header(bs, hdr, VHDX_HEADER1_OFFSET, false); > + /* XXX Ugly way to get blk->root, but that's a feature, not a bug. This > + * hack makes it obvious that vhdx_write_header() bypasses the BlockBackend > + * here, which it really shouldn't be doing. */ Short question: Why not? Long stuff: The question I'm asking myself is why do we have a BB here at all? We don't need it, really. Actually, the whole file creation business is pretty shady. See bdrv_create(), that thing takes a filename for whatever reason (OK, historical ones, I know). This is fine on the protocol level, but strange on the format level. Ideally we'd want to seperate the protocol creation from the format creation, and the format level just gets some (indirect?) handle to the protocol BDS. So that's where the BB comes from, because the format level has to open the file (on the protocol level) itself somehow, and we decided to frown upon naked accesses to a BDS. (But I feel like I'm forgetting something here, I think there was another reason, too...) My intuitive and personal distinction between BDS and BB accesses is the following: - A BB is somehow related to an external user. External users always have to go through a BB to access a BDS tree. It's VM-y (by that I mean it's related to what the whole qemu program does). - A BDS is just a way to access a host image. It's not VM-y. So in the case of image creation, I personally would be completely fine with accesses to the naked BDS, because those are not external users and they just want to access a host image directly. As long as there's no real difference between how accessing an image through a BB or without a BB works, I don't particularly care which is used. But there's a catch: Some format drivers reuse code they use for existing images for image creation, for instance, writing out the image header. If image creation works on a BB while code for existing images does not (it works on a BdrvChild), sharing this code does not work trivially. Previously, this was worked around by just bypassing the BB, and I think this is completely fine and very reasonable, because, as I said above, I don't see why code for new images should be treated differently than code for existing images. However, that no longer works after this series, as we can see here. Bypassing the BB is not trivial and you say it's not intended. However, that would mean that code for creating a new image has to be completely separate from code for an existing image, because one takes a BB and the other takes a BdrvChild, and I don't agree on that. I think the issue is another: At this point, the current idea of image creation and the premise of this series clash. The premise of this series is that you should only be allowed to access a BDS if it's your child, and this mostly works because any BDS is someone's child, and actually only that someone wants to perform accesses on this (one exception being the qcow{2,}_write_compressed() code). However, this premise breaks here. Since all format drivers have to create the protocol file and the protocol BDS themselves, these are suddenly not their children. We work around that by putting an anonymous BB on top of them, so that they are someone's children; but that someone is the BB, not the image creator. I think you should be able to access any BDS if you own it, not just if it's your child. I'd argue that any BDS you own should be your child, actually. And the issue with our image creation is that the format drivers do own the protocol BDS, but it's not their child. Your answer to this is: Image creation code should just use the BB. Technically a valid answer, but I think this leads to unnecessary code duplication, and I also don't understand the reasoning behind this. I see two other ways: The first one is to introduce a way to retrieve the BdrvChild from a BB. Technically very simple. I do agree that this is not how the BB is supposed to be used, so I would definitely restrict this to anonymous BBs. The idea behind this is: If you own a BB, you should transitively own the BDS behind it. Thus, it should be your child, too, and you should be able to access it as such. Other argument: We have blk_bs(). I don't see the difference, frankly. The second one would be to have a way to introduce a very lightweight BDS parent object. You can just put this on top of an existing BDS to claim your parenthood, basically, and this one gives you then the BdrvChild handle. Technically not so simple. And also, I'd claim that we already have such a lightweight parent object which is called "anonymous BB". In my mind, the use of anonymous BBs is really just to claim parenthood to a some BDS. Thus, I think it should be very much fine to have a BdrvChild *blk_child(BlockBackend *blk) function which asserts that the BB is indeed anonymous. Max > + child = QLIST_FIRST(&bs->parents); > + assert(!QLIST_NEXT(child, next_parent)); > + > + ret = vhdx_write_header(child, hdr, VHDX_HEADER1_OFFSET, false); > if (ret < 0) { > goto exit; > } > hdr->sequence_number++; > - ret = vhdx_write_header(bs, hdr, VHDX_HEADER2_OFFSET, false); > + ret = vhdx_write_header(child, hdr, VHDX_HEADER2_OFFSET, false); > if (ret < 0) { > goto exit; > }