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

[Max Reitz <mreitz@redhat.com>]

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

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.

Max


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