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

[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

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

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

[Markus Armbruster]

[Note cc: Eric & Luiz, because this also relates to QMP]

Benoît Canet <benoit.canet@irqsave.net> writes:

> 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.

Needs elaboration.  Let me try.

Currently, a BlockDriverState (short: BDS) has a device_name only when
it's a root of a BDS tree.  These roots are the drives defined with
-drive & friends.  There's code relying on "device_name implies root,
and vice versa".

We're working on giving the user much more control over how block
drivers are combined into backends.  One aspect of that is we need a way
for users to refer to a BDS.  Elsewhere in QEMU, we let users tack IDs
to stuff they create.  The new node_name suggested by Benoît is exactly
that.

Another aspect is that we're going to create a BlockBackend (short: BB)
object that captures the stuff now in BDS that is really about the whole
backend (and thus is used only in root BDSes now).

Semantically, device_name belongs to the whole backend, so it should
move into BB.

So far, QMP commands identify the object to be worked on by its
device_name, typically as parameter "device".  Consequently, they can
only work on roots now.

This is just fine for QMP commands that want to work on a backend.

It's not fine for QMP commands that want to work on an arbitrary,
possibly non-root BDS.

The first proposal is to add another parameter, say "id".  Users can
then refer either to an arbitrary BDS by "id", or (for backward
compatibility) to the root BDS by "device".  When the code sees
"device", it'll look up the BB, then fetch its root BDS.

CON: Existing parameter "device" becomes compatibility cruft.

PRO: Clean and obvious semantics (in my opinion).

The second proposal is to press the existing parameter "device" into
service for referring to BDS node_name.

To keep backward compatibility, we obviously need to ensure that
whenever the old code accepts a value of "device", the new code accepts
it as well, and both resolve it to the same BDS.

What about situations where the old code rejects a value of "device"?
The new code may resolve it to a non-root BDS that happens to have that
ID...

What about dynamic reconfiguration changing the root?  Example: a
synchronous snapshot splices in a QCOW2, which becomes the new root.  In
the current code, device_name refers to the new root.  Wouldn't that
require the BDS ID of the old root moves to the new root?  But that
would mean BDS IDs change!

To be honest, this scares me.  I've been burned by convenience magic and
compatibility magic often enough already.

> My personnal suggestion would be that non specified node-name would be set to
> "undefined" meaning that no operation could occur on this bs.

Yes, that's how IDs work elsewhere.

> For QMP access the device_name is accessed via bdrv_find() in a few place in
> blockdev.
>
> Here are the occurences of it:

[QMP and HMP code using bdrv_find() snipped]

I think we should review with the QMP schema first, code second.

> 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 ?

Should be obvious by now :)

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1927 bytes --]

On 10/30/2013 07:49 AM, Markus Armbruster wrote:

> 
> The first proposal is to add another parameter, say "id".  Users can
> then refer either to an arbitrary BDS by "id", or (for backward
> compatibility) to the root BDS by "device".  When the code sees
> "device", it'll look up the BB, then fetch its root BDS.
> 
> CON: Existing parameter "device" becomes compatibility cruft.
> 
> PRO: Clean and obvious semantics (in my opinion).

I like this one as well.

> 
> The second proposal is to press the existing parameter "device" into
> service for referring to BDS node_name.
> 
> To keep backward compatibility, we obviously need to ensure that
> whenever the old code accepts a value of "device", the new code accepts
> it as well, and both resolve it to the same BDS.
> 
> What about situations where the old code rejects a value of "device"?
> The new code may resolve it to a non-root BDS that happens to have that
> ID...
> 
> What about dynamic reconfiguration changing the root?  Example: a
> synchronous snapshot splices in a QCOW2, which becomes the new root.  In
> the current code, device_name refers to the new root.  Wouldn't that
> require the BDS ID of the old root moves to the new root?  But that
> would mean BDS IDs change!

Having device_name tied to the BB, and ID tied to each BDS, seems much
more workable in the long run.

>> My personnal suggestion would be that non specified node-name would be set to
>> "undefined" meaning that no operation could occur on this bs.
> 
> Yes, that's how IDs work elsewhere.

Agreed.

> [QMP and HMP code using bdrv_find() snipped]
> 
> I think we should review with the QMP schema first, code second.

Yes, get the interface right, and then it's easier to review the code
that implements the interface.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Luiz Capitulino]

On Wed, 30 Oct 2013 13:25:35 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 10/30/2013 07:49 AM, Markus Armbruster wrote:
> 
> > 
> > The first proposal is to add another parameter, say "id".  Users can
> > then refer either to an arbitrary BDS by "id", or (for backward
> > compatibility) to the root BDS by "device".  When the code sees
> > "device", it'll look up the BB, then fetch its root BDS.
> > 
> > CON: Existing parameter "device" becomes compatibility cruft.
> > 
> > PRO: Clean and obvious semantics (in my opinion).
> 
> I like this one as well.

Does this proposal makes "device" optional for existing commands? If it
does then I'm afraid it breaks compatibility because if you don't
specify a device you'll get an error today.

Have you considered adding new commands instead?

> > I think we should review with the QMP schema first, code second.
> 
> Yes, get the interface right, and then it's easier to review the code
> that implements the interface.

Agreed.

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2187 bytes --]

On 11/01/2013 08:51 AM, Luiz Capitulino wrote:
> On Wed, 30 Oct 2013 13:25:35 -0600
> Eric Blake <eblake@redhat.com> wrote:
> 
>> On 10/30/2013 07:49 AM, Markus Armbruster wrote:
>>
>>>
>>> The first proposal is to add another parameter, say "id".  Users can
>>> then refer either to an arbitrary BDS by "id", or (for backward
>>> compatibility) to the root BDS by "device".  When the code sees
>>> "device", it'll look up the BB, then fetch its root BDS.
>>>
>>> CON: Existing parameter "device" becomes compatibility cruft.
>>>
>>> PRO: Clean and obvious semantics (in my opinion).
>>
>> I like this one as well.
> 
> Does this proposal makes "device" optional for existing commands? If it
> does then I'm afraid it breaks compatibility because if you don't
> specify a device you'll get an error today.

Changing from error to success is not backwards-incompatible.  Old
applications will ALWAYS supply device (because it used to be
mandatory).  That is, a management application that was intentionally
omitting 'device' and expecting an error is so unlikely to exist that we
can consider such a management app as buggy.

For more examples of conversion from error to success, consider the
'block-commit' command.  As introduced in 1.3, we did not yet have the
implementation to commit the current image.  But we designed the command
with a view to the future (which we are nearly at, by the way, although
I don't know if it will make 1.7 or be delayed to 1.8).  In fact, we
specifically made the 'top' argument mandatory at the time, and
documented that if 'top' was the active layer that the command would
fail; but with the full intent of removing the error and instead
succeeding once we implement full commit; we also discussed the
possibility in the 1.3 time-frame that 'top' could be made optional once
block-commit could manage the current image.

> 
> Have you considered adding new commands instead?

I'm still not convinced we need new commands yet.  But again, proposing
the QMP schema first will make that clearer.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Luiz Capitulino]

On Fri, 01 Nov 2013 08:59:20 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 11/01/2013 08:51 AM, Luiz Capitulino wrote:
> > On Wed, 30 Oct 2013 13:25:35 -0600
> > Eric Blake <eblake@redhat.com> wrote:
> > 
> >> On 10/30/2013 07:49 AM, Markus Armbruster wrote:
> >>
> >>>
> >>> The first proposal is to add another parameter, say "id".  Users can
> >>> then refer either to an arbitrary BDS by "id", or (for backward
> >>> compatibility) to the root BDS by "device".  When the code sees
> >>> "device", it'll look up the BB, then fetch its root BDS.
> >>>
> >>> CON: Existing parameter "device" becomes compatibility cruft.
> >>>
> >>> PRO: Clean and obvious semantics (in my opinion).
> >>
> >> I like this one as well.
> > 
> > Does this proposal makes "device" optional for existing commands? If it
> > does then I'm afraid it breaks compatibility because if you don't
> > specify a device you'll get an error today.
> 
> Changing from error to success is not backwards-incompatible.  Old
> applications will ALWAYS supply device (because it used to be
> mandatory).  That is, a management application that was intentionally
> omitting 'device' and expecting an error is so unlikely to exist that we
> can consider such a management app as buggy.

Doing such changes makes me nervous nevertheless. In my mind a stable
API doesn't change. Of course that there might exceptions, but 99.9%
of those exceptions should be bug fixes not deliberate API extensions.

A more compelling argument against it is the quality of the resulting
command. I'm sure it's going to be anything but a simple, clean API.

Anyways, let's wait for a concrete proposal to have more concrete
feedback.

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

[Stefan Hajnoczi]

On Wed, Oct 30, 2013 at 02:49:32PM +0100, Markus Armbruster wrote:
> The first proposal is to add another parameter, say "id".  Users can
> then refer either to an arbitrary BDS by "id", or (for backward
> compatibility) to the root BDS by "device".  When the code sees
> "device", it'll look up the BB, then fetch its root BDS.
> 
> CON: Existing parameter "device" becomes compatibility cruft.
> 
> PRO: Clean and obvious semantics (in my opinion).

This proposal gets my vote.

> The second proposal is to press the existing parameter "device" into
> service for referring to BDS node_name.
> 
> To keep backward compatibility, we obviously need to ensure that
> whenever the old code accepts a value of "device", the new code accepts
> it as well, and both resolve it to the same BDS.

Different legacy commands given the same device name might need to
operate on different nodes.  Dynamic renaming does not solve this
problem, so I'm not convinced we can always choose a device name
matching a node name.

Device name commands are higher-level than graph node commands.  For
example, block_set_io_throttle makes sense on a device but less sense on
a graph node, unless we add the implicit assumption that the new
throttling node is created on top of the given node or updated in place
if the throttling node already exists (!!).

Stefan

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

[Fam Zheng]

On 11/04/2013 05:31 PM, Stefan Hajnoczi wrote:
> On Wed, Oct 30, 2013 at 02:49:32PM +0100, Markus Armbruster wrote:
>> The first proposal is to add another parameter, say "id".  Users can
>> then refer either to an arbitrary BDS by "id", or (for backward
>> compatibility) to the root BDS by "device".  When the code sees
>> "device", it'll look up the BB, then fetch its root BDS.
>>
>> CON: Existing parameter "device" becomes compatibility cruft.
>>
>> PRO: Clean and obvious semantics (in my opinion).
> This proposal gets my vote.
>
>> The second proposal is to press the existing parameter "device" into
>> service for referring to BDS node_name.
>>
>> To keep backward compatibility, we obviously need to ensure that
>> whenever the old code accepts a value of "device", the new code accepts
>> it as well, and both resolve it to the same BDS.
> Different legacy commands given the same device name might need to
> operate on different nodes.
Could you give an example for this?


> Dynamic renaming does not solve this
> problem, so I'm not convinced we can always choose a device name
> matching a node name.
>
> Device name commands are higher-level than graph node commands.  For
> example, block_set_io_throttle makes sense on a device but less sense on
> a graph node, unless we add the implicit assumption that the new
> throttling node is created on top of the given node or updated in place
> if the throttling node already exists (!!).
Throttling a node could be useful too, for example if we want to 
throttle backing_hd which is on shared storage, but not to throttle on 
the local image.

My ignorant question is: Why can't we just use one namespace, make sure 
no name collision between node_name and device_name, or even just drop 
device_name, so we treat the root node's node_name as device_name? For 
commands that only accept a device, this can be enforced in its 
implementation by checking against the whole graph to verify this.

Fam

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

[Kevin Wolf]

Am 04.11.2013 um 10:48 hat Fam Zheng geschrieben:
> 
> On 11/04/2013 05:31 PM, Stefan Hajnoczi wrote:
> >On Wed, Oct 30, 2013 at 02:49:32PM +0100, Markus Armbruster wrote:
> >>The first proposal is to add another parameter, say "id".  Users can
> >>then refer either to an arbitrary BDS by "id", or (for backward
> >>compatibility) to the root BDS by "device".  When the code sees
> >>"device", it'll look up the BB, then fetch its root BDS.
> >>
> >>CON: Existing parameter "device" becomes compatibility cruft.
> >>
> >>PRO: Clean and obvious semantics (in my opinion).
> >This proposal gets my vote.
> >
> >>The second proposal is to press the existing parameter "device" into
> >>service for referring to BDS node_name.
> >>
> >>To keep backward compatibility, we obviously need to ensure that
> >>whenever the old code accepts a value of "device", the new code accepts
> >>it as well, and both resolve it to the same BDS.
> >Different legacy commands given the same device name might need to
> >operate on different nodes.
> Could you give an example for this?
> 
> 
> >Dynamic renaming does not solve this
> >problem, so I'm not convinced we can always choose a device name
> >matching a node name.
> >
> >Device name commands are higher-level than graph node commands.  For
> >example, block_set_io_throttle makes sense on a device but less sense on
> >a graph node, unless we add the implicit assumption that the new
> >throttling node is created on top of the given node or updated in place
> >if the throttling node already exists (!!).
> Throttling a node could be useful too, for example if we want to
> throttle backing_hd which is on shared storage, but not to throttle
> on the local image.
> 
> My ignorant question is: Why can't we just use one namespace, make
> sure no name collision between node_name and device_name, or even
> just drop device_name, so we treat the root node's node_name as
> device_name? For commands that only accept a device, this can be
> enforced in its implementation by checking against the whole graph
> to verify this.

Markus described it somewhere in this thread: Live snapshots.
Currently, the device_name moves to the new BDS on the top (and
compatibility requires us to keep it that way), whereas a node name
should, of course, stay at its node.

When you consider this, the single namespace, as much as I would have
loved it, is pretty much dead.

Kevin

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

[Kevin Wolf]

Am 01.11.2013 um 16:12 hat Luiz Capitulino geschrieben:
> On Fri, 01 Nov 2013 08:59:20 -0600
> Eric Blake <eblake@redhat.com> wrote:
> 
> > On 11/01/2013 08:51 AM, Luiz Capitulino wrote:
> > > On Wed, 30 Oct 2013 13:25:35 -0600
> > > Eric Blake <eblake@redhat.com> wrote:
> > > 
> > >> On 10/30/2013 07:49 AM, Markus Armbruster wrote:
> > >>
> > >>>
> > >>> The first proposal is to add another parameter, say "id".  Users can
> > >>> then refer either to an arbitrary BDS by "id", or (for backward
> > >>> compatibility) to the root BDS by "device".  When the code sees
> > >>> "device", it'll look up the BB, then fetch its root BDS.
> > >>>
> > >>> CON: Existing parameter "device" becomes compatibility cruft.
> > >>>
> > >>> PRO: Clean and obvious semantics (in my opinion).
> > >>
> > >> I like this one as well.
> > > 
> > > Does this proposal makes "device" optional for existing commands? If it
> > > does then I'm afraid it breaks compatibility because if you don't
> > > specify a device you'll get an error today.
> > 
> > Changing from error to success is not backwards-incompatible.  Old
> > applications will ALWAYS supply device (because it used to be
> > mandatory).  That is, a management application that was intentionally
> > omitting 'device' and expecting an error is so unlikely to exist that we
> > can consider such a management app as buggy.
> 
> Doing such changes makes me nervous nevertheless. In my mind a stable
> API doesn't change. Of course that there might exceptions, but 99.9%
> of those exceptions should be bug fixes not deliberate API extensions.

Stable API means that whatever was working before keeps working. Nothing
less, but also nothing more.

If an extension that changes from error to success is breaking the API,
adding a new QMP command is breaking the API as well. Because sending an
unknown command means returning an error...

> A more compelling argument against it is the quality of the resulting
> command. I'm sure it's going to be anything but a simple, clean API.

I consider one command with an argument made optional a higher quality
API than having two different commands that do almost the same, except
that the older one has a mandatory argument where the newer one has an
optional argument.

Kevin

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

[Fam Zheng]

On 11/04/2013 07:06 PM, Kevin Wolf wrote:
> Am 04.11.2013 um 10:48 hat Fam Zheng geschrieben:
>> On 11/04/2013 05:31 PM, Stefan Hajnoczi wrote:
>>> On Wed, Oct 30, 2013 at 02:49:32PM +0100, Markus Armbruster wrote:
>>>> The first proposal is to add another parameter, say "id".  Users can
>>>> then refer either to an arbitrary BDS by "id", or (for backward
>>>> compatibility) to the root BDS by "device".  When the code sees
>>>> "device", it'll look up the BB, then fetch its root BDS.
>>>>
>>>> CON: Existing parameter "device" becomes compatibility cruft.
>>>>
>>>> PRO: Clean and obvious semantics (in my opinion).
>>> This proposal gets my vote.
>>>
>>>> The second proposal is to press the existing parameter "device" into
>>>> service for referring to BDS node_name.
>>>>
>>>> To keep backward compatibility, we obviously need to ensure that
>>>> whenever the old code accepts a value of "device", the new code accepts
>>>> it as well, and both resolve it to the same BDS.
>>> Different legacy commands given the same device name might need to
>>> operate on different nodes.
>> Could you give an example for this?
>>
>>
>>> Dynamic renaming does not solve this
>>> problem, so I'm not convinced we can always choose a device name
>>> matching a node name.
>>>
>>> Device name commands are higher-level than graph node commands.  For
>>> example, block_set_io_throttle makes sense on a device but less sense on
>>> a graph node, unless we add the implicit assumption that the new
>>> throttling node is created on top of the given node or updated in place
>>> if the throttling node already exists (!!).
>> Throttling a node could be useful too, for example if we want to
>> throttle backing_hd which is on shared storage, but not to throttle
>> on the local image.
>>
>> My ignorant question is: Why can't we just use one namespace, make
>> sure no name collision between node_name and device_name, or even
>> just drop device_name, so we treat the root node's node_name as
>> device_name? For commands that only accept a device, this can be
>> enforced in its implementation by checking against the whole graph
>> to verify this.
> Markus described it somewhere in this thread: Live snapshots.
> Currently, the device_name moves to the new BDS on the top (and
> compatibility requires us to keep it that way), whereas a node name
> should, of course, stay at its node.
>
> When you consider this, the single namespace, as much as I would have
> loved it, is pretty much dead.

Thanks for explaining (again). I get the reason now.

Fam

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

[Luiz Capitulino]

On Mon, 4 Nov 2013 12:13:59 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 01.11.2013 um 16:12 hat Luiz Capitulino geschrieben:
> > On Fri, 01 Nov 2013 08:59:20 -0600
> > Eric Blake <eblake@redhat.com> wrote:
> > 
> > > On 11/01/2013 08:51 AM, Luiz Capitulino wrote:
> > > > On Wed, 30 Oct 2013 13:25:35 -0600
> > > > Eric Blake <eblake@redhat.com> wrote:
> > > > 
> > > >> On 10/30/2013 07:49 AM, Markus Armbruster wrote:
> > > >>
> > > >>>
> > > >>> The first proposal is to add another parameter, say "id".  Users can
> > > >>> then refer either to an arbitrary BDS by "id", or (for backward
> > > >>> compatibility) to the root BDS by "device".  When the code sees
> > > >>> "device", it'll look up the BB, then fetch its root BDS.
> > > >>>
> > > >>> CON: Existing parameter "device" becomes compatibility cruft.
> > > >>>
> > > >>> PRO: Clean and obvious semantics (in my opinion).
> > > >>
> > > >> I like this one as well.
> > > > 
> > > > Does this proposal makes "device" optional for existing commands? If it
> > > > does then I'm afraid it breaks compatibility because if you don't
> > > > specify a device you'll get an error today.
> > > 
> > > Changing from error to success is not backwards-incompatible.  Old
> > > applications will ALWAYS supply device (because it used to be
> > > mandatory).  That is, a management application that was intentionally
> > > omitting 'device' and expecting an error is so unlikely to exist that we
> > > can consider such a management app as buggy.
> > 
> > Doing such changes makes me nervous nevertheless. In my mind a stable
> > API doesn't change. Of course that there might exceptions, but 99.9%
> > of those exceptions should be bug fixes not deliberate API extensions.
> 
> Stable API means that whatever was working before keeps working. Nothing
> less, but also nothing more.
> 
> If an extension that changes from error to success is breaking the API,
> adding a new QMP command is breaking the API as well. Because sending an
> unknown command means returning an error...

Let's not turn this into a surreal discussion, I'm sure we all want the
best for our users.

There's another problem, btw. We're not doing command extensions for
input arguments before heaving a way to discover them. Even if there's
consensus that extending the command is okay, we need the query-qmp-schema
command merged first. IMO, this a blocker requirement (although it
shouldn't be difficult to finalize what has been posted already).

> > A more compelling argument against it is the quality of the resulting
> > command. I'm sure it's going to be anything but a simple, clean API.
> 
> I consider one command with an argument made optional a higher quality
> API than having two different commands that do almost the same, except
> that the older one has a mandatory argument where the newer one has an
> optional argument.

I wouldn't expect the new command to be just a duplication of the old
one with a new argument. I'm expecting it to have a one-cover all
argument or to have a dict or something that makes more sense for
the new capabilities. But yes, we need a schema example to see how it would
look like.

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

[Benoît Canet]

Le Monday 04 Nov 2013 à 19:33:21 (+0800), Fam Zheng a écrit :
> 
> On 11/04/2013 07:06 PM, Kevin Wolf wrote:
> >Am 04.11.2013 um 10:48 hat Fam Zheng geschrieben:
> >>On 11/04/2013 05:31 PM, Stefan Hajnoczi wrote:
> >>>On Wed, Oct 30, 2013 at 02:49:32PM +0100, Markus Armbruster wrote:
> >>>>The first proposal is to add another parameter, say "id".  Users can
> >>>>then refer either to an arbitrary BDS by "id", or (for backward
> >>>>compatibility) to the root BDS by "device".  When the code sees
> >>>>"device", it'll look up the BB, then fetch its root BDS.
> >>>>
> >>>>CON: Existing parameter "device" becomes compatibility cruft.
> >>>>
> >>>>PRO: Clean and obvious semantics (in my opinion).
> >>>This proposal gets my vote.
> >>>
> >>>>The second proposal is to press the existing parameter "device" into
> >>>>service for referring to BDS node_name.
> >>>>
> >>>>To keep backward compatibility, we obviously need to ensure that
> >>>>whenever the old code accepts a value of "device", the new code accepts
> >>>>it as well, and both resolve it to the same BDS.
> >>>Different legacy commands given the same device name might need to
> >>>operate on different nodes.
> >>Could you give an example for this?
> >>
> >>
> >>>Dynamic renaming does not solve this
> >>>problem, so I'm not convinced we can always choose a device name
> >>>matching a node name.
> >>>
> >>>Device name commands are higher-level than graph node commands.  For
> >>>example, block_set_io_throttle makes sense on a device but less sense on
> >>>a graph node, unless we add the implicit assumption that the new
> >>>throttling node is created on top of the given node or updated in place
> >>>if the throttling node already exists (!!).
> >>Throttling a node could be useful too, for example if we want to
> >>throttle backing_hd which is on shared storage, but not to throttle
> >>on the local image.
> >>
> >>My ignorant question is: Why can't we just use one namespace, make
> >>sure no name collision between node_name and device_name, or even
> >>just drop device_name, so we treat the root node's node_name as
> >>device_name? For commands that only accept a device, this can be
> >>enforced in its implementation by checking against the whole graph
> >>to verify this.
> >Markus described it somewhere in this thread: Live snapshots.
> >Currently, the device_name moves to the new BDS on the top (and
> >compatibility requires us to keep it that way), whereas a node name
> >should, of course, stay at its node.
> >
> >When you consider this, the single namespace, as much as I would have
> >loved it, is pretty much dead.

Good everyone agree on the direction to take.
I'll write some code.

Best regards

Benoît

> 
> Thanks for explaining (again). I get the reason now.
> 
> Fam
>