Re: [Qemu-devel] How to introduce bs->node_name ?

[Fam Zheng]

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