Re: [PATCH] blockdev: modify blockdev-change-medium to change non-removable device

[Denis Plotnikov]

On 22.10.2019 16:18, Max Reitz wrote:
> On 22.10.19 14:53, Denis Plotnikov wrote:
>> On 22.10.2019 14:05, Max Reitz wrote:
>>> On 21.10.19 08:50, Denis Plotnikov wrote:
>>>> On 18.10.2019 18:02, Max Reitz wrote:
>>>>> On 18.10.19 14:09, Denis Plotnikov wrote:
>>>>>> The modification is useful to workaround exclusive file access 
>>>>>> restrictions,
>>>>>> e.g. to implement VM migration with shared disk stored on a storage with
>>>>>> the exclusive file opening model: a destination VM is started waiting for
>>>>>> incomming migration with a fake image drive, and later, on the last 
>>>>>> migration
>>>>>> phase, the fake image file is replaced with the real one.
>>>>>>
>>>>>> Signed-off-by: Denis Plotnikov <address@hidden>
>>>>> Isn’t this what we would want to use reopen for?
>>>>>
>>>>> Max
>>>> Could you please explain what is "use reopen"?
>>> I was thinking of using (x-)blockdev-reopen to change the file that is
>>> used by the format node (e.g. from a null-co node to a real file); or to
>>> change the filename of the protocol node.
>>>
>>> Kevin has pointed out (on IRC) that this will not allow you to change
>>> the node that is directly attached to the device.  While I don’t know
>>> whether that’s really necessary in this case, if it were indeed
>>> necessary, I’d prefer a method to change a guest device’s @drive option
>>> because that seems more natural to me.
>>>
>>> In contrast, the approach taken in this patch seems not quite right to
>>> me, because it overloads the whole blockdev-change-medium command with a
>>> completely new and different implementation based on whether there’s a
>>> removable medium or not.  If the implementation is so different (and the
>>> interface is, too, because in one path you must give @medium whereas the
>>> other doesn’t evaluate it at all), it should be a new command.
>>>
>>> I don’t know whether we need a new command at all, though.  On the node
>>> level, we have (x-)blockdev-reopen.  So assuming we need something to
>>> change the link between the guest device and the block layer, I wonder
>>> whether there isn’t something similar; specifically, I’d prefer
>>> something to simply change the device’s @drive option.
>>>
>>> Kevin has pointed out (on IRC again) that there is indeed one such
>>> command, and that’s qom-set.  Unfortunately, this is what happens if you
>>> try to use it for @drive:
>>>
>>> {"error": {"class": "GenericError", "desc": "Attempt to set property
>>> 'drive' on anonymous device (type 'virtio-blk-device') after it was
>>> realized"}}
>>>
>>> However, Kevin has claimed it would be technically possible to make an
>>> exception for @drive.  Maybe this is worth investigating?
>> Is there any guess how complex it might be? In the case if it's quite
>> complex may be it's worth to make the separate command?
> I can translate the chat log for you:
>
> <kevin> In theory that’s called qom-set
> <kevin> However, I believe it doesn’t support qdev properties
> <kevin> Hm, but that could be changed specifically for the drive property
> <kevin> qdev keeps confusing me.  Drive isn’t supposed to call
> qdev_prop_set_after_realize(), but the error message’s still there.
> Where is that hidden call...?
> <kevin> Ah, set_pointer() does
> <kevin> Yes, then it should be possible to make that work rather locally
>
> And that took him about 10 minutes.
>
> So I suppose it would be to check in set_drive() and
> set_drive_iothread() whether the device is already realized, and if so,
> divert it to some other function that does the runtime change?
ok, that might be a good starting point for me. Thanks.
>
> (No idea how the qdev maintainers think about doing that in set_drive()
> and set_drive_iothread(), though)
>
>>> (As for blockdev-change-medium, as I’ve said, I don’t really think this
>>> fits there.  Furthermore, blockdev-change-medium is kind of a legacy
>>> command because I think every command but blockdev-add that does a
>>> bdrv_open() kind of is a legacy command.
>> Out of curiosity, could you please explain why it's decided to be so?
> Because we have blockdev-add, which supports all block device options
> there are and so on.  blockdev-change-medium (which is basically just a
> more rigid “change”) only gets filename, which isn’t as expressive.
>
> We generally want users to add new nodes with blockdev-add and let all
> other commands only take node-names.
>
> (There’s also the fact that historically we’ve used filenames to
> identify BlockDriverStates, but that doesn’t work so well.  Thus I think
> we should get away from using filenames as much as we can so people
> don’t use them for identification again.)
>
> Max

Thanks for the explanation, Max!

Denis

>