[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 09/13] monitor/hmp: move remaining hmp_block* functions to
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [PATCH v3 09/13] monitor/hmp: move remaining hmp_block* functions to block-hmp-cmds.c |
Date: |
Tue, 28 Jan 2020 18:51:57 +0000 |
User-agent: |
Mutt/1.13.3 (2020-01-12) |
* Maxim Levitsky (address@hidden) wrote:
> Signed-off-by: Maxim Levitsky <address@hidden>
Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> ---
> block/monitor/block-hmp-cmds.c | 138 +++++++++++++++++++++++++++++
> include/block/block-hmp-commands.h | 9 ++
> include/monitor/hmp.h | 6 --
> monitor/hmp-cmds.c | 137 ----------------------------
> 4 files changed, 147 insertions(+), 143 deletions(-)
>
> diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
> index df0178d0f9..60d63bfe18 100644
> --- a/block/monitor/block-hmp-cmds.c
> +++ b/block/monitor/block-hmp-cmds.c
> @@ -29,6 +29,7 @@
> #include "block/block_int.h"
> #include "block/block-hmp-commands.h"
> #include "monitor/hmp.h"
> +#include "qemu-io.h"
>
> void hmp_drive_add(Monitor *mon, const QDict *qdict)
> {
> @@ -415,3 +416,140 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict
> *qdict)
> qmp_nbd_server_stop(&err);
> hmp_handle_error(mon, err);
> }
> +
> +void hmp_block_resize(Monitor *mon, const QDict *qdict)
> +{
> + const char *device = qdict_get_str(qdict, "device");
> + int64_t size = qdict_get_int(qdict, "size");
> + Error *err = NULL;
> +
> + qmp_block_resize(true, device, false, NULL, size, &err);
> + hmp_handle_error(mon, err);
> +}
> +
> +void hmp_block_stream(Monitor *mon, const QDict *qdict)
> +{
> + Error *error = NULL;
> + const char *device = qdict_get_str(qdict, "device");
> + const char *base = qdict_get_try_str(qdict, "base");
> + int64_t speed = qdict_get_try_int(qdict, "speed", 0);
> +
> + qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
> + false, NULL, qdict_haskey(qdict, "speed"), speed, true,
> + BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
> + &error);
> +
> + hmp_handle_error(mon, error);
> +}
> +
> +void hmp_block_passwd(Monitor *mon, const QDict *qdict)
> +{
> + const char *device = qdict_get_str(qdict, "device");
> + const char *password = qdict_get_str(qdict, "password");
> + Error *err = NULL;
> +
> + qmp_block_passwd(true, device, false, NULL, password, &err);
> + hmp_handle_error(mon, err);
> +}
> +
> +void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
> +{
> + Error *err = NULL;
> + char *device = (char *) qdict_get_str(qdict, "device");
> + BlockIOThrottle throttle = {
> + .bps = qdict_get_int(qdict, "bps"),
> + .bps_rd = qdict_get_int(qdict, "bps_rd"),
> + .bps_wr = qdict_get_int(qdict, "bps_wr"),
> + .iops = qdict_get_int(qdict, "iops"),
> + .iops_rd = qdict_get_int(qdict, "iops_rd"),
> + .iops_wr = qdict_get_int(qdict, "iops_wr"),
> + };
> +
> + /* qmp_block_set_io_throttle has separate parameters for the
> + * (deprecated) block device name and the qdev ID but the HMP
> + * version has only one, so we must decide which one to pass. */
> + if (blk_by_name(device)) {
> + throttle.has_device = true;
> + throttle.device = device;
> + } else {
> + throttle.has_id = true;
> + throttle.id = device;
> + }
> +
> + qmp_block_set_io_throttle(&throttle, &err);
> + hmp_handle_error(mon, err);
> +}
> +
> +void hmp_eject(Monitor *mon, const QDict *qdict)
> +{
> + bool force = qdict_get_try_bool(qdict, "force", false);
> + const char *device = qdict_get_str(qdict, "device");
> + Error *err = NULL;
> +
> + qmp_eject(true, device, false, NULL, true, force, &err);
> + hmp_handle_error(mon, err);
> +}
> +
> +void hmp_qemu_io(Monitor *mon, const QDict *qdict)
> +{
> + BlockBackend *blk;
> + BlockBackend *local_blk = NULL;
> + bool qdev = qdict_get_try_bool(qdict, "qdev", false);
> + const char* device = qdict_get_str(qdict, "device");
> + const char* command = qdict_get_str(qdict, "command");
> + Error *err = NULL;
> + int ret;
> +
> + if (qdev) {
> + blk = blk_by_qdev_id(device, &err);
> + if (!blk) {
> + goto fail;
> + }
> + } else {
> + blk = blk_by_name(device);
> + if (!blk) {
> + BlockDriverState *bs = bdrv_lookup_bs(NULL, device, &err);
> + if (bs) {
> + blk = local_blk = blk_new(bdrv_get_aio_context(bs),
> + 0, BLK_PERM_ALL);
> + ret = blk_insert_bs(blk, bs, &err);
> + if (ret < 0) {
> + goto fail;
> + }
> + } else {
> + goto fail;
> + }
> + }
> + }
> +
> + /*
> + * Notably absent: Proper permission management. This is sad, but it
> seems
> + * almost impossible to achieve without changing the semantics and
> thereby
> + * limiting the use cases of the qemu-io HMP command.
> + *
> + * In an ideal world we would unconditionally create a new BlockBackend
> for
> + * qemuio_command(), but we have commands like 'reopen' and want them to
> + * take effect on the exact BlockBackend whose name the user passed
> instead
> + * of just on a temporary copy of it.
> + *
> + * Another problem is that deleting the temporary BlockBackend involves
> + * draining all requests on it first, but some qemu-iotests cases want to
> + * issue multiple aio_read/write requests and expect them to complete in
> + * the background while the monitor has already returned.
> + *
> + * This is also what prevents us from saving the original permissions and
> + * restoring them later: We can't revoke permissions until all requests
> + * have completed, and we don't know when that is nor can we really let
> + * anything else run before we have revoken them to avoid race
> conditions.
> + *
> + * What happens now is that command() in qemu-io-cmds.c can extend the
> + * permissions if necessary for the qemu-io command. And they simply stay
> + * extended, possibly resulting in a read-only guest device keeping write
> + * permissions. Ugly, but it appears to be the lesser evil.
> + */
> + qemuio_command(blk, command);
> +
> +fail:
> + blk_unref(local_blk);
> + hmp_handle_error(mon, err);
> +}
> diff --git a/include/block/block-hmp-commands.h
> b/include/block/block-hmp-commands.h
> index 721b9a1978..99145c8fcf 100644
> --- a/include/block/block-hmp-commands.h
> +++ b/include/block/block-hmp-commands.h
> @@ -26,4 +26,13 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
> void hmp_nbd_server_remove(Monitor *mon, const QDict *qdict);
> void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
>
> +void hmp_block_resize(Monitor *mon, const QDict *qdict);
> +void hmp_block_stream(Monitor *mon, const QDict *qdict);
> +void hmp_block_passwd(Monitor *mon, const QDict *qdict);
> +void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
> +void hmp_eject(Monitor *mon, const QDict *qdict);
> +
> +void hmp_qemu_io(Monitor *mon, const QDict *qdict);
> +
> +
> #endif
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index 736a969131..47a7cad734 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -58,9 +58,7 @@ void hmp_cont(Monitor *mon, const QDict *qdict);
> void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
> void hmp_nmi(Monitor *mon, const QDict *qdict);
> void hmp_set_link(Monitor *mon, const QDict *qdict);
> -void hmp_block_passwd(Monitor *mon, const QDict *qdict);
> void hmp_balloon(Monitor *mon, const QDict *qdict);
> -void hmp_block_resize(Monitor *mon, const QDict *qdict);
> void hmp_loadvm(Monitor *mon, const QDict *qdict);
> void hmp_savevm(Monitor *mon, const QDict *qdict);
> void hmp_delvm(Monitor *mon, const QDict *qdict);
> @@ -80,10 +78,7 @@ void hmp_migrate_start_postcopy(Monitor *mon, const QDict
> *qdict);
> void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict);
> void hmp_set_password(Monitor *mon, const QDict *qdict);
> void hmp_expire_password(Monitor *mon, const QDict *qdict);
> -void hmp_eject(Monitor *mon, const QDict *qdict);
> void hmp_change(Monitor *mon, const QDict *qdict);
> -void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
> -void hmp_block_stream(Monitor *mon, const QDict *qdict);
> void hmp_migrate(Monitor *mon, const QDict *qdict);
> void hmp_device_add(Monitor *mon, const QDict *qdict);
> void hmp_device_del(Monitor *mon, const QDict *qdict);
> @@ -98,7 +93,6 @@ void hmp_chardev_add(Monitor *mon, const QDict *qdict);
> void hmp_chardev_change(Monitor *mon, const QDict *qdict);
> void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
> void hmp_chardev_send_break(Monitor *mon, const QDict *qdict);
> -void hmp_qemu_io(Monitor *mon, const QDict *qdict);
> void hmp_cpu_add(Monitor *mon, const QDict *qdict);
> void hmp_object_add(Monitor *mon, const QDict *qdict);
> void hmp_object_del(Monitor *mon, const QDict *qdict);
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 67d2ca8a4c..c224e0f338 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -46,7 +46,6 @@
> #include "qom/object_interfaces.h"
> #include "ui/console.h"
> #include "block/qapi.h"
> -#include "qemu-io.h"
> #include "qemu/cutils.h"
> #include "qemu/error-report.h"
> #include "exec/ramlist.h"
> @@ -1307,16 +1306,6 @@ void hmp_set_link(Monitor *mon, const QDict *qdict)
> hmp_handle_error(mon, err);
> }
>
> -void hmp_block_passwd(Monitor *mon, const QDict *qdict)
> -{
> - const char *device = qdict_get_str(qdict, "device");
> - const char *password = qdict_get_str(qdict, "password");
> - Error *err = NULL;
> -
> - qmp_block_passwd(true, device, false, NULL, password, &err);
> - hmp_handle_error(mon, err);
> -}
> -
> void hmp_balloon(Monitor *mon, const QDict *qdict)
> {
> int64_t value = qdict_get_int(qdict, "value");
> @@ -1326,16 +1315,6 @@ void hmp_balloon(Monitor *mon, const QDict *qdict)
> hmp_handle_error(mon, err);
> }
>
> -void hmp_block_resize(Monitor *mon, const QDict *qdict)
> -{
> - const char *device = qdict_get_str(qdict, "device");
> - int64_t size = qdict_get_int(qdict, "size");
> - Error *err = NULL;
> -
> - qmp_block_resize(true, device, false, NULL, size, &err);
> - hmp_handle_error(mon, err);
> -}
> -
> void hmp_loadvm(Monitor *mon, const QDict *qdict)
> {
> int saved_vm_running = runstate_is_running();
> @@ -1818,15 +1797,6 @@ void hmp_expire_password(Monitor *mon, const QDict
> *qdict)
> hmp_handle_error(mon, err);
> }
>
> -void hmp_eject(Monitor *mon, const QDict *qdict)
> -{
> - bool force = qdict_get_try_bool(qdict, "force", false);
> - const char *device = qdict_get_str(qdict, "device");
> - Error *err = NULL;
> -
> - qmp_eject(true, device, false, NULL, true, force, &err);
> - hmp_handle_error(mon, err);
> -}
>
> #ifdef CONFIG_VNC
> static void hmp_change_read_arg(void *opaque, const char *password,
> @@ -1884,49 +1854,6 @@ void hmp_change(Monitor *mon, const QDict *qdict)
> hmp_handle_error(mon, err);
> }
>
> -void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
> -{
> - Error *err = NULL;
> - char *device = (char *) qdict_get_str(qdict, "device");
> - BlockIOThrottle throttle = {
> - .bps = qdict_get_int(qdict, "bps"),
> - .bps_rd = qdict_get_int(qdict, "bps_rd"),
> - .bps_wr = qdict_get_int(qdict, "bps_wr"),
> - .iops = qdict_get_int(qdict, "iops"),
> - .iops_rd = qdict_get_int(qdict, "iops_rd"),
> - .iops_wr = qdict_get_int(qdict, "iops_wr"),
> - };
> -
> - /* qmp_block_set_io_throttle has separate parameters for the
> - * (deprecated) block device name and the qdev ID but the HMP
> - * version has only one, so we must decide which one to pass. */
> - if (blk_by_name(device)) {
> - throttle.has_device = true;
> - throttle.device = device;
> - } else {
> - throttle.has_id = true;
> - throttle.id = device;
> - }
> -
> - qmp_block_set_io_throttle(&throttle, &err);
> - hmp_handle_error(mon, err);
> -}
> -
> -void hmp_block_stream(Monitor *mon, const QDict *qdict)
> -{
> - Error *error = NULL;
> - const char *device = qdict_get_str(qdict, "device");
> - const char *base = qdict_get_try_str(qdict, "base");
> - int64_t speed = qdict_get_try_int(qdict, "speed", 0);
> -
> - qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
> - false, NULL, qdict_haskey(qdict, "speed"), speed, true,
> - BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
> - &error);
> -
> - hmp_handle_error(mon, error);
> -}
> -
> typedef struct HMPMigrationStatus
> {
> QEMUTimer *timer;
> @@ -2219,70 +2146,6 @@ void hmp_chardev_send_break(Monitor *mon, const QDict
> *qdict)
> hmp_handle_error(mon, local_err);
> }
>
> -void hmp_qemu_io(Monitor *mon, const QDict *qdict)
> -{
> - BlockBackend *blk;
> - BlockBackend *local_blk = NULL;
> - bool qdev = qdict_get_try_bool(qdict, "qdev", false);
> - const char* device = qdict_get_str(qdict, "device");
> - const char* command = qdict_get_str(qdict, "command");
> - Error *err = NULL;
> - int ret;
> -
> - if (qdev) {
> - blk = blk_by_qdev_id(device, &err);
> - if (!blk) {
> - goto fail;
> - }
> - } else {
> - blk = blk_by_name(device);
> - if (!blk) {
> - BlockDriverState *bs = bdrv_lookup_bs(NULL, device, &err);
> - if (bs) {
> - blk = local_blk = blk_new(bdrv_get_aio_context(bs),
> - 0, BLK_PERM_ALL);
> - ret = blk_insert_bs(blk, bs, &err);
> - if (ret < 0) {
> - goto fail;
> - }
> - } else {
> - goto fail;
> - }
> - }
> - }
> -
> - /*
> - * Notably absent: Proper permission management. This is sad, but it
> seems
> - * almost impossible to achieve without changing the semantics and
> thereby
> - * limiting the use cases of the qemu-io HMP command.
> - *
> - * In an ideal world we would unconditionally create a new BlockBackend
> for
> - * qemuio_command(), but we have commands like 'reopen' and want them to
> - * take effect on the exact BlockBackend whose name the user passed
> instead
> - * of just on a temporary copy of it.
> - *
> - * Another problem is that deleting the temporary BlockBackend involves
> - * draining all requests on it first, but some qemu-iotests cases want to
> - * issue multiple aio_read/write requests and expect them to complete in
> - * the background while the monitor has already returned.
> - *
> - * This is also what prevents us from saving the original permissions and
> - * restoring them later: We can't revoke permissions until all requests
> - * have completed, and we don't know when that is nor can we really let
> - * anything else run before we have revoken them to avoid race
> conditions.
> - *
> - * What happens now is that command() in qemu-io-cmds.c can extend the
> - * permissions if necessary for the qemu-io command. And they simply stay
> - * extended, possibly resulting in a read-only guest device keeping write
> - * permissions. Ugly, but it appears to be the lesser evil.
> - */
> - qemuio_command(blk, command);
> -
> -fail:
> - blk_unref(local_blk);
> - hmp_handle_error(mon, err);
> -}
> -
> void hmp_object_del(Monitor *mon, const QDict *qdict)
> {
> const char *id = qdict_get_str(qdict, "id");
> --
> 2.17.2
>
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK
- [PATCH v3 06/13] monitor/hmp: move hmp_block_job* to block-hmp-cmds.c, (continued)
- [PATCH v3 06/13] monitor/hmp: move hmp_block_job* to block-hmp-cmds.c, Maxim Levitsky, 2020/01/27
- [PATCH v3 07/13] monitor/hmp: move hmp_snapshot_* to block-hmp-cmds.c, Maxim Levitsky, 2020/01/27
- [PATCH v3 08/13] monitor/hmp: move hmp_nbd_server* to block-hmp-cmds.c, Maxim Levitsky, 2020/01/27
- [PATCH v3 09/13] monitor/hmp: move remaining hmp_block* functions to block-hmp-cmds.c, Maxim Levitsky, 2020/01/27
- [PATCH v3 11/13] monitor: Move hmp_drive_add_node to block-hmp-cmds.c, Maxim Levitsky, 2020/01/27
- [PATCH v3 10/13] monitor/hmp: move hmp_info_block* to block-hmp-cmds.c, Maxim Levitsky, 2020/01/27
- [PATCH v3 13/13] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands, Maxim Levitsky, 2020/01/27
- [PATCH v3 12/13] add 'error' prefix to vreport, Maxim Levitsky, 2020/01/27