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

[Max Reitz]

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

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

[Wen Congyang]

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

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

[Max Reitz]

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

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

[Max Reitz]

On 10.09.2015 05:22, 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 
>> ---
>>  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.
> 
> I don't find such patches in the maillist.

That's because Jeff is still working on designing and writing them.

Max



signature.asc
Description: OpenPGP digital signature

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

[Wen Congyang]

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 
>>> ---
>>>  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.

Thanks
Wen Congyang

> 
> Max
> 
>> Thanks
>> Wen Congyang
>>
>>> +
>>> +qmp_blockdev_insert_anon_medium(device, bs, errp);
>>> +}
>>> +
>>>  /* throttling disk I/O limits */
>>>  void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t 
>>> bps_rd,
>>> int64_t bps_wr,
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 63a83e4..84c9b23 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1925,6 +1925,23 @@
>>>  { 'command': 'blockdev-remove-medium',
>>>'data': { 'device': 'str' } }
>>>  
>>> +##
>>> +# @blockdev-insert-medium:
>>> +#
>>> +# Inserts a medium (a block driver state tree) into a block device. That 
>>> block
>>> +# device's tray must currently be open and there must be no medium inserted
>>> +# already.
>>> +#
>>> +# @device:block device name
>>> +#
>>> +# @node-name: name of a node in the block driver state graph
>>> +#
>>> +# Since: 2.5
>>> +##
>>> +{ 'command': 'blockdev-insert-medium',
>>> +  'data': { 'device': 'str',
>>> +'node-name': 'str'} }
>>> +
>>>  
>>>  ##
>>>  # @BlockErrorAction
>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>> index ff6c572..b4c34fe 100644
>>> --- a/qmp-commands.hx
>>> +++ b/qmp-commands.hx
>>> @@ -3991,6 +3991,43 @@ Example:
>>>  EQMP
>>>  
>>>  {
>>> +.name   = "blockdev-insert-medium",
>>> +.args_type  = "device:s,node-name:s",
>>> +.mhandler.cmd_new = qmp_marshal_input_blockdev_insert_medium,
>>> +},
>>> +
>>> 

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

[Max Reitz]

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



signature.asc
Description: OpenPGP digital signature

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

[Wen Congyang]

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 
> ---
>  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?

Thanks
Wen Congyang

> +
> +qmp_blockdev_insert_anon_medium(device, bs, errp);
> +}
> +
>  /* throttling disk I/O limits */
>  void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t 
> bps_rd,
> int64_t bps_wr,
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 63a83e4..84c9b23 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1925,6 +1925,23 @@
>  { 'command': 'blockdev-remove-medium',
>'data': { 'device': 'str' } }
>  
> +##
> +# @blockdev-insert-medium:
> +#
> +# Inserts a medium (a block driver state tree) into a block device. That 
> block
> +# device's tray must currently be open and there must be no medium inserted
> +# already.
> +#
> +# @device:block device name
> +#
> +# @node-name: name of a node in the block driver state graph
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'blockdev-insert-medium',
> +  'data': { 'device': 'str',
> +'node-name': 'str'} }
> +
>  
>  ##
>  # @BlockErrorAction
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ff6c572..b4c34fe 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3991,6 +3991,43 @@ Example:
>  EQMP
>  
>  {
> +.name   = "blockdev-insert-medium",
> +.args_type  = "device:s,node-name:s",
> +.mhandler.cmd_new = qmp_marshal_input_blockdev_insert_medium,
> +},
> +
> +SQMP
> +blockdev-insert-medium
> +--
> +
> +Inserts a medium (a block driver state tree) into a block device. That block
> +device's tray must currently be open and there must be no medium inserted
> +already.
> +
> +Arguments:
> +
> +- "device": block device name (json-string)
> +- "node-name": root node of the BDS tree to insert into the block device
> +
> +Example:
> +
> +-> { "execute": "blockdev-add",
> + "arguments": { "options": { "node-name": "node0",
> + "driver": "raw",
> + "file": { "driver": "file",
> +   "filename": "fedora.iso" } } } }
> +
> +<- { "return": {} }
> +
> +-> { "execute": "blockdev-insert-medium",
> + "arguments": { "device": "ide1-cd0",
> +"node-name": "node0" } }
> +
> +<- { "return": {} }
> +
> +EQMP
> +
> +{
>  .name   = "query-named-block-nodes",
>  .args_type  = "",
>  .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
> 

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

[Max Reitz]

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 
>> ---
>>  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.

Max

> Thanks
> Wen Congyang
> 
>> +
>> +qmp_blockdev_insert_anon_medium(device, bs, errp);
>> +}
>> +
>>  /* throttling disk I/O limits */
>>  void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t 
>> bps_rd,
>> int64_t bps_wr,
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 63a83e4..84c9b23 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1925,6 +1925,23 @@
>>  { 'command': 'blockdev-remove-medium',
>>'data': { 'device': 'str' } }
>>  
>> +##
>> +# @blockdev-insert-medium:
>> +#
>> +# Inserts a medium (a block driver state tree) into a block device. That 
>> block
>> +# device's tray must currently be open and there must be no medium inserted
>> +# already.
>> +#
>> +# @device:block device name
>> +#
>> +# @node-name: name of a node in the block driver state graph
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'blockdev-insert-medium',
>> +  'data': { 'device': 'str',
>> +'node-name': 'str'} }
>> +
>>  
>>  ##
>>  # @BlockErrorAction
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index ff6c572..b4c34fe 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -3991,6 +3991,43 @@ Example:
>>  EQMP
>>  
>>  {
>> +.name   = "blockdev-insert-medium",
>> +.args_type  = "device:s,node-name:s",
>> +.mhandler.cmd_new = qmp_marshal_input_blockdev_insert_medium,
>> +},
>> +
>> +SQMP
>> +blockdev-insert-medium
>> +--
>> +
>> +Inserts a medium (a block driver state tree) into a block device. That block
>> +device's tray must currently be open and there must be no medium inserted
>> +already.
>> +
>> +Arguments:
>> +
>> +- "device": block device name (json-string)
>> +- "node-name": root node of the BDS tree to insert into the block device
>> +
>> +Example:
>> +
>> +-> 

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

[Eric Blake]

On 09/07/2015 11:53 PM, 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).
> 
> Is it OK to insert a medium to more than one BB?

A read-only medium - sure :)

A read-write medium - probably a recipe for data corruption.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature