On Mon, 10/28 16:40, Benoît Canet wrote:
>
> Hi list,
>
> After a discussion on irc we have two potential solution in order to introduce
> a new bs->node_name member in order to be able to manipulate the graph from
> the
> monitors.
>
> The first one is to make the QMP device parameter of the block commands
> optional
> and add the node-name parameter as a second optional parameter.
> This is Markus prefered solution and Eric is ok with making mandatory
> parameters
> optional in QMP.
>
> The second one suggested by Kevin Would be to add some magic to the new
> node_name member by making it equal to device_name for backends and then
> making
> the qmp commands operate only on node-names.
> My personnal suggestion would be that non specified node-name would be set to
> "undefined" meaning that no operation could occur on this bs.
>
> For QMP access the device_name is accessed via bdrv_find() in a few place in
> blockdev.
>
> Here are the occurences of it:
>
> commit
> ------
> void do_commit(Monitor *mon, const QDict *qdict)
> {
> const char *device = qdict_get_str(qdict, "device");
> BlockDriverState *bs;
> int ret;
>
> if (!strcmp(device, "all")) {
> ret = bdrv_commit_all();
> } else {
> bs = bdrv_find(device);
> if (!bs) {
> monitor_printf(mon, "Device '%s' not found\n", device);
> return;
> }
> ret = bdrv_commit(bs);
> }
> if (ret < 0) {
> monitor_printf(mon, "'commit' error for '%s': %s\n", device,
> strerror(-ret));
> }
> }
>
> internal snapshot deletion
> --------------------------
> SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
> bool has_id,
> const char *id,
> bool has_name,
> const char *name,
> Error **errp)
> {
> BlockDriverState *bs = bdrv_find(device);
> QEMUSnapshotInfo sn;
> Error *local_err = NULL;
> SnapshotInfo *info = NULL;
>
>
> Internal snapshot preparation
> -----------------------------
> static void internal_snapshot_prepare(BlkTransactionState *common,
> Error **errp)
> {
> const char *device;
> const char *name;
>
> BlockDriverState *bs;
> QEMUSnapshotInfo old_sn, *sn;
> bool ret;
> qemu_timeval tv;
> BlockdevSnapshotInternal *internal;
> InternalSnapshotState *state;
> int ret1;
>
> g_assert(common->action->kind ==
> TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
> internal = common->action->blockdev_snapshot_internal_sync;
> state = DO_UPCAST(InternalSnapshotState, common, common);
>
> /* 1. parse input */
> device = internal->device;
> name = internal->name;
>
> /* 2. check for validation */
> bs = bdrv_find(device);
> if (!bs) {
> error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> return;
> }
>
> Drive backup
> ------------
> static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
> {
> DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
> DriveBackup *backup;
> Error *local_err = NULL;
>
> assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
> backup = common->action->drive_backup;
>
> qmp_drive_backup(backup->device, backup->target,
> backup->has_format, backup->format,
> backup->sync,
> backup->has_mode, backup->mode,
> backup->has_speed, backup->speed,
> backup->has_on_source_error, backup->on_source_error,
> backup->has_on_target_error, backup->on_target_error,
> &local_err);
> if (error_is_set(&local_err)) {
> error_propagate(errp, local_err);
> state->bs = NULL;
> state->job = NULL;
> return;
> }
>
> state->bs = bdrv_find(backup->device);
> state->job = state->bs->job;
> }
>
> Eject which should operate on backends
> --------------------------------------
> void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
> {
> BlockDriverState *bs;
>
> bs = bdrv_find(device);
> if (!bs) {
> error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> return;
> }
>
> eject_device(bs, force, errp);
> }
>
> QCow2 crypto
> ------------
> void qmp_block_passwd(const char *device, const char *password, Error **errp)
> {
> BlockDriverState *bs;
> int err;
>
> bs = bdrv_find(device);
> if (!bs) {
> error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> return;
> }
>
> err = bdrv_set_key(bs, password);
> if (err == -EINVAL) {
> error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
> return;
> } else if (err < 0) {
> error_set(errp, QERR_INVALID_PASSWORD);
> return;
> }
> }
>
> Change blockdev (I don't know what it is used for)
> --------------------------------------------------
> void qmp_change_blockdev(const char *device, const char *filename,
> bool has_format, const char *format, Error **errp)
> {
> BlockDriverState *bs;
> BlockDriver *drv = NULL;
> int bdrv_flags;
> Error *err = NULL;
>
> bs = bdrv_find(device);
> if (!bs) {
> error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> return;
> }
>
> if (format) {
> drv = bdrv_find_whitelisted_format(format, bs->read_only);
> if (!drv) {
> error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> return;
> }
> }
>
> eject_device(bs, 0, &err);
> if (error_is_set(&err)) {
> error_propagate(errp, err);
> return;
> }
>
> bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
>
> qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);
> }
>
> Throttling
> ----------
> void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t
> bps_rd,
> int64_t bps_wr,
> int64_t iops,
> int64_t iops_rd,
> int64_t iops_wr,
> bool has_bps_max,
> int64_t bps_max,
> bool has_bps_rd_max,
> int64_t bps_rd_max,
> bool has_bps_wr_max,
> int64_t bps_wr_max,
> bool has_iops_max,
> int64_t iops_max,
> bool has_iops_rd_max,
> int64_t iops_rd_max,
> bool has_iops_wr_max,
> int64_t iops_wr_max,
> bool has_iops_size,
> int64_t iops_size, Error **errp)
> {
> ThrottleConfig cfg;
> BlockDriverState *bs;
>
> bs = bdrv_find(device);
> if (!bs) {
> error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> return;
> }
>
> Drive deletion
> --------------
> int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> {
> const char *id = qdict_get_str(qdict, "id");
> BlockDriverState *bs;
>
> bs = bdrv_find(id);
> if (!bs) {
> qerror_report(QERR_DEVICE_NOT_FOUND, id);
> return -1;
> }
>
> block resize
> ------------
> void qmp_block_resize(const char *device, int64_t size, Error **errp)
> {
> BlockDriverState *bs;
> int ret;
>
> bs = bdrv_find(device);
> if (!bs) {
> error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> return;
> }
>
> streaming
> ---------
> void qmp_block_stream(const char *device, bool has_base,
> const char *base, bool has_speed, int64_t speed,
> bool has_on_error, BlockdevOnError on_error,
> Error **errp)
> {
> BlockDriverState *bs;
> BlockDriverState *base_bs = NULL;
> Error *local_err = NULL;
>
> if (!has_on_error) {
> on_error = BLOCKDEV_ON_ERROR_REPORT;
> }
>
> bs = bdrv_find(device);
> if (!bs) {
> error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> return;
> }
>
> commit
> ------
> void qmp_block_commit(const char *device,
> bool has_base, const char *base, const char *top,
> bool has_speed, int64_t speed,
> Error **errp)
> {
> BlockDriverState *bs;
> BlockDriverState *base_bs, *top_bs;
> Error *local_err = NULL;
> /* This will be part of the QMP command, if/when the
> * BlockdevOnError change for blkmirror makes it in
> */
> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>
> /* drain all i/o before commits */
> bdrv_drain_all();
>
> bs = bdrv_find(device);
> if (!bs) {
> error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> return;
> }
>
>
> backup:
> -------
> void qmp_drive_backup(const char *device, const char *target,
> bool has_format, const char *format,
> enum MirrorSyncMode sync,
> bool has_mode, enum NewImageMode mode,
> bool has_speed, int64_t speed,
> bool has_on_source_error, BlockdevOnError
> on_source_error,
> bool has_on_target_error, BlockdevOnError
> on_target_error,
> Error **errp)
> {
> BlockDriverState *bs;
> BlockDriverState *target_bs;
> BlockDriverState *source = NULL;
> BlockDriver *drv = NULL;
> Error *local_err = NULL;
> int flags;
> int64_t size;
> int ret;
>
> if (!has_speed) {
> speed = 0;
> }
> if (!has_on_source_error) {
> on_source_error = BLOCKDEV_ON_ERROR_REPORT;
> }
> if (!has_on_target_error) {
> on_target_error = BLOCKDEV_ON_ERROR_REPORT;
> }
> if (!has_mode) {
> mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> }
>
> bs = bdrv_find(device);
> if (!bs) {
> error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> return;
> }
>
>
> mirror
> ------
> void qmp_drive_mirror(const char *device, const char *target,
> bool has_format, const char *format,
> enum MirrorSyncMode sync,
> bool has_mode, enum NewImageMode mode,
> bool has_speed, int64_t speed,
> bool has_granularity, uint32_t granularity,
> bool has_buf_size, int64_t buf_size,
> bool has_on_source_error, BlockdevOnError
> on_source_error,
> bool has_on_target_error, BlockdevOnError
> on_target_error,
> Error **errp)
> {
> BlockDriverState *bs;
> BlockDriverState *source, *target_bs;
> BlockDriver *drv = NULL;
> Error *local_err = NULL;
> int flags;
> int64_t size;
> int ret;
>
> if (!has_speed) {
> speed = 0;
> }
> if (!has_on_source_error) {
> on_source_error = BLOCKDEV_ON_ERROR_REPORT;
> }
> if (!has_on_target_error) {
> on_target_error = BLOCKDEV_ON_ERROR_REPORT;
> }
> if (!has_mode) {
> mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> }
> if (!has_granularity) {
> granularity = 0;
> }
> if (!has_buf_size) {
> buf_size = DEFAULT_MIRROR_BUF_SIZE;
> }
>
> if (granularity != 0 && (granularity < 512 || granularity > 1048576 *
> 64)) {
> error_set(errp, QERR_INVALID_PARAMETER, device);
> return;
> }
> if (granularity & (granularity - 1)) {
> error_set(errp, QERR_INVALID_PARAMETER, device);
> return;
> }
>
> bs = bdrv_find(device);
> if (!bs) {
> error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> return;
> }
>
> find block job
> --------------
> static BlockJob *find_block_job(const char *device)
> {
> BlockDriverState *bs;
>
> bs = bdrv_find(device);
> if (!bs || !bs->job) {
> return NULL;
> }
> return bs->job;
> }
>
And nbd-server-add.
> Very few of them operate on what is destined to become block backend and most
> of
> them should be able to operate on nodes of the graph;
>
> What solution do you prefer ?
My feeling is not to distinguish node-name and device-name, with the same
reason as above: most of them should be able to operate on nodes of the graph.
So I prefer we don't touch the qmp interface. We can always introduce optional
parameter and make mandatory ones optional, if necessary. But for now, just
equalize node-name and device-name should be neater from a users view. If a
command is only for backend, we can check internally and report error if the
provided node is not.
Thanks,
Fam
