Am 28.08.2018 um 16:26 hat Peter Krempa geschrieben:
> On Fri, Aug 10, 2018 at 18:26:57 +0200, Kevin Wolf wrote:
> > The block-commit QMP command required specifying the top and base nodes
> > of the commit jobs using the file name of that node. While this works
> > in simple cases (local files with absolute paths), the file names
> > generated for more complicated setups can be hard to predict.
> > 
> > This adds two new options top-node and base-node to the command, which
> > allow specifying node names instead. They are mutually exclusive with
> > the old options.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/block-core.json | 24 ++++++++++++++++++------
> >  blockdev.c           | 32 ++++++++++++++++++++++++++++++--
> >  2 files changed, 48 insertions(+), 8 deletions(-)
> 
> While the below is not strictly relevant to this patch usage
> block-commit is not possible when using -blockdev. Thus the new
> arguments are not very useful otherwise.
> 
> With the new options I'm getting:
> 
> {"execute":"block-commit",
>  "arguments": { "device":"libvirt-3-format",
>                 "job-id":"libvirt-3-format",
>                 "top-node":"libvirt-8-format",
>                 "base-node":"libvirt-9-format",
>                 "auto-finalize":true,
>                 "auto-dismiss":false},
>  "id":"libvirt-16"}
> 
> {"id":"libvirt-16",
>  "error":{"class":"GenericError",
>           "desc":"Block node is read-only"}}
> 
> I'm pointing into the backing chain so the files are declared as read-only.
> 
> It works just-fine if I open them as read-write with
> -blockdev/blockdev-add but that obviously is not correct as you can't
> then share parts of the backing chain with other VMs due to image
> locking.
> 
> libvirt-3-format is read-write and all other node names are readonly in
> the above example.
> 
> The same also happens when using filenames:
> 
> {"execute":"block-commit",
>  "arguments" : {"device":"libvirt-3-format",
>                 "job-id":"libvirt-3-format",
>                 "top":"/var/lib/libvirt/images/rhel7.3.1483615252",
>                 "base":"/var/lib/libvirt/images/rhel7.3.1483605924",
>                 "auto-finalize":true,
>                 "auto-dismiss":false},
>  "id":"libvirt-13"}
> 
> {"id":"libvirt-13","error":{"class":"GenericError","desc":"Block node is read-only"}}

I see what's happening here. So we have a graph like this:

     guest device
          |
          v
    overlay-format -------> backing-format
    [read-only=off]         [read-only=on]
          |                        |
          v                        v
    overlay-proto           backing-proto
    [read-only=off]         [read-only=on]

The difference between your -blockdev use and -drive is that you
explicitly specify the read-only option for backing-proto (and you use a
separate -blockdev option anyway), so it doesn't just inherit it from
backing-format.

Now, when the commit job tries to reopen backing-format, your explicit
read-only=on for backing-proto still takes precedence and the node stays
read-only. If you hadn't used a separate -blockdev for backing-proto,
but included it in the definition for backing-format and left out the
read-only option, it would have inherited the option and reopen would
adjust both nodes. This is what happens with -drive.

So essentially, I guess, all places that want to switch between
read-only and read-write need to learn which other nodes (apart from the
top-level node they are interested in) must be reopened as well.

This looks a bit messy. :-/

Any good ideas anyone?

Kevin