On 18.05.20 14:20, Maxim Levitsky wrote: > Hi! > Here is the updated series of my patches, incorporating all the feedback I > received.
You asked me on IRC what to do to get this series to move forward; considering I don’t think there were objections from anyone but me on v6, there are no further (substantial) objections from anyone on v7, and I gave all feedback I had on v6, I don’t think there’s much you can do right now. (Sorry for the delay, but, well, I was on PTO as you know.) As usual, I’ll try to get around to see whether I can rebase your series myself (because Dan hinted at some conflicts), and whether I feel like my comments were appropriately addressed (which I have little doubt about). That’s the plan. Note from a couple minutes later: From what I can see, the rebase conflicts don’t look too wild, but I don’t feel quite comfortable addressing all of them. There’s a functional one I could address in qemu-img.c (patch 3), where we need to bump OPTION_FORCE from 269 to 276. I could do that, but that’s not the only one, unfortunately. Patch 7 needs a bit more extensive modification: First, we need %s/bdrv_filter_default_perms/bdrv_default_perms/. Second, this will still not compile, because the .bdrv_child_perm interface has changed. BdrvChildRole is now an integer, so we also need s/const BdrvChildRole \*/BdrvChildRole /. (That gives us the nice side effect of being able to align the second line of the bdrv_default_perms() parameters (called from block_crypto_child_perms()) on the opening parenthesis.) Third, it becomes really interesting. With these changes, it would be wrong, because bdrv_default_perms() will then not use the permissions for a filter but for an image file with metadata – because that’s what the LUKS file child is. But that’s actually a bug that’s already there (and that I introduced). It makese sense to fix it in your series here, because to fix it we need a dedicated block_crypto_child_perms() function anyway. So, well. Do we want to cheat? Just let block_crypto_child_perms() call bdrv_default_perms() with role=BDRV_CHILD_FILTERED? That would be the previous behavior, but it would also be bad cheating. The comment that exists (before patch 7) above bdrv_crypto_luks.bdrv_child_perm says that we just want to allow share-rw=on, and we could achieve that simply by force-sharing the WRITE permission after invoking bdrv_default_perms() with the actual role (which is BDRV_CHILD_IMAGE). But what about the other stuff that sets bdrv_default_perms_for_storage() apart from bdrv_filter_default_perms()? I.e., force-taking WRITE and RESIZE permissions if the file is writable; force-taking the CONSISTENT_READ permission because we need the metadata; and unsharing the RESIZE permission? I think the best thing to do would be to call bdrv_default_perms() with the @role passed to block_crypto_child_perms() (i.e., BDRV_CHILD_IMAGE), and then relaxing the permissions, that is to share the WRITE and RESIZE persmissions, and to perhaps restore the WRITE and RESIZE permissions from what’s given to block_crypto_child_perms() (i.e., *nperm &= ~(WRITE | RESIZE); *nperm |= perm & (WRITE | RESIZE);), because LUKS doesn’t need them unless the image may actually be written. (OTOH, force-taking CONSISTENT_READ seems entirely reasonable.) Max
