Message ID | 1437414365-11881-30-git-send-email-mreitz@redhat.com |
---|---|
State | New |
Headers | show |
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? Thanks Wen Congyang > > 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; > + } > + > + 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, >
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? 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, >
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.
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. 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: >> + >> +-> { "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, >> >
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. 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, >>> + }, >>> + >>> +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, >>> >> > >
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
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). Thanks Wen Congyang > > Max >
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. I don't find such patches in the maillist. Thanks Wen Congyang > > 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 >
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. Max
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 <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. > > I don't find such patches in the maillist. That's because Jeff is still working on designing and writing them. Max
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. Thanks Wen Congyang > > Max >
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
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; + } + + 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,
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(+)