| Message ID | 20191018120950.26849-1-dplotnikov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blockdev: modify blockdev-change-medium to change non-removable device | expand |
On 10/18/19 7:09 AM, 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 <dplotnikov@virtuozzo.com> > --- > +++ b/qapi/block-core.json > @@ -4769,6 +4769,8 @@ > # @read-only-mode: change the read-only mode of the device; defaults > # to 'retain' > # > +# @medium-name: drive-name when changing the media in non-removable devices > +# ignored when changing media in removable devices s/devices ignored/devices. Ignored/ Missing a '(since 4.2)' tag.
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 <dplotnikov@virtuozzo.com> Isn’t this what we would want to use reopen for? Max
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 <dplotnikov@virtuozzo.com> > Isn’t this what we would want to use reopen for? > > Max Could you please explain what is "use reopen"? Denis >
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 <dplotnikov@virtuozzo.com> >> 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? (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. So if anything, it should be a new command that then takes a node-name. But OTOH, it would be a bit strange to add a separate command for something that in theory should be covered by qom-set @drive.) Max
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 <dplotnikov@virtuozzo.com> >>> 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? > > > (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? > So if anything, it should be a > new command that then takes a node-name. > But OTOH, it would be a bit strange to add a separate command for > something that in theory should be covered by qom-set @drive.) > > Max >
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 <dplotnikov@virtuozzo.com> >>>> 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
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 <dplotnikov@virtuozzo.com> >>>>> 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 >
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 <dplotnikov@virtuozzo.com> >>> 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. Ok, assume we can set @drive option with help of improved qom-set. But how to create this new blk? blockdev-add don't have 'id' parameter anymore and don't create blk... > > 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? > > > (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. So if anything, it should be a > new command that then takes a node-name. > But OTOH, it would be a bit strange to add a separate command for > something that in theory should be covered by qom-set @drive.) > > Max >
23.10.2019 16:56, Vladimir Sementsov-Ogievskiy wrote: > 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 <dplotnikov@virtuozzo.com> >>>> 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. > > Ok, assume we can set @drive option with help of improved qom-set. > But how to create this new blk? blockdev-add don't have 'id' parameter anymore > and don't create blk... Hmm, I see, we have command blockdev-insert-medium(id, node-name). Isn't it what we want? > >> >> 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? >> >> >> (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. So if anything, it should be a >> new command that then takes a node-name. >> But OTOH, it would be a bit strange to add a separate command for >> something that in theory should be covered by qom-set @drive.) >> >> Max >> > >
Am 23.10.2019 um 15:56 hat Vladimir Sementsov-Ogievskiy geschrieben: > 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 <dplotnikov@virtuozzo.com> > >>> 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. > > Ok, assume we can set @drive option with help of improved qom-set. > But how to create this new blk? blockdev-add don't have 'id' parameter anymore > and don't create blk... We don't need to create a new BlockBackend. You would set the drive qdev property to a new node name and that would just internally call blk_remove_bs() and blk_insert_bs() for the existing BlockBackend that is owned by the device. Kevin
On 23.10.19 16:10, Vladimir Sementsov-Ogievskiy wrote: > 23.10.2019 16:56, Vladimir Sementsov-Ogievskiy wrote: >> 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 <dplotnikov@virtuozzo.com> >>>>> 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. >> >> Ok, assume we can set @drive option with help of improved qom-set. >> But how to create this new blk? blockdev-add don't have 'id' parameter anymore >> and don't create blk... > > > Hmm, I see, we have command blockdev-insert-medium(id, node-name). Isn't it what > we want? blockdev-insert-medium requires that the device be empty, so it would need to be preceded by a blockdev-remove-medium. And then it isn’t a transaction... Also, it currently is limited (intentionally) to devices with removable media. I suppose all of this could be changed, but basically I don’t see why we wouldn’t use qom-set. (Well, because it’s ugly to call this new functionality from set_drive(), but other than that...) (And I don’t quite know whether there isn’t more to be done for devices that don’t inherently support removable media than just to swap the node attached to the BB. Don’t they also need to be drained or something?) Max
diff --git a/blockdev.c b/blockdev.c index d358169995..23f3465cfc 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2609,6 +2609,8 @@ void qmp_blockdev_change_medium(bool has_device, const char *device, bool has_format, const char *format, bool has_read_only, BlockdevChangeReadOnlyMode read_only, + bool has_medium_name, + const char *medium_name, Error **errp) { BlockBackend *blk; @@ -2667,29 +2669,56 @@ void qmp_blockdev_change_medium(bool has_device, const char *device, goto fail; } - rc = do_open_tray(has_device ? device : NULL, - has_id ? id : NULL, - false, &err); - if (rc && rc != -ENOSYS) { - error_propagate(errp, err); - goto fail; - } - error_free(err); - err = NULL; + if (blk_dev_has_removable_media(blk)) { + rc = do_open_tray(has_device ? device : NULL, + has_id ? id : NULL, + false, &err); + if (rc && rc != -ENOSYS) { + error_propagate(errp, err); + goto fail; + } + error_free(err); + err = NULL; - blockdev_remove_medium(has_device, device, has_id, id, &err); - if (err) { - error_propagate(errp, err); - goto fail; - } + blockdev_remove_medium(has_device, device, has_id, id, &err); + if (err) { + error_propagate(errp, err); + goto fail; + } - qmp_blockdev_insert_anon_medium(blk, medium_bs, &err); - if (err) { - error_propagate(errp, err); - goto fail; - } + qmp_blockdev_insert_anon_medium(blk, medium_bs, &err); + if (err) { + error_propagate(errp, err); + goto fail; + } + + qmp_blockdev_close_tray(has_device, device, has_id, id, errp); + } else { + if (!medium_name) { + error_setg(errp, "A medium name should be given"); + goto fail; + } - qmp_blockdev_close_tray(has_device, device, has_id, id, errp); + if (runstate_is_running()) { + error_setg(errp, "Can't set a medium for non-removable device " + "in a running VM"); + goto fail; + } + + if (strlen(blk_name(blk))) { + error_setg(errp, "The device already has a medium"); + goto fail; + } + + if (blk_insert_bs(blk, medium_bs, &err) < 0) { + error_propagate(errp, err); + goto fail; + } + + if (!monitor_add_blk(blk, medium_name, &err)) { + error_propagate(errp, err); + } + } fail: /* If the medium has been inserted, the device has its own reference, so diff --git a/hmp.c b/hmp.c index 8eec768088..fc7bac5b4b 100644 --- a/hmp.c +++ b/hmp.c @@ -1948,6 +1948,7 @@ void hmp_change(Monitor *mon, const QDict *qdict) const char *target = qdict_get_str(qdict, "target"); const char *arg = qdict_get_try_str(qdict, "arg"); const char *read_only = qdict_get_try_str(qdict, "read-only-mode"); + const char *target_name = qdict_get_try_str(qdict, "target-name"); BlockdevChangeReadOnlyMode read_only_mode = 0; Error *err = NULL; @@ -1982,6 +1983,7 @@ void hmp_change(Monitor *mon, const QDict *qdict) qmp_blockdev_change_medium(true, device, false, NULL, target, !!arg, arg, !!read_only, read_only_mode, + !!target_name, target_name, &err); } diff --git a/qapi/block-core.json b/qapi/block-core.json index 7ccbfff9d0..f493a7c737 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -4769,6 +4769,8 @@ # @read-only-mode: change the read-only mode of the device; defaults # to 'retain' # +# @medium-name: drive-name when changing the media in non-removable devices +# ignored when changing media in removable devices # Since: 2.5 # # Examples: @@ -4807,9 +4809,8 @@ '*id': 'str', 'filename': 'str', '*format': 'str', - '*read-only-mode': 'BlockdevChangeReadOnlyMode' } } - - + '*read-only-mode': 'BlockdevChangeReadOnlyMode', + '*medium-name': 'str' } } ## # @BlockErrorAction: # diff --git a/qmp.c b/qmp.c index b92d62cd5f..c95831a49f 100644 --- a/qmp.c +++ b/qmp.c @@ -399,7 +399,8 @@ void qmp_change(const char *device, const char *target, #endif } else { qmp_blockdev_change_medium(true, device, false, NULL, target, - has_arg, arg, false, 0, errp); + has_arg, arg, false, 0, false, NULL, + errp); } }
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 <dplotnikov@virtuozzo.com> --- blockdev.c | 69 +++++++++++++++++++++++++++++++------------- hmp.c | 2 ++ qapi/block-core.json | 7 +++-- qmp.c | 3 +- 4 files changed, 57 insertions(+), 24 deletions(-)