On Wed, 24 Aug 2011 13:43:08 -0500
Anthony Liguori <aligu...@us.ibm.com> wrote:
> Convert the change command to QAPI and document all of its gloriousness.
>
> Signed-off-by: Anthony Liguori <aligu...@us.ibm.com>
> ---
> blockdev.c | 27 ++++++++----------
> blockdev.h | 6 +++-
> hmp-commands.hx | 3 +-
> hmp.c | 59 ++++++++++++++++++++++++++++++++++++++++
> hmp.h | 1 +
> monitor.c | 78 +++++++----------------------------------------------
> qapi-schema.json | 38 ++++++++++++++++++++++++++
> qmp-commands.hx | 3 +-
> 8 files changed, 127 insertions(+), 88 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index c00c69d..3832118 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -785,8 +785,9 @@ void qmp_change_blockdev(const char *device, const char
> *filename,
> qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, password, errp);
> }
>
> -int do_change_block(Monitor *mon, const char *device,
> - const char *filename, const char *fmt)
> +void deprecated_qmp_change_blockdev(const char *device, const char *filename,
> + bool has_format, const char *format,
> + Error **errp)
> {
> BlockDriverState *bs;
> BlockDriver *drv = NULL;
> @@ -795,14 +796,14 @@ int do_change_block(Monitor *mon, const char *device,
>
> bs = bdrv_find(device);
> if (!bs) {
> - qerror_report(QERR_DEVICE_NOT_FOUND, device);
> - return -1;
> + error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> + return;
> }
> - if (fmt) {
> - drv = bdrv_find_whitelisted_format(fmt);
> + if (has_format) {
> + drv = bdrv_find_whitelisted_format(format);
> if (!drv) {
> - qerror_report(QERR_INVALID_BLOCK_FORMAT, fmt);
> - return -1;
> + error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> + return;
> }
> }
>
> @@ -811,17 +812,13 @@ int do_change_block(Monitor *mon, const char *device,
>
> eject_device(bs, 0, &err);
> if (err) {
> - qerror_report_err(err);
> - return -1;
> + error_propagate(errp, err);
> + return;
> }
>
> qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err);
> - if (err) {
> - qerror_report_err(err);
> - return -1;
> - }
>
> - return 0;
> + error_propagate(errp, err);
> }
>
> int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> diff --git a/blockdev.h b/blockdev.h
> index badbf01..54b142f 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -12,6 +12,7 @@
>
> #include "block.h"
> #include "qemu-queue.h"
> +#include "error.h"
>
> void blockdev_mark_auto_del(BlockDriverState *bs);
> void blockdev_auto_del(BlockDriverState *bs);
> @@ -59,8 +60,9 @@ DriveInfo *add_init_drive(const char *opts);
>
> void do_commit(Monitor *mon, const QDict *qdict);
> int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject
> **ret_data);
> -int do_change_block(Monitor *mon, const char *device,
> - const char *filename, const char *fmt);
> +void deprecated_qmp_change_blockdev(const char *device, const char *filename,
> + bool has_format, const char *format,
> + Error **errp);
> int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
> int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
> int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 2f0ffa3..700d2f1 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -108,8 +108,7 @@ ETEXI
> .args_type = "device:B,target:F,arg:s?",
> .params = "device filename [format]",
> .help = "change a removable medium, optional format",
> - .user_print = monitor_user_noop,
> - .mhandler.cmd_new = do_change,
> + .mhandler.cmd = hmp_change,
> },
>
> STEXI
> diff --git a/hmp.c b/hmp.c
> index a8ae36b..e8bca20 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -50,3 +50,62 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict)
> error_free(err);
> }
> }
> +
> +static void cb_hmp_change_bdrv_pwd(Monitor *mon, const char *password,
> + void *opaque)
> +{
> + Error *encryption_err = opaque;
> + Error *err = NULL;
> + const char *device;
> +
> + device = error_get_field(encryption_err, "device");
> +
> + qmp_block_passwd(device, password, &err);
> + if (err) {
> + monitor_printf(mon, "invalid password\n");
> + error_free(err);
> + }
> +
> + error_free(encryption_err);
> +
> + monitor_read_command(mon, 1);
> +}
> +
> +static void hmp_change_read_arg(Monitor *mon, const char *password,
> + void *opaque)
> +{
> + qmp_change_vnc_password(password, NULL);
> + monitor_read_command(mon, 1);
> +}
> +
> +void hmp_change(Monitor *mon, const QDict *qdict)
> +{
> + const char *device = qdict_get_str(qdict, "device");
> + const char *target = qdict_get_str(qdict, "target");
> + const char *arg = qdict_get_try_str(qdict, "arg");
> + Error *err = NULL;
> +
> + if (strcmp(device, "vnc") == 0 &&
> + (strcmp(target, "passwd") == 0 ||
> + strcmp(target, "password") == 0)) {
> + if (arg == NULL) {
> + monitor_read_password(mon, hmp_change_read_arg, NULL);
> + return;
> + }
> + }
> +
> + qmp_change(device, target, !!arg, arg, &err);
> + if (error_is_type(err, QERR_DEVICE_ENCRYPTED)) {
You need to do:
if (err && error_is_type(err, QERR_DEVICE_ENCRYPTED)) {
Otherwise it segfaults. I'd also add a 'assert(err);' in error_is_type().
Or return false in error_is_type() is 'err' is NULL.
> + monitor_printf(mon, "%s (%s) is encrypted.\n",
> + error_get_field(err, "device"),
> + error_get_field(err, "encrypted_filename"));
> + if (!monitor_get_rs(mon)) {
> + monitor_printf(mon,
> + "terminal does not support password prompting\n");
> + error_free(err);
> + return;
> + }
> + readline_start(monitor_get_rs(mon), "Password: ", 1,
> + cb_hmp_change_bdrv_pwd, err);
> + }
> +}
> diff --git a/hmp.h b/hmp.h
> index 8f72ef2..9df6ccc 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -20,5 +20,6 @@
> void hmp_info_name(Monitor *mon);
> void hmp_eject(Monitor *mon, const QDict *args);
> void hmp_block_passwd(Monitor *mon, const QDict *qdict);
> +void hmp_change(Monitor *mon, const QDict *qdict);
>
> #endif
> diff --git a/monitor.c b/monitor.c
> index 5cb36cd..9801a2d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1005,78 +1005,22 @@ static int do_quit(Monitor *mon, const QDict *qdict,
> QObject **ret_data)
> return 0;
> }
>
> -#ifdef CONFIG_VNC
> -static int change_vnc_password(const char *password)
> -{
> - if (!password || !password[0]) {
> - if (vnc_display_disable_login(NULL)) {
> - qerror_report(QERR_SET_PASSWD_FAILED);
> - return -1;
> - }
> - return 0;
> - }
> -
> - if (vnc_display_password(NULL, password) < 0) {
> - qerror_report(QERR_SET_PASSWD_FAILED);
> - return -1;
> - }
> -
> - return 0;
> -}
> -
> -static void change_vnc_password_cb(Monitor *mon, const char *password,
> - void *opaque)
> +void qmp_change(const char *device, const char *target,
> + bool has_arg, const char *arg, Error **err)
> {
> - change_vnc_password(password);
> - monitor_read_command(mon, 1);
> -}
> -
> -static int do_change_vnc(Monitor *mon, const char *target, const char *arg)
> -{
> - if (strcmp(target, "passwd") == 0 ||
> - strcmp(target, "password") == 0) {
> - if (arg) {
> - char password[9];
> - strncpy(password, arg, sizeof(password));
> - password[sizeof(password) - 1] = '\0';
> - return change_vnc_password(password);
> + if (strcmp(device, "vnc") == 0) {
> + if (strcmp(target, "passwd") == 0 || strcmp(target, "password") ==
> 0) {
> + if (!has_arg || !arg[0]) {
> + vnc_display_disable_login(NULL);
> + } else {
> + qmp_change_vnc_password(arg, err);
> + }
> } else {
> - return monitor_read_password(mon, change_vnc_password_cb, NULL);
> + qmp_change_vnc_listen(target, err);
> }
> } else {
> - if (vnc_display_open(NULL, target) < 0) {
> - qerror_report(QERR_VNC_SERVER_FAILED, target);
> - return -1;
> - }
> + deprecated_qmp_change_blockdev(device, target, has_arg, arg, err);
> }
> -
> - return 0;
> -}
> -#else
> -static int do_change_vnc(Monitor *mon, const char *target, const char *arg)
> -{
> - qerror_report(QERR_FEATURE_DISABLED, "vnc");
> - return -ENODEV;
> -}
> -#endif
> -
> -/**
> - * do_change(): Change a removable medium, or VNC configuration
> - */
> -static int do_change(Monitor *mon, const QDict *qdict, QObject **ret_data)
> -{
> - const char *device = qdict_get_str(qdict, "device");
> - const char *target = qdict_get_str(qdict, "target");
> - const char *arg = qdict_get_try_str(qdict, "arg");
> - int ret;
> -
> - if (strcmp(device, "vnc") == 0) {
> - ret = do_change_vnc(mon, target, arg);
> - } else {
> - ret = do_change_block(mon, device, target, arg);
> - }
> -
> - return ret;
> }
>
> static int set_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 139c6e3..f108d20 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -84,6 +84,44 @@
> 'data': {'device': 'str', 'password': 'str'} }
>
> ##
> +# @change:
> +#
> +# This command is multiple commands multiplexed together. Generally
> speaking,
> +# it should not be used in favor of the single purpose alternatives such as
> +# @change-vnc-listen, @change-vnc-password, and @change-blockdev.
> +#
> +# @device: This is normally the name of a block device but it may also be
> 'vnc'.
> +# when it's 'vnc', then sub command depends on @target
> +#
> +# @target: If @device is a block device, then this is the new filename.
> +# If @device is 'vnc', then if the value 'password' selects the vnc
> +# change password command. Otherwise, this specifies a new server
> URI
> +# address to listen to for VNC connections.
> +#
> +# @arg: If @device is a block device, then this is an optional format to
> open
> +# the device with.
> +# If @device is 'vnc' and @target is 'password', this is the new VNC
> +# password to set. If this argument is an empty string, then no
> future
> +# logins will be allowed.
> +#
> +# Returns: Nothing on success.
> +# If @device is not a valid block device, DeviceNotFound
> +# If @format is not a valid block format, InvalidBlockFormat
> +# If the new block device is encrypted, DeviceEncrypted. Note that
> +# if this error is returned, the device has been opened successfully
> +# and an additional call to @block_passwd is required to set the
> +# device's password. The behavior of reads and writes to the block
> +# device between when these calls are executed is undefined.
> +#
> +# Notes: It is strongly recommended that this interface is not used
> especially
> +# for changing block devices.
I'd drop the 'especially' part.
> +#
> +# Since: 0.14.0
> +##
> +{ 'command': 'change',
> + 'data': {'device': 'str', 'target': 'str', '*arg': 'str'} }
> +
> +##
> # @set-blockdev-password:
> #
> # Alias for @block_passwd.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index cec7135..89b8d00 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -117,8 +117,7 @@ EQMP
> .args_type = "device:B,target:F,arg:s?",
> .params = "device filename [format]",
> .help = "change a removable medium, optional format",
> - .user_print = monitor_user_noop,
> - .mhandler.cmd_new = do_change,
> + .mhandler.cmd_new = qmp_marshal_input_change,
> },
>
> {
