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 <[email protected]> >>>>> 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 >
