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

[Max Reitz]

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?

(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

signature.asc
Description: OpenPGP digital signature