| Message ID | 1386862440-8003-5-git-send-email-benoit@irqsave.net |
|---|---|
| State | New |
| Headers | show |
On Thu, 12/12 16:33, Benoît Canet wrote: > There was two candidate ways to implement named node manipulation: > > 1) > { 'command': 'block_passwd', 'data': {'*device': 'str', > '*node-name': 'str', 'password': 'str'} > } > > 2) > > { 'command': 'block_passwd', 'data': {'device': 'str', > '*device-is-node': 'bool', > 'password': 'str'} } > > Luiz proposed 1 and says 2 was an abuse of the QMP interface and proposed to > rewrite the QMP block interface for 2.0. > > Luiz does not like in 1 the fact that 2 fields are optional but one of them must > be specified leading to an abuse of the QMP semantic. > > Kevin argumented that 2 what a clear abuse of the device field and would not be > practical when reading fast some log file because the user would read "device" > and think that a device is manipulated when it's in fact a node name. > Documentation of 1 make it pretty clear what to do for the user. > > Kevin argued that all bs are node including devices ones so 2 does not make > sense. > > Kevin also argued that rewriting the QMP block interface would not make disapear > the current one. > > Kevin pushed the argument that making the QAPI generator compatible with the > semantic of the operation would need a rewrite that no one has done yet. > > A vote has been done on the list to elect the version to use and 1 won. > > For reference the complete thread is: > "[Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver > states." > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > --- > block.c | 32 ++++++++++++++++++++++++++++++++ > blockdev.c | 13 +++++++++---- > hmp.c | 2 +- > include/block/block.h | 3 +++ > qapi-schema.json | 9 +++++++-- > qmp-commands.hx | 3 ++- > 6 files changed, 54 insertions(+), 8 deletions(-) > > diff --git a/block.c b/block.c > index 78d13e5..22190a4 100644 > --- a/block.c > +++ b/block.c > @@ -3207,6 +3207,38 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void) > return list; > } > > +BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device, > + bool has_node_name, const char *node_name, > + Error **errp) > +{ > + BlockDriverState *bs = NULL; > + > + if (has_device == has_node_name) { > + error_setg(errp, "Use either device or node-name but not both"); > + return NULL; > + } > + > + if (has_device) { > + bs = bdrv_find(device); > + > + if (!bs) { > + error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + return NULL; > + } > + > + return bs; > + } > + > + bs = bdrv_find_node(node_name); > + > + if (!bs) { > + error_set(errp, QERR_DEVICE_NOT_FOUND, node_name); > + return NULL; > + } > + > + return bs; > +} > + > BlockDriverState *bdrv_next(BlockDriverState *bs) > { > if (!bs) { > diff --git a/blockdev.c b/blockdev.c > index 204ab40..838df50 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1474,14 +1474,19 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp) > eject_device(bs, force, errp); > } > > -void qmp_block_passwd(const char *device, const char *password, Error **errp) > +void qmp_block_passwd(bool has_device, const char *device, > + bool has_node_name, const char *node_name, > + const char *password, Error **errp) > { > + Error *local_err = NULL; > BlockDriverState *bs; > int err; > > - bs = bdrv_find(device); > - if (!bs) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + bs = bdrv_lookup_bs(has_device, device, > + has_node_name, node_name, > + &local_err); > + if (error_is_set(&local_err)) { > + error_propagate(errp, local_err); > return; > } > > diff --git a/hmp.c b/hmp.c > index 32ee285..3820fbe 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -870,7 +870,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict) > const char *password = qdict_get_str(qdict, "password"); > Error *errp = NULL; > > - qmp_block_passwd(device, password, &errp); > + qmp_block_passwd(true, device, false, NULL, password, &errp); > hmp_handle_error(mon, &errp); > } > > diff --git a/include/block/block.h b/include/block/block.h > index 8c10123..f7d8017 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -376,6 +376,9 @@ const char *bdrv_get_format_name(BlockDriverState *bs); > BlockDriverState *bdrv_find(const char *name); > BlockDriverState *bdrv_find_node(const char *node_name); > BlockDeviceInfoList *bdrv_named_nodes_list(void); > +BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device, > + bool has_node_name, const char *node_name, > + Error **errp); > BlockDriverState *bdrv_next(BlockDriverState *bs); > void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), > void *opaque); > diff --git a/qapi-schema.json b/qapi-schema.json > index 0dadd5d..903fcb6 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1676,7 +1676,11 @@ > # determine which ones are encrypted, set the passwords with this command, and > # then start the guest with the @cont command. > # > -# @device: the name of the device to set the password on > +# Either @device or @node-name must be set but not both. > +# > +# @device: #optional the name of the block backend device to set the password on > +# > +# @node-name: #optional graph node name to set the password on (Since 2.0) > # > # @password: the password to use for the device > # > @@ -1690,7 +1694,8 @@ > # > # Since: 0.14.0 > ## > -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} } > +{ 'command': 'block_passwd', 'data': {'*device': 'str', > + '*node-name': 'str', 'password': 'str'} } > > ## > # @balloon: > diff --git a/qmp-commands.hx b/qmp-commands.hx > index d644fe9..1451c1a 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -1452,7 +1452,7 @@ EQMP > > { > .name = "block_passwd", > - .args_type = "device:B,password:s", > + .args_type = "device:s?,node-name:s?,password:s", > .mhandler.cmd_new = qmp_marshal_input_block_passwd, > }, > > @@ -1465,6 +1465,7 @@ Set the password of encrypted block devices. > Arguments: > > - "device": device name (json-string) > +- "node-name": name in the block driver state graph (json-string) > - "password": password (json-string) > > Example: > -- > 1.8.3.2 > > Reviewed-by: Fam Zheng <famz@redhat.com>
Am 12.12.2013 um 16:33 hat Benoît Canet geschrieben: > There was two candidate ways to implement named node manipulation: > > 1) > { 'command': 'block_passwd', 'data': {'*device': 'str', > '*node-name': 'str', 'password': 'str'} > } > > 2) > > { 'command': 'block_passwd', 'data': {'device': 'str', > '*device-is-node': 'bool', > 'password': 'str'} } > > Luiz proposed 1 and says 2 was an abuse of the QMP interface and proposed to > rewrite the QMP block interface for 2.0. > > Luiz does not like in 1 the fact that 2 fields are optional but one of them must > be specified leading to an abuse of the QMP semantic. > > Kevin argumented that 2 what a clear abuse of the device field and would not be > practical when reading fast some log file because the user would read "device" > and think that a device is manipulated when it's in fact a node name. > Documentation of 1 make it pretty clear what to do for the user. > > Kevin argued that all bs are node including devices ones so 2 does not make > sense. > > Kevin also argued that rewriting the QMP block interface would not make disapear > the current one. > > Kevin pushed the argument that making the QAPI generator compatible with the > semantic of the operation would need a rewrite that no one has done yet. > > A vote has been done on the list to elect the version to use and 1 won. > > For reference the complete thread is: > "[Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver > states." > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > --- > block.c | 32 ++++++++++++++++++++++++++++++++ > blockdev.c | 13 +++++++++---- > hmp.c | 2 +- > include/block/block.h | 3 +++ > qapi-schema.json | 9 +++++++-- > qmp-commands.hx | 3 ++- > 6 files changed, 54 insertions(+), 8 deletions(-) > > diff --git a/block.c b/block.c > index 78d13e5..22190a4 100644 > --- a/block.c > +++ b/block.c > @@ -3207,6 +3207,38 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void) > return list; > } > > +BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device, > + bool has_node_name, const char *node_name, > + Error **errp) This is a normal function without generated callers, so can we get rid of has_device/has_node_name and just switch to passing NULL if it's not present? That would make it more convenient to use this function in other places. Kevin
diff --git a/block.c b/block.c index 78d13e5..22190a4 100644 --- a/block.c +++ b/block.c @@ -3207,6 +3207,38 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void) return list; } +BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device, + bool has_node_name, const char *node_name, + Error **errp) +{ + BlockDriverState *bs = NULL; + + if (has_device == has_node_name) { + error_setg(errp, "Use either device or node-name but not both"); + return NULL; + } + + if (has_device) { + bs = bdrv_find(device); + + if (!bs) { + error_set(errp, QERR_DEVICE_NOT_FOUND, device); + return NULL; + } + + return bs; + } + + bs = bdrv_find_node(node_name); + + if (!bs) { + error_set(errp, QERR_DEVICE_NOT_FOUND, node_name); + return NULL; + } + + return bs; +} + BlockDriverState *bdrv_next(BlockDriverState *bs) { if (!bs) { diff --git a/blockdev.c b/blockdev.c index 204ab40..838df50 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1474,14 +1474,19 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp) eject_device(bs, force, errp); } -void qmp_block_passwd(const char *device, const char *password, Error **errp) +void qmp_block_passwd(bool has_device, const char *device, + bool has_node_name, const char *node_name, + const char *password, Error **errp) { + Error *local_err = NULL; BlockDriverState *bs; int err; - bs = bdrv_find(device); - if (!bs) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + bs = bdrv_lookup_bs(has_device, device, + has_node_name, node_name, + &local_err); + if (error_is_set(&local_err)) { + error_propagate(errp, local_err); return; } diff --git a/hmp.c b/hmp.c index 32ee285..3820fbe 100644 --- a/hmp.c +++ b/hmp.c @@ -870,7 +870,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict) const char *password = qdict_get_str(qdict, "password"); Error *errp = NULL; - qmp_block_passwd(device, password, &errp); + qmp_block_passwd(true, device, false, NULL, password, &errp); hmp_handle_error(mon, &errp); } diff --git a/include/block/block.h b/include/block/block.h index 8c10123..f7d8017 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -376,6 +376,9 @@ const char *bdrv_get_format_name(BlockDriverState *bs); BlockDriverState *bdrv_find(const char *name); BlockDriverState *bdrv_find_node(const char *node_name); BlockDeviceInfoList *bdrv_named_nodes_list(void); +BlockDriverState *bdrv_lookup_bs(bool has_device, const char *device, + bool has_node_name, const char *node_name, + Error **errp); BlockDriverState *bdrv_next(BlockDriverState *bs); void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque); diff --git a/qapi-schema.json b/qapi-schema.json index 0dadd5d..903fcb6 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1676,7 +1676,11 @@ # determine which ones are encrypted, set the passwords with this command, and # then start the guest with the @cont command. # -# @device: the name of the device to set the password on +# Either @device or @node-name must be set but not both. +# +# @device: #optional the name of the block backend device to set the password on +# +# @node-name: #optional graph node name to set the password on (Since 2.0) # # @password: the password to use for the device # @@ -1690,7 +1694,8 @@ # # Since: 0.14.0 ## -{ 'command': 'block_passwd', 'data': {'device': 'str', 'password': 'str'} } +{ 'command': 'block_passwd', 'data': {'*device': 'str', + '*node-name': 'str', 'password': 'str'} } ## # @balloon: diff --git a/qmp-commands.hx b/qmp-commands.hx index d644fe9..1451c1a 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1452,7 +1452,7 @@ EQMP { .name = "block_passwd", - .args_type = "device:B,password:s", + .args_type = "device:s?,node-name:s?,password:s", .mhandler.cmd_new = qmp_marshal_input_block_passwd, }, @@ -1465,6 +1465,7 @@ Set the password of encrypted block devices. Arguments: - "device": device name (json-string) +- "node-name": name in the block driver state graph (json-string) - "password": password (json-string) Example:
There was two candidate ways to implement named node manipulation: 1) { 'command': 'block_passwd', 'data': {'*device': 'str', '*node-name': 'str', 'password': 'str'} } 2) { 'command': 'block_passwd', 'data': {'device': 'str', '*device-is-node': 'bool', 'password': 'str'} } Luiz proposed 1 and says 2 was an abuse of the QMP interface and proposed to rewrite the QMP block interface for 2.0. Luiz does not like in 1 the fact that 2 fields are optional but one of them must be specified leading to an abuse of the QMP semantic. Kevin argumented that 2 what a clear abuse of the device field and would not be practical when reading fast some log file because the user would read "device" and think that a device is manipulated when it's in fact a node name. Documentation of 1 make it pretty clear what to do for the user. Kevin argued that all bs are node including devices ones so 2 does not make sense. Kevin also argued that rewriting the QMP block interface would not make disapear the current one. Kevin pushed the argument that making the QAPI generator compatible with the semantic of the operation would need a rewrite that no one has done yet. A vote has been done on the list to elect the version to use and 1 won. For reference the complete thread is: "[Qemu-devel] [PATCH V4 4/7] qmp: Allow to change password on names block driver states." Signed-off-by: Benoit Canet <benoit@irqsave.net> --- block.c | 32 ++++++++++++++++++++++++++++++++ blockdev.c | 13 +++++++++---- hmp.c | 2 +- include/block/block.h | 3 +++ qapi-schema.json | 9 +++++++-- qmp-commands.hx | 3 ++- 6 files changed, 54 insertions(+), 8 deletions(-)