On 11.09.2015 09:30, Wen Congyang wrote: > On 09/11/2015 03:09 AM, Max Reitz wrote: >> On 10.09.2015 03:12, Wen Congyang wrote: >>> On 09/09/2015 08:59 PM, Max Reitz wrote: >>>> On 09.09.2015 12:01, Wen Congyang wrote: >>>>> On 09/09/2015 05:20 AM, Max Reitz wrote: >>>>>> On 08.09.2015 11:13, Wen Congyang wrote: >>>>>>> On 07/21/2015 01:45 AM, Max Reitz wrote: >>>>>>>> And a helper function for that, which directly takes a pointer to the >>>>>>>> BDS to be inserted instead of its node-name (which will be used for >>>>>>>> implementing 'change' using blockdev-insert-medium). >>>>>>>> >>>>>>>> Signed-off-by: Max Reitz <mre...@redhat.com> >>>>>>>> --- >>>>>>>> blockdev.c | 48 >>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>> qapi/block-core.json | 17 +++++++++++++++++ >>>>>>>> qmp-commands.hx | 37 +++++++++++++++++++++++++++++++++++++ >>>>>>>> 3 files changed, 102 insertions(+) >>>>>>>> >>>>>>>> diff --git a/blockdev.c b/blockdev.c >>>>>>>> index 481760a..a80d0e2 100644 >>>>>>>> --- a/blockdev.c >>>>>>>> +++ b/blockdev.c >>>>>>>> @@ -2164,6 +2164,54 @@ void qmp_blockdev_remove_medium(const char >>>>>>>> *device, Error **errp) >>>>>>>> } >>>>>>>> } >>>>>>>> >>>>>>>> +static void qmp_blockdev_insert_anon_medium(const char *device, >>>>>>>> + BlockDriverState *bs, >>>>>>>> Error **errp) >>>>>>>> +{ >>>>>>>> + BlockBackend *blk; >>>>>>>> + bool has_device; >>>>>>>> + >>>>>>>> + blk = blk_by_name(device); >>>>>>>> + if (!blk) { >>>>>>>> + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, >>>>>>>> + "Device '%s' not found", device); >>>>>>>> + return; >>>>>>>> + } >>>>>>>> + >>>>>>>> + /* For BBs without a device, we can exchange the BDS tree at will >>>>>>>> */ >>>>>>>> + has_device = blk_get_attached_dev(blk); >>>>>>>> + >>>>>>>> + if (has_device && !blk_dev_has_removable_media(blk)) { >>>>>>>> + error_setg(errp, "Device '%s' is not removable", device); >>>>>>>> + return; >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (has_device && !blk_dev_is_tray_open(blk)) { >>>>>>>> + error_setg(errp, "Tray of device '%s' is not open", device); >>>>>>>> + return; >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (blk_bs(blk)) { >>>>>>>> + error_setg(errp, "There already is a medium in device '%s'", >>>>>>>> device); >>>>>>>> + return; >>>>>>>> + } >>>>>>>> + >>>>>>>> + blk_insert_bs(blk, bs); >>>>>>>> +} >>>>>>>> + >>>>>>>> +void qmp_blockdev_insert_medium(const char *device, const char >>>>>>>> *node_name, >>>>>>>> + Error **errp) >>>>>>>> +{ >>>>>>>> + BlockDriverState *bs; >>>>>>>> + >>>>>>>> + bs = bdrv_find_node(node_name); >>>>>>>> + if (!bs) { >>>>>>>> + error_setg(errp, "Node '%s' not found", node_name); >>>>>>>> + return; >>>>>>>> + } >>>>>>> >>>>>>> Hmm, it is OK if the bs is not top BDS? >>>>>> >>>>>> I think so, yes. Generally, there's probably no reason to do that, but I >>>>>> don't know why we should not allow that case. For instance, you might >>>>>> want to make a backing file available read-only somewhere. >>>>>> >>>>>> It should be impossible to make it available writable, and it should not >>>>>> be allowed to start a block-commit operation while the backing file can >>>>>> be accessed by the guest, but this should be achieved using op blockers. >>>>>> >>>>>> What we need for this to work are fine-grained op blockers, I think. But >>>>>> working around that for now by only allowing to insert top BDS won't >>>>>> work, since you can still start block jobs which target top BDS, too >>>>>> (e.g. blockdev-backup can write to a BDS/BB that is visible to the >>>>>> guest). >>>>>> >>>>>> All in all, I think it's fine to insert non-top BDS, but we should >>>>>> definitely worry about which exact BDS one can insert once we have >>>>>> fine-grained op blockers. >>>>> >>>>> A BDS can be written by its parent, its block backend, a block job.. >>>>> So I think we should have some way to avoid more than two sources writing >>>>> to it, otherwise the data may be corrupted. >>>> >>>> Yes, and that would be op blockers. >>>> >>>> As I said, using blockdev-backup you can write to a BB that can be >>>> written to by the guest as well. I think this is a bug, but it is a bug >>>> that needs to be fixed by having better op blockers in place, which Jeff >>>> Cody is working on. >>>> >>>> Regarding this series, I don't consider this to be too big of an issue. >>>> Yes, if you are working with floppy disks, you can have the case of a >>>> block job and the guest writing to the BDS at the same time. But I can't >>>> really imagine who would use floppy disks and block jobs at the same >>>> time (people who still use floppy disks for their VMs don't strike me as >>>> the kind of people who use the management features of qemu, especially >>>> not for those floppy disks). >>>> >>>> Other than that, this function (blockdev-insert-medium) can only be used >>>> for optical ROM devices (I don't think we have CD/DVD-RW support, do >>>> we?), so it's much less of an issue there. >>>> >>>> So all in all I don't consider this too big of an issue here. If others >>>> think different, then I would delay this part of the series (which >>>> overhauls the "change" command) until we have fine-grained op blockers. >>> >>> In most cases, the user uses this command to change CD/DVD media, so it is >>> OK. >>> But IIRC scsi disk can also be changed. So we can mark this command as >>> experimental >>> (the command name can be x-blockdev-insert-medium). >> >> I'd rather delay this part than mark it experimental. But then again, >> seeing that we have cases like this already (i.e. blockdev-backup) and >> nobody seems to be complaining, I still think it should be fine. > > Hmm, another question, when will you post the newest patchset? Block > replication is > based on this patchset.
The best I can say is "once I have the time". There are a lot of things going on right now, not only regarding code I have to write, but also regarding patches I have to review. This series hasn't really been on other people's priority list for eight months now, so I had to take up other things in between which means that now I cannot just focus on this series alone and don't do anything else. Be assured that I took notice that you requested a new version, though, so I will work on it once I can. Max
signature.asc
Description: OpenPGP digital signature