Re: [Qemu-devel] [PATCH v4 29/38] blockdev: Add blockdev-insert-medium

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6558 bytes --]

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 <mreitz@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]