| Message ID | 20200510134037.18487-12-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LUKS: encryption slot management using amend interface | expand |
On 10.05.20 15:40, Maxim Levitsky wrote: > blockdev-amend will be used similiar to blockdev-create > to allow on the fly changes of the structure of the format based block devices. > > Current plan is to first support encryption keyslot management for luks > based formats (raw and embedded in qcow2) > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > --- > block/Makefile.objs | 2 +- > block/amend.c | 108 ++++++++++++++++++++++++++++++++++++++ > include/block/block_int.h | 21 +++++--- > qapi/block-core.json | 42 +++++++++++++++ > qapi/job.json | 4 +- > 5 files changed, 169 insertions(+), 8 deletions(-) > create mode 100644 block/amend.c [...] > diff --git a/block/amend.c b/block/amend.c > new file mode 100644 > index 0000000000..4840c0ffef > --- /dev/null > +++ b/block/amend.c [...] > +void qmp_x_blockdev_amend(const char *job_id, > + const char *node_name, > + BlockdevAmendOptions *options, > + bool has_force, > + bool force, > + Error **errp) > +{ > + BlockdevAmendJob *s; > + const char *fmt = BlockdevDriver_str(options->driver); > + BlockDriver *drv = bdrv_find_format(fmt); > + BlockDriverState *bs = bdrv_find_node(node_name); > + > + /* > + * If the driver is in the schema, we know that it exists. I wonder why create.c then still checks whether drv == NULL. I wouldn’t count on the schema. For example, with modularized block driver I could imagine that a driver appears in the schema, but loading the module fails, so that drv still ends up NULL. > But it may not > + * be whitelisted. > + */ > + assert(drv); > + if (bdrv_uses_whitelist() && !bdrv_is_whitelisted(drv, false)) { > + error_setg(errp, "Driver is not whitelisted"); > + return; > + } [...] > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 0a71357b50..fdb0cdbcdd 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -133,12 +133,27 @@ struct BlockDriver { > int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags, > Error **errp); > void (*bdrv_close)(BlockDriverState *bs); > + > + > int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts, > Error **errp); > int coroutine_fn (*bdrv_co_create_opts)(BlockDriver *drv, > const char *filename, > QemuOpts *opts, > Error **errp); > + > + int coroutine_fn (*bdrv_co_amend)(BlockDriverState *bs, > + BlockdevAmendOptions *opts, > + bool force, > + Error **errp); > + > + int (*bdrv_amend_options)(BlockDriverState *bs, > + QemuOpts *opts, > + BlockDriverAmendStatusCB *status_cb, > + void *cb_opaque, > + bool force, > + Error **errp); > + > int (*bdrv_make_empty)(BlockDriverState *bs); > > /* > @@ -433,12 +448,6 @@ struct BlockDriver { > BdrvCheckResult *result, > BdrvCheckMode fix); > > - int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts, > - BlockDriverAmendStatusCB *status_cb, > - void *cb_opaque, > - bool force, > - Error **errp); > - No harm done, but why not just leave it where it was? > void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent event); > > /* TODO Better pass a option string/QDict/QemuOpts to add any rule? */ > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 943df1926a..74db515414 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -4649,6 +4649,48 @@ > 'data': { 'job-id': 'str', > 'options': 'BlockdevCreateOptions' } } > > +## > +# @BlockdevAmendOptions: > +# > +# Options for amending an image format > +# > +# @driver block driver that is suitable for the image Missing colon after @driver. Also, what does “suitable” mean? Shouldn’t it be exactly the node’s driver? (i.e. “Block driver of the node to amend”) Max
On Thu, 2020-05-14 at 17:47 +0200, Max Reitz wrote: > On 10.05.20 15:40, Maxim Levitsky wrote: > > blockdev-amend will be used similiar to blockdev-create > > to allow on the fly changes of the structure of the format based block devices. > > > > Current plan is to first support encryption keyslot management for luks > > based formats (raw and embedded in qcow2) > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > block/Makefile.objs | 2 +- > > block/amend.c | 108 ++++++++++++++++++++++++++++++++++++++ > > include/block/block_int.h | 21 +++++--- > > qapi/block-core.json | 42 +++++++++++++++ > > qapi/job.json | 4 +- > > 5 files changed, 169 insertions(+), 8 deletions(-) > > create mode 100644 block/amend.c > > [...] > > > diff --git a/block/amend.c b/block/amend.c > > new file mode 100644 > > index 0000000000..4840c0ffef > > --- /dev/null > > +++ b/block/amend.c > > [...] > > > +void qmp_x_blockdev_amend(const char *job_id, > > + const char *node_name, > > + BlockdevAmendOptions *options, > > + bool has_force, > > + bool force, > > + Error **errp) > > +{ > > + BlockdevAmendJob *s; > > + const char *fmt = BlockdevDriver_str(options->driver); > > + BlockDriver *drv = bdrv_find_format(fmt); > > + BlockDriverState *bs = bdrv_find_node(node_name); > > + > > + /* > > + * If the driver is in the schema, we know that it exists. > > I wonder why create.c then still checks whether drv == NULL. It because I copy&pasta'ed that code when this fix wasn't there. I'll copy it to amend.c, since this check doesn't hurt anyway. I do wonder if we should make a common function for that can check that!= NULL and whitelist? Thoughts? > > I wouldn’t count on the schema. For example, with modularized block > driver I could imagine that a driver appears in the schema, but loading > the module fails, so that drv still ends up NULL. > > > But it may not > > + * be whitelisted. > > + */ > > + assert(drv); > > + if (bdrv_uses_whitelist() && !bdrv_is_whitelisted(drv, false)) { > > + error_setg(errp, "Driver is not whitelisted"); > > + return; > > + } > > [...] > > > diff --git a/include/block/block_int.h b/include/block/block_int.h > > index 0a71357b50..fdb0cdbcdd 100644 > > --- a/include/block/block_int.h > > +++ b/include/block/block_int.h > > @@ -133,12 +133,27 @@ struct BlockDriver { > > int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags, > > Error **errp); > > void (*bdrv_close)(BlockDriverState *bs); > > + > > + > > int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts, > > Error **errp); > > int coroutine_fn (*bdrv_co_create_opts)(BlockDriver *drv, > > const char *filename, > > QemuOpts *opts, > > Error **errp); > > + > > + int coroutine_fn (*bdrv_co_amend)(BlockDriverState *bs, > > + BlockdevAmendOptions *opts, > > + bool force, > > + Error **errp); > > + > > + int (*bdrv_amend_options)(BlockDriverState *bs, > > + QemuOpts *opts, > > + BlockDriverAmendStatusCB *status_cb, > > + void *cb_opaque, > > + bool force, > > + Error **errp); > > + > > int (*bdrv_make_empty)(BlockDriverState *bs); > > > > /* > > @@ -433,12 +448,6 @@ struct BlockDriver { > > BdrvCheckResult *result, > > BdrvCheckMode fix); > > > > - int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts, > > - BlockDriverAmendStatusCB *status_cb, > > - void *cb_opaque, > > - bool force, > > - Error **errp); > > - > > No harm done, but why not just leave it where it was? I wanted to have both legacy and qmp amend interfaces appear close in the header file. > > > void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent event); > > > > /* TODO Better pass a option string/QDict/QemuOpts to add any rule? */ > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > index 943df1926a..74db515414 100644 > > --- a/qapi/block-core.json > > +++ b/qapi/block-core.json > > @@ -4649,6 +4649,48 @@ > > 'data': { 'job-id': 'str', > > 'options': 'BlockdevCreateOptions' } } > > > > +## > > +# @BlockdevAmendOptions: > > +# > > +# Options for amending an image format > > +# > > +# @driver block driver that is suitable for the image > > Missing colon after @driver. Also, what does “suitable” mean? > Shouldn’t it be exactly the node’s driver? (i.e. “Block driver of the > node to amend”) Fixed! > > Max > Best regards, Maxim Levitsky
diff --git a/block/Makefile.objs b/block/Makefile.objs index 3635b6b4c1..a0988638d5 100644 --- a/block/Makefile.objs +++ b/block/Makefile.objs @@ -19,7 +19,7 @@ block-obj-$(CONFIG_WIN32) += file-win32.o win32-aio.o block-obj-$(CONFIG_POSIX) += file-posix.o block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o block-obj-$(CONFIG_LINUX_IO_URING) += io_uring.o -block-obj-y += null.o mirror.o commit.o io.o create.o +block-obj-y += null.o mirror.o commit.o io.o create.o amend.o block-obj-y += throttle-groups.o block-obj-$(CONFIG_LINUX) += nvme.o diff --git a/block/amend.c b/block/amend.c new file mode 100644 index 0000000000..4840c0ffef --- /dev/null +++ b/block/amend.c @@ -0,0 +1,108 @@ +/* + * Block layer code related to image options amend + * + * Copyright (c) 2018 Kevin Wolf <kwolf@redhat.com> + * Copyright (c) 2020 Red Hat. Inc + * + * Heavily based on create.c + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#include "qemu/osdep.h" +#include "block/block_int.h" +#include "qemu/job.h" +#include "qemu/main-loop.h" +#include "qapi/qapi-commands-block-core.h" +#include "qapi/qapi-visit-block-core.h" +#include "qapi/clone-visitor.h" +#include "qapi/error.h" + +typedef struct BlockdevAmendJob { + Job common; + BlockdevAmendOptions *opts; + BlockDriverState *bs; + bool force; +} BlockdevAmendJob; + +static int coroutine_fn blockdev_amend_run(Job *job, Error **errp) +{ + BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common); + int ret; + + job_progress_set_remaining(&s->common, 1); + ret = s->bs->drv->bdrv_co_amend(s->bs, s->opts, s->force, errp); + job_progress_update(&s->common, 1); + qapi_free_BlockdevAmendOptions(s->opts); + return ret; +} + +static const JobDriver blockdev_amend_job_driver = { + .instance_size = sizeof(BlockdevAmendJob), + .job_type = JOB_TYPE_AMEND, + .run = blockdev_amend_run, +}; + +void qmp_x_blockdev_amend(const char *job_id, + const char *node_name, + BlockdevAmendOptions *options, + bool has_force, + bool force, + Error **errp) +{ + BlockdevAmendJob *s; + const char *fmt = BlockdevDriver_str(options->driver); + BlockDriver *drv = bdrv_find_format(fmt); + BlockDriverState *bs = bdrv_find_node(node_name); + + /* + * If the driver is in the schema, we know that it exists. But it may not + * be whitelisted. + */ + assert(drv); + if (bdrv_uses_whitelist() && !bdrv_is_whitelisted(drv, false)) { + error_setg(errp, "Driver is not whitelisted"); + return; + } + + if (bs->drv != drv) { + error_setg(errp, + "x-blockdev-amend doesn't support changing the block driver"); + return; + } + + /* Error out if the driver doesn't support .bdrv_co_amend */ + if (!drv->bdrv_co_amend) { + error_setg(errp, "Driver does not support x-blockdev-amend"); + return; + } + + /* Create the block job */ + s = job_create(job_id, &blockdev_amend_job_driver, NULL, + bdrv_get_aio_context(bs), JOB_DEFAULT | JOB_MANUAL_DISMISS, + NULL, NULL, errp); + if (!s) { + return; + } + + s->bs = bs, + s->opts = QAPI_CLONE(BlockdevAmendOptions, options), + s->force = has_force ? force : false; + job_start(&s->common); +} diff --git a/include/block/block_int.h b/include/block/block_int.h index 0a71357b50..fdb0cdbcdd 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -133,12 +133,27 @@ struct BlockDriver { int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags, Error **errp); void (*bdrv_close)(BlockDriverState *bs); + + int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts, Error **errp); int coroutine_fn (*bdrv_co_create_opts)(BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp); + + int coroutine_fn (*bdrv_co_amend)(BlockDriverState *bs, + BlockdevAmendOptions *opts, + bool force, + Error **errp); + + int (*bdrv_amend_options)(BlockDriverState *bs, + QemuOpts *opts, + BlockDriverAmendStatusCB *status_cb, + void *cb_opaque, + bool force, + Error **errp); + int (*bdrv_make_empty)(BlockDriverState *bs); /* @@ -433,12 +448,6 @@ struct BlockDriver { BdrvCheckResult *result, BdrvCheckMode fix); - int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts, - BlockDriverAmendStatusCB *status_cb, - void *cb_opaque, - bool force, - Error **errp); - void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent event); /* TODO Better pass a option string/QDict/QemuOpts to add any rule? */ diff --git a/qapi/block-core.json b/qapi/block-core.json index 943df1926a..74db515414 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -4649,6 +4649,48 @@ 'data': { 'job-id': 'str', 'options': 'BlockdevCreateOptions' } } +## +# @BlockdevAmendOptions: +# +# Options for amending an image format +# +# @driver block driver that is suitable for the image +# +# Since: 5.1 +## +{ 'union': 'BlockdevAmendOptions', + 'base': { + 'driver': 'BlockdevDriver' }, + 'discriminator': 'driver', + 'data': { + } } + +## +# @x-blockdev-amend: +# +# Starts a job to amend format specific options of an existing open block device +# The job is automatically finalized, but a manual job-dismiss is required. +# +# @job-id: Identifier for the newly created job. +# +# @node-name: Name of the block node to work on +# +# @options: Options (driver specific) +# +# @force: Allow unsafe operations, format specific +# For luks that allows erase of the last active keyslot +# (permanent loss of data), +# and replacement of an active keyslot +# (possible loss of data if IO error happens) +# +# Since: 5.1 +## +{ 'command': 'x-blockdev-amend', + 'data': { 'job-id': 'str', + 'node-name': 'str', + 'options': 'BlockdevAmendOptions', + '*force': 'bool' } } + ## # @BlockErrorAction: # diff --git a/qapi/job.json b/qapi/job.json index 5e658281f5..c48a0c3e34 100644 --- a/qapi/job.json +++ b/qapi/job.json @@ -19,10 +19,12 @@ # # @create: image creation job type, see "blockdev-create" (since 3.0) # +# @amend: image options amend job type, see "x-blockdev-amend" (since 5.1) +# # Since: 1.7 ## { 'enum': 'JobType', - 'data': ['commit', 'stream', 'mirror', 'backup', 'create'] } + 'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] } ## # @JobStatus: