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

[Benoît Canet]

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;
}

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 ?

Best regards

Benoît