From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49196) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vaxih-00038e-AB for qemu-devel@nongnu.org; Mon, 28 Oct 2013 21:03:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vaxib-0007pu-6U for qemu-devel@nongnu.org; Mon, 28 Oct 2013 21:03:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29662) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vaxia-0007oa-Qb for qemu-devel@nongnu.org; Mon, 28 Oct 2013 21:03:37 -0400 Date: Tue, 29 Oct 2013 09:03:27 +0800 From: Fam Zheng Message-ID: <20131029010327.GA30739@T430s.nay.redhat.com> References: <20131028154038.GG2890@irqsave.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20131028154038.GG2890@irqsave.net> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] How to introduce bs->node_name ? Reply-To: famz@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, armbru@redhat.com On Mon, 10/28 16:40, Beno=EEt Canet wrote: >=20 > Hi list, >=20 > After a discussion on irc we have two potential solution in order to in= troduce > a new bs->node_name member in order to be able to manipulate the graph = from the > monitors. >=20 > 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 p= arameters > optional in QMP. >=20 > 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 the= n 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. >=20 > For QMP access the device_name is accessed via bdrv_find() in a few pla= ce in > blockdev. >=20 > Here are the occurences of it: >=20 > commit > ------ > void do_commit(Monitor *mon, const QDict *qdict) > { > const char *device =3D qdict_get_str(qdict, "device"); > BlockDriverState *bs; > int ret; >=20 > if (!strcmp(device, "all")) { > ret =3D bdrv_commit_all(); > } else { > bs =3D bdrv_find(device); > if (!bs) { > monitor_printf(mon, "Device '%s' not found\n", device); > return; > } > ret =3D bdrv_commit(bs); > } > if (ret < 0) { > monitor_printf(mon, "'commit' error for '%s': %s\n", device, > strerror(-ret)); > } > } >=20 > internal snapshot deletion > -------------------------- > SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *de= vice, > bool has_id, > const char *id= , > bool has_name, > const char *na= me, > Error **errp) > { > BlockDriverState *bs =3D bdrv_find(device); > QEMUSnapshotInfo sn; > Error *local_err =3D NULL; > SnapshotInfo *info =3D NULL; >=20 >=20 > Internal snapshot preparation > ----------------------------- > static void internal_snapshot_prepare(BlkTransactionState *common, > Error **errp) > { > const char *device; > const char *name; >=20 > BlockDriverState *bs; > QEMUSnapshotInfo old_sn, *sn; > bool ret; > qemu_timeval tv; > BlockdevSnapshotInternal *internal; > InternalSnapshotState *state; > int ret1; >=20 > g_assert(common->action->kind =3D=3D > TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC); > internal =3D common->action->blockdev_snapshot_internal_sync; > state =3D DO_UPCAST(InternalSnapshotState, common, common); >=20 > /* 1. parse input */ > device =3D internal->device; > name =3D internal->name; >=20 > /* 2. check for validation */ > bs =3D bdrv_find(device); > if (!bs) { > error_set(errp, QERR_DEVICE_NOT_FOUND, device); > return; > } >=20 > Drive backup > ------------ > static void drive_backup_prepare(BlkTransactionState *common, Error **e= rrp) > { > DriveBackupState *state =3D DO_UPCAST(DriveBackupState, common, com= mon); > DriveBackup *backup; > Error *local_err =3D NULL; >=20 > assert(common->action->kind =3D=3D TRANSACTION_ACTION_KIND_DRIVE_BA= CKUP); > backup =3D common->action->drive_backup; >=20 > 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_err= or, > backup->has_on_target_error, backup->on_target_err= or, > &local_err); > if (error_is_set(&local_err)) { > error_propagate(errp, local_err); > state->bs =3D NULL; > state->job =3D NULL; > return; > } >=20 > state->bs =3D bdrv_find(backup->device); > state->job =3D state->bs->job; > } >=20 > Eject which should operate on backends > -------------------------------------- > void qmp_eject(const char *device, bool has_force, bool force, Error **= errp) > { > BlockDriverState *bs; >=20 > bs =3D bdrv_find(device); > if (!bs) { > error_set(errp, QERR_DEVICE_NOT_FOUND, device); > return; > } >=20 > eject_device(bs, force, errp); > } >=20 > QCow2 crypto > ------------ > void qmp_block_passwd(const char *device, const char *password, Error *= *errp) > { > BlockDriverState *bs; > int err; >=20 > bs =3D bdrv_find(device); > if (!bs) { > error_set(errp, QERR_DEVICE_NOT_FOUND, device); > return; > } >=20 > err =3D bdrv_set_key(bs, password); > if (err =3D=3D -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; > } > } >=20 > 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 **e= rrp) > { > BlockDriverState *bs; > BlockDriver *drv =3D NULL; > int bdrv_flags; > Error *err =3D NULL; >=20 > bs =3D bdrv_find(device); > if (!bs) { > error_set(errp, QERR_DEVICE_NOT_FOUND, device); > return; > } >=20 > if (format) { > drv =3D bdrv_find_whitelisted_format(format, bs->read_only); > if (!drv) { > error_set(errp, QERR_INVALID_BLOCK_FORMAT, format); > return; > } > } >=20 > eject_device(bs, 0, &err); > if (error_is_set(&err)) { > error_propagate(errp, err); > return; > } >=20 > bdrv_flags =3D bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; > bdrv_flags |=3D bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0; >=20 > qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp); > } >=20 > 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; >=20 > bs =3D bdrv_find(device); > if (!bs) { > error_set(errp, QERR_DEVICE_NOT_FOUND, device); > return; > } >=20 > Drive deletion > -------------- > int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) > { > const char *id =3D qdict_get_str(qdict, "id"); > BlockDriverState *bs; >=20 > bs =3D bdrv_find(id); > if (!bs) { > qerror_report(QERR_DEVICE_NOT_FOUND, id); > return -1; > } >=20 > block resize > ------------ > void qmp_block_resize(const char *device, int64_t size, Error **errp) > { > BlockDriverState *bs; > int ret; >=20 > bs =3D bdrv_find(device); > if (!bs) { > error_set(errp, QERR_DEVICE_NOT_FOUND, device); > return; > } >=20 > 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 =3D NULL; > Error *local_err =3D NULL; >=20 > if (!has_on_error) { > on_error =3D BLOCKDEV_ON_ERROR_REPORT; > } >=20 > bs =3D bdrv_find(device); > if (!bs) { > error_set(errp, QERR_DEVICE_NOT_FOUND, device); > return; > } >=20 > 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 =3D NULL; > /* This will be part of the QMP command, if/when the > * BlockdevOnError change for blkmirror makes it in > */ > BlockdevOnError on_error =3D BLOCKDEV_ON_ERROR_REPORT; >=20 > /* drain all i/o before commits */ > bdrv_drain_all(); >=20 > bs =3D bdrv_find(device); > if (!bs) { > error_set(errp, QERR_DEVICE_NOT_FOUND, device); > return; > } >=20 >=20 > 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_sour= ce_error, > bool has_on_target_error, BlockdevOnError on_targ= et_error, > Error **errp) > { > BlockDriverState *bs; > BlockDriverState *target_bs; > BlockDriverState *source =3D NULL; > BlockDriver *drv =3D NULL; > Error *local_err =3D NULL; > int flags; > int64_t size; > int ret; >=20 > if (!has_speed) { > speed =3D 0; > } > if (!has_on_source_error) { > on_source_error =3D BLOCKDEV_ON_ERROR_REPORT; > } > if (!has_on_target_error) { > on_target_error =3D BLOCKDEV_ON_ERROR_REPORT; > } > if (!has_mode) { > mode =3D NEW_IMAGE_MODE_ABSOLUTE_PATHS; > } >=20 > bs =3D bdrv_find(device); > if (!bs) { > error_set(errp, QERR_DEVICE_NOT_FOUND, device); > return; > } >=20 >=20 > 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_sour= ce_error, > bool has_on_target_error, BlockdevOnError on_targ= et_error, > Error **errp) > { > BlockDriverState *bs; > BlockDriverState *source, *target_bs; > BlockDriver *drv =3D NULL; > Error *local_err =3D NULL; > int flags; > int64_t size; > int ret; >=20 > if (!has_speed) { > speed =3D 0; > } > if (!has_on_source_error) { > on_source_error =3D BLOCKDEV_ON_ERROR_REPORT; > } > if (!has_on_target_error) { > on_target_error =3D BLOCKDEV_ON_ERROR_REPORT; > } > if (!has_mode) { > mode =3D NEW_IMAGE_MODE_ABSOLUTE_PATHS; > } > if (!has_granularity) { > granularity =3D 0; > } > if (!has_buf_size) { > buf_size =3D DEFAULT_MIRROR_BUF_SIZE; > } >=20 > if (granularity !=3D 0 && (granularity < 512 || granularity > 10485= 76 * 64)) { > error_set(errp, QERR_INVALID_PARAMETER, device); > return; > } > if (granularity & (granularity - 1)) { > error_set(errp, QERR_INVALID_PARAMETER, device); > return; > } >=20 > bs =3D bdrv_find(device); > if (!bs) { > error_set(errp, QERR_DEVICE_NOT_FOUND, device); > return; > } >=20 > find block job > -------------- > static BlockJob *find_block_job(const char *device) > { > BlockDriverState *bs; >=20 > bs =3D bdrv_find(device); > if (!bs || !bs->job) { > return NULL; > } > return bs->job; > } >=20 And nbd-server-add. > Very few of them operate on what is destined to become block backend an= d most of > them should be able to operate on nodes of the graph; >=20 > 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 g= raph. So I prefer we don't touch the qmp interface. We can always introduce opt= ional parameter and make mandatory ones optional, if necessary. But for now, ju= st equalize node-name and device-name should be neater from a users view. I= f a command is only for backend, we can check internally and report error if = the provided node is not. Thanks, Fam