| Message ID | 20190912223028.18496-8-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFC crypto/luks: encryption key managment using amend interface | expand |
On 13.09.19 00:30, Maxim Levitsky wrote: > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > block/Makefile.objs | 2 +- > block/amend.c | 116 ++++++++++++++++++++++++++++++++++++++ > include/block/block_int.h | 23 ++++++-- > qapi/block-core.json | 26 +++++++++ > qapi/job.json | 4 +- > 5 files changed, 163 insertions(+), 8 deletions(-) > create mode 100644 block/amend.c I think I’d move this (and everything to belongs to it) to a different series. > diff --git a/block/Makefile.objs b/block/Makefile.objs > index 35f3bca4d9..10d0308792 100644 > --- a/block/Makefile.objs > +++ b/block/Makefile.objs > @@ -18,7 +18,7 @@ block-obj-y += block-backend.o snapshot.o qapi.o > 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-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..9bd28e08e7 > --- /dev/null > +++ b/block/amend.c > @@ -0,0 +1,116 @@ > +/* > + * Block layer code related to image options amend > + * > + * Copyright (c) 2018 Kevin Wolf <kwolf@redhat.com> > + * Copyright (c) 2019 Maxim Levitsky <mlevitsk@redhat.com> > + * > + * 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; > + BlockdevCreateOptions *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); It would be nice if the amend job could make use of the progress reporting that we have in place for amend. > + > + qapi_free_BlockdevCreateOptions(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, > + BlockdevCreateOptions *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 > + * TODO Running in the main context. Block drivers need to error out or add > + * locking when they use a BDS in a different AioContext. Why shouldn’t the job just run in the node’s context? Max
Maxim Levitsky <mlevitsk@redhat.com> writes: > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- [...] > diff --git a/qapi/block-core.json b/qapi/block-core.json > index e6edd641f1..7900914506 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -4650,6 +4650,32 @@ > 'data': { 'job-id': 'str', > 'options': 'BlockdevCreateOptions' } } > > +## > +# @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 (same as for image creation) > +# > +# @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: 4.2 > +## > +{ 'command': 'x-blockdev-amend', > + 'data': { 'job-id': 'str', > + 'node-name': 'str', > + 'options': 'BlockdevCreateOptions', > + '*force': 'bool' } } > + > ## > # @blockdev-open-tray: > # > diff --git a/qapi/job.json b/qapi/job.json > index a121b615fb..342d29a7aa 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 4.2) > +# > # Since: 1.7 > ## > { 'enum': 'JobType', > - 'data': ['commit', 'stream', 'mirror', 'backup', 'create'] } > + 'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] } > > ## > # @JobStatus: Aha, you introduce "amend" after using the concept in PATCH 02. Rather confusing, I'm afraid. I guess secret deletion makes sense with amend somehow, and only with amend. If yes, the QAPI documentaion should spell that out, and the code should diagnose invalid use (maybe it does already; I haven't looked at it).
On Fri, 2019-10-04 at 20:53 +0200, Max Reitz wrote: > On 13.09.19 00:30, Maxim Levitsky wrote: > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > block/Makefile.objs | 2 +- > > block/amend.c | 116 ++++++++++++++++++++++++++++++++++++++ > > include/block/block_int.h | 23 ++++++-- > > qapi/block-core.json | 26 +++++++++ > > qapi/job.json | 4 +- > > 5 files changed, 163 insertions(+), 8 deletions(-) > > create mode 100644 block/amend.c > > I think I’d move this (and everything to belongs to it) to a different > series. I already kind of do this, patches prior to this one fully implment the existing amend code path, while this and patches after this one implement the qmp x-blockdev-amend code path. i don't mind sending this as two separate patch series, now that first refactoring patch series is committed upsteam. > > > diff --git a/block/Makefile.objs b/block/Makefile.objs > > index 35f3bca4d9..10d0308792 100644 > > --- a/block/Makefile.objs > > +++ b/block/Makefile.objs > > @@ -18,7 +18,7 @@ block-obj-y += block-backend.o snapshot.o qapi.o > > 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-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..9bd28e08e7 > > --- /dev/null > > +++ b/block/amend.c > > @@ -0,0 +1,116 @@ > > +/* > > + * Block layer code related to image options amend > > + * > > + * Copyright (c) 2018 Kevin Wolf <kwolf@redhat.com> > > + * Copyright (c) 2019 Maxim Levitsky <mlevitsk@redhat.com> > > + * > > + * 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; > > + BlockdevCreateOptions *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); > > It would be nice if the amend job could make use of the progress > reporting that we have in place for amend. I also thought about it, but is it worth it? I looked through the status reporting of the qcow2 amend code (which doesn't really allowed to be run through qmp blockdev-amend, due to complexity of changing the qcow2 format on the fly). > > > + > > + qapi_free_BlockdevCreateOptions(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, > > + BlockdevCreateOptions *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 > > + * TODO Running in the main context. Block drivers need to error out or add > > + * locking when they use a BDS in a different AioContext. > > Why shouldn’t the job just run in the node’s context? This is shameless copy&pasta from the blockdev-create code (which I did note in the copyright of the file) Best regards, Maxim Levitsky > > Max >
On Fri, 2019-10-04 at 20:53 +0200, Max Reitz wrote: > On 13.09.19 00:30, Maxim Levitsky wrote: > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > block/Makefile.objs | 2 +- > > block/amend.c | 116 ++++++++++++++++++++++++++++++++++++++ > > include/block/block_int.h | 23 ++++++-- > > qapi/block-core.json | 26 +++++++++ > > qapi/job.json | 4 +- > > 5 files changed, 163 insertions(+), 8 deletions(-) > > create mode 100644 block/amend.c > > I think I’d move this (and everything to belongs to it) to a different > series. I already kind of do this, patches prior to this one fully implement the existing amend code path, while this and patches after this one implement the qmp x-blockdev-amend code path. i don't mind sending this as two separate patch series, now that first refactoring patch series is committed upsteam. > > > diff --git a/block/Makefile.objs b/block/Makefile.objs > > index 35f3bca4d9..10d0308792 100644 > > --- a/block/Makefile.objs > > +++ b/block/Makefile.objs > > @@ -18,7 +18,7 @@ block-obj-y += block-backend.o snapshot.o qapi.o > > 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-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..9bd28e08e7 > > --- /dev/null > > +++ b/block/amend.c > > @@ -0,0 +1,116 @@ > > +/* > > + * Block layer code related to image options amend > > + * > > + * Copyright (c) 2018 Kevin Wolf <kwolf@redhat.com> > > + * Copyright (c) 2019 Maxim Levitsky <mlevitsk@redhat.com> > > + * > > + * 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; > > + BlockdevCreateOptions *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); > > It would be nice if the amend job could make use of the progress > reporting that we have in place for amend. I also thought about it, but is it worth it? I looked through the status reporting of the qcow2 amend code (which doesn't really allowed to be run through qmp blockdev-amend, due to complexity of changing the qcow2 format on the fly). > > > + > > + qapi_free_BlockdevCreateOptions(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, > > + BlockdevCreateOptions *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 > > + * TODO Running in the main context. Block drivers need to error out or add > > + * locking when they use a BDS in a different AioContext. > > Why shouldn’t the job just run in the node’s context? This is shameless copy&pasta from the blockdev-create code (which I did note in the copyright of the file) Best regards, Maxim Levitsky > > Max >
On 08.11.19 10:26, Maxim Levitsky wrote: > On Fri, 2019-10-04 at 20:53 +0200, Max Reitz wrote: >> On 13.09.19 00:30, Maxim Levitsky wrote: >>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> >>> --- >>> block/Makefile.objs | 2 +- >>> block/amend.c | 116 ++++++++++++++++++++++++++++++++++++++ >>> include/block/block_int.h | 23 ++++++-- >>> qapi/block-core.json | 26 +++++++++ >>> qapi/job.json | 4 +- >>> 5 files changed, 163 insertions(+), 8 deletions(-) >>> create mode 100644 block/amend.c [...] >>> +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); >> >> It would be nice if the amend job could make use of the progress >> reporting that we have in place for amend. > > I also thought about it, but is it worth it? > > I looked through the status reporting of the qcow2 amend > code (which doesn't really allowed to be run through > qmp blockdev-amend, due to complexity of changing > the qcow2 format on the fly). True, and we could always add it later. I suppose I was mostly wondering because bdrv_amend_options already has all of that infrastructure and I was assuming that qcow2's bdrv_co_amend implementation would make some use of the existing function. Well, it doesn’t, so *shrug* [...] >>> + /* >>> + * Create the block job >>> + * TODO Running in the main context. Block drivers need to error out or add >>> + * locking when they use a BDS in a different AioContext. >> >> Why shouldn’t the job just run in the node’s context? > > This is shameless copy&pasta from the blockdev-create code > (which I did note in the copyright of the file) Well, you noted that it’s heavily based on it, not that it’s just C&P. So I suppose the comment is just wrong here? Max
On Fri, 2019-11-08 at 11:36 +0100, Max Reitz wrote: > On 08.11.19 10:26, Maxim Levitsky wrote: > > On Fri, 2019-10-04 at 20:53 +0200, Max Reitz wrote: > > > On 13.09.19 00:30, Maxim Levitsky wrote: > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > > > --- > > > > block/Makefile.objs | 2 +- > > > > block/amend.c | 116 ++++++++++++++++++++++++++++++++++++++ > > > > include/block/block_int.h | 23 ++++++-- > > > > qapi/block-core.json | 26 +++++++++ > > > > qapi/job.json | 4 +- > > > > 5 files changed, 163 insertions(+), 8 deletions(-) > > > > create mode 100644 block/amend.c > > [...] > > > > > +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); > > > > > > It would be nice if the amend job could make use of the progress > > > reporting that we have in place for amend. > > > > I also thought about it, but is it worth it? > > > > I looked through the status reporting of the qcow2 amend > > code (which doesn't really allowed to be run through > > qmp blockdev-amend, due to complexity of changing > > the qcow2 format on the fly). > > True, and we could always add it later. > > I suppose I was mostly wondering because bdrv_amend_options already has > all of that infrastructure and I was assuming that qcow2's bdrv_co_amend > implementation would make some use of the existing function. Well, it > doesn’t, so *shrug* > > [...] > > > > > + /* > > > > + * Create the block job > > > > + * TODO Running in the main context. Block drivers need to error out or add > > > > + * locking when they use a BDS in a different AioContext. > > > > > > Why shouldn’t the job just run in the node’s context? > > > > This is shameless copy&pasta from the blockdev-create code > > (which I did note in the copyright of the file) > > Well, you noted that it’s heavily based on it, not that it’s just C&P. 'heavily based' is a polite way to say that file is copied and then changed to fit new purpose, isn't it :-) > > So I suppose the comment is just wrong here? Yes, I absolutely missed this part since I don't know the block layer well enough. Thanks for explaining this on IRC, that blockdev_create job is special in a sense that block device state doesn't exit yet for it (I also was already bitten by this in luks block driver), thus the job runs in main AIO context. No need to do so in amend, and thus I'll remove that wrongly copy&pasta'ed comment. Thanks a lot!! Best regards, Maxim Levitsky > > Max >
On Mon, 2019-10-07 at 09:53 +0200, Markus Armbruster wrote: > Maxim Levitsky <mlevitsk@redhat.com> writes: > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > [...] > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > index e6edd641f1..7900914506 100644 > > --- a/qapi/block-core.json > > +++ b/qapi/block-core.json > > @@ -4650,6 +4650,32 @@ > > 'data': { 'job-id': 'str', > > 'options': 'BlockdevCreateOptions' } } > > > > +## > > +# @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 (same as for image creation) > > +# > > +# @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: 4.2 > > +## > > +{ 'command': 'x-blockdev-amend', > > + 'data': { 'job-id': 'str', > > + 'node-name': 'str', > > + 'options': 'BlockdevCreateOptions', > > + '*force': 'bool' } } > > + > > ## > > # @blockdev-open-tray: > > # > > diff --git a/qapi/job.json b/qapi/job.json > > index a121b615fb..342d29a7aa 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 4.2) > > +# > > # Since: 1.7 > > ## > > { 'enum': 'JobType', > > - 'data': ['commit', 'stream', 'mirror', 'backup', 'create'] } > > + 'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] } > > > > ## > > # @JobStatus: > > Aha, you introduce "amend" after using the concept in PATCH 02. > Rather confusing, I'm afraid. > > I guess secret deletion makes sense with amend somehow, and only with > amend. If yes, the QAPI documentaion should spell that out, and the > code should diagnose invalid use (maybe it does already; I haven't > looked at it). I think I did mentioned that in QAPI, and I do check for optional arguments in the code. I will probably stop using the blockdev-create options for amend after all. Best regards, Maxim Levitsky
diff --git a/block/Makefile.objs b/block/Makefile.objs index 35f3bca4d9..10d0308792 100644 --- a/block/Makefile.objs +++ b/block/Makefile.objs @@ -18,7 +18,7 @@ block-obj-y += block-backend.o snapshot.o qapi.o 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-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..9bd28e08e7 --- /dev/null +++ b/block/amend.c @@ -0,0 +1,116 @@ +/* + * Block layer code related to image options amend + * + * Copyright (c) 2018 Kevin Wolf <kwolf@redhat.com> + * Copyright (c) 2019 Maxim Levitsky <mlevitsk@redhat.com> + * + * 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; + BlockdevCreateOptions *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_BlockdevCreateOptions(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, + BlockdevCreateOptions *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 + * TODO Running in the main context. Block drivers need to error out or add + * locking when they use a BDS in a different AioContext. + */ + s = job_create(job_id, &blockdev_amend_job_driver, NULL, + qemu_get_aio_context(), JOB_DEFAULT | JOB_MANUAL_DISMISS, + NULL, NULL, errp); + if (!s) { + return; + } + + s->bs = bs, + s->opts = QAPI_CLONE(BlockdevCreateOptions, 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 5ea30f9d58..3001114361 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -133,11 +133,28 @@ 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)(const char *filename, QemuOpts *opts, Error **errp); + + + int coroutine_fn (*bdrv_co_amend)(BlockDriverState *bs, + BlockdevCreateOptions *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); /* @@ -408,12 +425,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 e6edd641f1..7900914506 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -4650,6 +4650,32 @@ 'data': { 'job-id': 'str', 'options': 'BlockdevCreateOptions' } } +## +# @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 (same as for image creation) +# +# @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: 4.2 +## +{ 'command': 'x-blockdev-amend', + 'data': { 'job-id': 'str', + 'node-name': 'str', + 'options': 'BlockdevCreateOptions', + '*force': 'bool' } } + ## # @blockdev-open-tray: # diff --git a/qapi/job.json b/qapi/job.json index a121b615fb..342d29a7aa 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 4.2) +# # Since: 1.7 ## { 'enum': 'JobType', - 'data': ['commit', 'stream', 'mirror', 'backup', 'create'] } + 'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] } ## # @JobStatus:
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- block/Makefile.objs | 2 +- block/amend.c | 116 ++++++++++++++++++++++++++++++++++++++ include/block/block_int.h | 23 ++++++-- qapi/block-core.json | 26 +++++++++ qapi/job.json | 4 +- 5 files changed, 163 insertions(+), 8 deletions(-) create mode 100644 block/amend.c