| Message ID | 20201204220758.2879-10-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Apply COR-filter to the block-stream permanently | expand |
On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote: > From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > > Avoid writing a filter JSON file name and a filter format name to QCOW2 > image when the backing file is being changed after the block stream > job. It can occur due to a concurrent commit job on the same backing > chain. > A user is still able to assign the 'backing-file' parameter for a > block-stream job keeping in mind the possible issue mentioned above. > If the user does not specify the 'backing-file' parameter, QEMU will > assign it automatically. > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > [vsementsov: use unfiltered_bs for bdrv_find_backing_image()] > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/stream.c | 21 +++++++++++++++++++-- > blockdev.c | 8 +------- > 2 files changed, 20 insertions(+), 9 deletions(-) > > diff --git a/block/stream.c b/block/stream.c > index 6e281c71ac..c208393c34 100644 > --- a/block/stream.c > +++ b/block/stream.c [...] > @@ -75,8 +78,22 @@ static int stream_prepare(Job *job) > const char *base_id = NULL, *base_fmt = NULL; > if (base) { > base_id = s->backing_file_str; > - if (base->drv) { > - base_fmt = base->drv->format_name; > + if (base_id) { > + backing_bs = bdrv_find_backing_image(unfiltered_bs, base_id); > + if (backing_bs && backing_bs->drv) { > + base_fmt = backing_bs->drv->format_name; > + } else { > + error_report("Format not found for backing file %s", > + s->backing_file_str); I think it’s actually going to be rather likely that we’re not going to find the backing file here. If the user were to use a filename that just appears as-such in the backing chain, they wouldn’t need to specify a backing-file parameter at all, because the one figured out automatically would be just fine. But then again, what are we supposed to do then. We can continue as before, which is to just use the base node’s format. But if the user wants to perhaps use a backing file that isn’t even open in qemu (a copy of the the base on some different storage), we have no idea what format it’s in. So printing an error here, but continuing on with setting a backing_fmt is probably the most reasonable thing to do indeed. > + } > + } else { > + base_unfiltered = bdrv_skip_filters(base); > + if (base_unfiltered) { @base_unfiltered cannot be NULL here (because @base is sure not to be NULL). Of course, double-checking isn’t wrong, it just looks a bit weird, because it seems to imply that we might end up with a case where base != NULL, but base_id == NULL. Anyway: Reviewed-by: Max Reitz <mreitz@redhat.com> > + base_id = base_unfiltered->filename; > + if (base_unfiltered->drv) { > + base_fmt = base_unfiltered->drv->format_name; > + } > + } > } > } > bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
diff --git a/block/stream.c b/block/stream.c index 6e281c71ac..c208393c34 100644 --- a/block/stream.c +++ b/block/stream.c @@ -17,6 +17,7 @@ #include "block/blockjob_int.h" #include "qapi/error.h" #include "qapi/qmp/qerror.h" +#include "qemu/error-report.h" #include "qemu/ratelimit.h" #include "sysemu/block-backend.h" @@ -65,6 +66,8 @@ static int stream_prepare(Job *job) BlockDriverState *bs = blk_bs(bjob->blk); BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs); BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base); + BlockDriverState *base_unfiltered; + BlockDriverState *backing_bs; Error *local_err = NULL; int ret = 0; @@ -75,8 +78,22 @@ static int stream_prepare(Job *job) const char *base_id = NULL, *base_fmt = NULL; if (base) { base_id = s->backing_file_str; - if (base->drv) { - base_fmt = base->drv->format_name; + if (base_id) { + backing_bs = bdrv_find_backing_image(unfiltered_bs, base_id); + if (backing_bs && backing_bs->drv) { + base_fmt = backing_bs->drv->format_name; + } else { + error_report("Format not found for backing file %s", + s->backing_file_str); + } + } else { + base_unfiltered = bdrv_skip_filters(base); + if (base_unfiltered) { + base_id = base_unfiltered->filename; + if (base_unfiltered->drv) { + base_fmt = base_unfiltered->drv->format_name; + } + } } } bdrv_set_backing_hd(unfiltered_bs, base, &local_err); diff --git a/blockdev.c b/blockdev.c index c917625245..70900f4f77 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2508,7 +2508,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, BlockDriverState *base_bs = NULL; AioContext *aio_context; Error *local_err = NULL; - const char *base_name = NULL; int job_flags = JOB_DEFAULT; if (!has_on_error) { @@ -2536,7 +2535,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, goto out; } assert(bdrv_get_aio_context(base_bs) == aio_context); - base_name = base; } if (has_base_node) { @@ -2551,7 +2549,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, } assert(bdrv_get_aio_context(base_bs) == aio_context); bdrv_refresh_filename(base_bs); - base_name = base_bs->filename; } /* Check for op blockers in the whole chain between bs and base */ @@ -2571,9 +2568,6 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, goto out; } - /* backing_file string overrides base bs filename */ - base_name = has_backing_file ? backing_file : base_name; - if (has_auto_finalize && !auto_finalize) { job_flags |= JOB_MANUAL_FINALIZE; } @@ -2581,7 +2575,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, job_flags |= JOB_MANUAL_DISMISS; } - stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name, + stream_start(has_job_id ? job_id : NULL, bs, base_bs, backing_file, job_flags, has_speed ? speed : 0, on_error, filter_node_name, &local_err); if (local_err) {