Re: [Qemu-block] [PATCH 01/14] blockjob: Wake up BDS when job becomes idle

[Fam Zheng]

On Fri, 09/07 18:15, Kevin Wolf wrote:
> In the context of draining a BDS, the .drained_poll callback of block
> jobs is called. If this returns true (i.e. there is still some activity
> pending), the drain operation may call aio_poll() with blocking=true to
> wait for completion.
> 
> As soon as the pending activity is completed and the job finally arrives
> in a quiescent state (i.e. its coroutine either yields with busy=false
> or terminates), the block job must notify the aio_poll() loop to wake
> up, otherwise we get a deadlock if both are running in different
> threads.
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [PATCH 02/14] test-bdrv-drain: Drain with block jobs in an I/O thread

[Fam Zheng]

On Fri, 09/07 18:15, Kevin Wolf wrote:
> This extends the existing drain test with a block job to include
> variants where the block job runs in a different AioContext.
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [PATCH 03/14] test-blockjob: Acquire AioContext around job_finish_sync()

[Fam Zheng]

On Fri, 09/07 18:15, Kevin Wolf wrote:
> All callers in QEMU proper hold the AioContext lock when calling
> job_finish_sync(). test-blockjob should do the same.

I think s/job_finish_sync/job_cancel_sync/ in the subject is more accurate?

Reviewed-by: Fam Zheng 

> 
> Signed-off-by: Kevin Wolf 
> ---
>  include/qemu/job.h| 6 ++
>  tests/test-blockjob.c | 6 ++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 0dae5b8481..8ac48dbd28 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -520,6 +520,8 @@ void job_user_cancel(Job *job, bool force, Error **errp);
>   *
>   * Returns the return value from the job if the job actually completed
>   * during the call, or -ECANCELED if it was canceled.
> + *
> + * Callers must hold the AioContext lock of job->aio_context.
>   */
>  int job_cancel_sync(Job *job);
>  
> @@ -537,6 +539,8 @@ void job_cancel_sync_all(void);
>   * function).
>   *
>   * Returns the return value from the job.
> + *
> + * Callers must hold the AioContext lock of job->aio_context.
>   */
>  int job_complete_sync(Job *job, Error **errp);
>  
> @@ -579,6 +583,8 @@ void job_defer_to_main_loop(Job *job, 
> JobDeferToMainLoopFn *fn, void *opaque);
>   *
>   * Returns 0 if the job is successfully completed, -ECANCELED if the job was
>   * cancelled before completing, and -errno in other error cases.
> + *
> + * Callers must hold the AioContext lock of job->aio_context.
>   */
>  int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error 
> **errp);
>  
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index cb42f06e61..8c2babbe35 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -230,6 +230,10 @@ static void cancel_common(CancelJob *s)
>  BlockJob *job = &s->common;
>  BlockBackend *blk = s->blk;
>  JobStatus sts = job->job.status;
> +AioContext *ctx;
> +
> +ctx = job->job.aio_context;
> +aio_context_acquire(ctx);
>  
>  job_cancel_sync(&job->job);
>  if (sts != JOB_STATUS_CREATED && sts != JOB_STATUS_CONCLUDED) {
> @@ -239,6 +243,8 @@ static void cancel_common(CancelJob *s)
>  assert(job->job.status == JOB_STATUS_NULL);
>  job_unref(&job->job);
>  destroy_blk(blk);
> +
> +aio_context_release(ctx);
>  }
>  
>  static void test_cancel_created(void)
> -- 
> 2.13.6
> 

Re: [Qemu-block] [PATCH 04/14] job: Use AIO_WAIT_WHILE() in job_finish_sync()

[Fam Zheng]

On Fri, 09/07 18:15, Kevin Wolf wrote:
> job_finish_sync() needs to release the AioContext lock of the job before
> calling aio_poll(). Otherwise, callbacks called by aio_poll() would
> possibly take the lock a second time and run into a deadlock with a
> nested AIO_WAIT_WHILE() call.
> 
> Also, job_drain() without aio_poll() isn't necessarily enough to make
> progress on a job, it could depend on bottom halves to be executed.
> 
> Combine both open-coded while loops into a single AIO_WAIT_WHILE() call
> that solves both of these problems.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  job.c | 14 ++
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/job.c b/job.c
> index 9ad0b7476a..8480eda188 100644
> --- a/job.c
> +++ b/job.c
> @@ -29,6 +29,7 @@
>  #include "qemu/job.h"
>  #include "qemu/id.h"
>  #include "qemu/main-loop.h"
> +#include "block/aio-wait.h"
>  #include "trace-root.h"
>  #include "qapi/qapi-events-job.h"
>  
> @@ -998,6 +999,7 @@ void job_defer_to_main_loop(Job *job, 
> JobDeferToMainLoopFn *fn, void *opaque)
>  int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error 
> **errp)
>  {
>  Error *local_err = NULL;
> +AioWait dummy_wait = {};
>  int ret;
>  
>  job_ref(job);
> @@ -1010,14 +1012,10 @@ int job_finish_sync(Job *job, void (*finish)(Job *, 
> Error **errp), Error **errp)
>  job_unref(job);
>  return -EBUSY;
>  }
> -/* job_drain calls job_enter, and it should be enough to induce progress
> - * until the job completes or moves to the main thread. */
> -while (!job->deferred_to_main_loop && !job_is_completed(job)) {
> -job_drain(job);
> -}
> -while (!job_is_completed(job)) {
> -aio_poll(qemu_get_aio_context(), true);
> -}
> +
> +AIO_WAIT_WHILE(&dummy_wait, job->aio_context,
> +   (job_drain(job), !job_is_completed(job)));

The condition expression would read more elegant if job_drain() returns
progress.

Reviewed-by: Fam Zheng 

> +
>  ret = (job_is_cancelled(job) && job->ret == 0) ? -ECANCELED : job->ret;
>  job_unref(job);
>  return ret;
> -- 
> 2.13.6
> 

Re: [Qemu-block] [PATCH 05/14] test-bdrv-drain: Test AIO_WAIT_WHILE() in completion callback

[Fam Zheng]

On Fri, 09/07 18:15, Kevin Wolf wrote:
> This is a regression test for a deadlock that occurred in block job
> completion callbacks (via job_defer_to_main_loop) because the AioContext
> lock was taken twice: once in job_finish_sync() and then again in
> job_defer_to_main_loop_bh(). This would cause AIO_WAIT_WHILE() to hang.
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [PATCH 06/14] block: Add missing locking in bdrv_co_drain_bh_cb()

[Fam Zheng]

On Fri, 09/07 18:15, Kevin Wolf wrote:
> bdrv_do_drained_begin/end() assume that they are called with the
> AioContext lock of bs held. If we call drain functions from a coroutine
> with the AioContext lock held, we yield and schedule a BH to move out of
> coroutine context. This means that the lock for the home context of the
> coroutine is released and must be re-acquired in the bottom half.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  include/qemu/coroutine.h |  5 +
>  block/io.c   | 15 +++
>  util/qemu-coroutine.c|  5 +
>  3 files changed, 25 insertions(+)
> 
> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index 6f8a487041..9801e7f5a4 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -90,6 +90,11 @@ void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine 
> *co);
>  void coroutine_fn qemu_coroutine_yield(void);
>  
>  /**
> + * Get the AioContext of the given coroutine
> + */
> +AioContext *coroutine_fn qemu_coroutine_get_aio_context(Coroutine *co);
> +
> +/**
>   * Get the currently executing coroutine
>   */
>  Coroutine *coroutine_fn qemu_coroutine_self(void);
> diff --git a/block/io.c b/block/io.c
> index 7100344c7b..914ba78f1a 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -288,6 +288,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)
>  BlockDriverState *bs = data->bs;
>  
>  if (bs) {
> +AioContext *ctx = bdrv_get_aio_context(bs);
> +AioContext *co_ctx = qemu_coroutine_get_aio_context(co);
> +
> +/*
> + * When the coroutine yielded, the lock for its home context was
> + * released, so we need to re-acquire it here. If it explicitly
> + * acquired a different context, the lock is still held and we don't
> + * want to lock it a second time (or AIO_WAIT_WHILE() would hang).
> + */

This condition is rather obscure. When is ctx not equal to co_ctx?

> +if (ctx == co_ctx) {
> +aio_context_acquire(ctx);
> +}
>  bdrv_dec_in_flight(bs);
>  if (data->begin) {
>  bdrv_do_drained_begin(bs, data->recursive, data->parent,
> @@ -296,6 +308,9 @@ static void bdrv_co_drain_bh_cb(void *opaque)
>  bdrv_do_drained_end(bs, data->recursive, data->parent,
>  data->ignore_bds_parents);
>  }
> +if (ctx == co_ctx) {
> +aio_context_release(ctx);
> +}
>  } else {
>  assert(data->begin);
>  bdrv_drain_all_begin();
> diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
> index 1ba4191b84..2295928d33 100644
> --- a/util/qemu-coroutine.c
> +++ b/util/qemu-coroutine.c
> @@ -198,3 +198,8 @@ bool qemu_coroutine_entered(Coroutine *co)
>  {
>  return co->caller;
>  }
> +
> +AioContext *coroutine_fn qemu_coroutine_get_aio_context(Coroutine *co)
> +{
> +return co->ctx;
> +}
> -- 
> 2.13.6
> 

Re: [Qemu-block] [PATCH v2] qcow2: Release dirty entries with cache-clean-interval

[Alberto Garcia]

On Mon 10 Sep 2018 09:22:01 PM CEST, John Snow wrote:
> On 08/09/2018 09:44 AM, Alberto Garcia wrote:
>> The cache-clean-interval option is used to periodically release unused
>> entries from the L2 and refcount caches. Dirty cache entries are left
>> untouched, even if they are otherwise valid candidates for removal.
>> 
>> This patch allows releasing those entries by flushing them to disk
>> first. This is a blocking operation, so we need to do it in coroutine
>> context.
>> 
>> Signed-off-by: Alberto Garcia 
>
> This hit over a month old today, do we still want this for 3.1?

Well, it needs to be reviewed first :)

Berto

Re: [Qemu-block] [PATCH 07/14] aio-wait: Increase num_waiters even in home thread

[Fam Zheng]

On Fri, 09/07 18:15, Kevin Wolf wrote:
> Even if AIO_WAIT_WHILE() is called in the home context of the
> AioContext, we still want to allow the condition to change depending on
> other threads as long as they kick the AioWait. Specfically block jobs
> can be running in an I/O thread and should then be able to kick a drain
> in the main loop context.

We can now move the atomic_inc/atomic_dec pair outside the if/else block,
but that's cosmetic.

Reviewed-by: Fam Zheng 


> 
> Signed-off-by: Kevin Wolf 
> ---
>  include/block/aio-wait.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
> index c85a62f798..46ba7f9111 100644
> --- a/include/block/aio-wait.h
> +++ b/include/block/aio-wait.h
> @@ -77,10 +77,12 @@ typedef struct {
>  AioWait *wait_ = (wait);   \
>  AioContext *ctx_ = (ctx);  \
>  if (ctx_ && in_aio_context_home_thread(ctx_)) {\
> +atomic_inc(&wait_->num_waiters);   \
>  while ((cond)) {   \
>  aio_poll(ctx_, true);  \
>  waited_ = true;\
>  }  \
> +atomic_dec(&wait_->num_waiters);   \
>  } else {   \
>  assert(qemu_get_current_aio_context() ==   \
> qemu_get_aio_context());\
> -- 
> 2.13.6
> 

Re: [Qemu-block] [PATCH 08/14] block-backend: Add .drained_poll callback

[Fam Zheng]

On Fri, 09/07 18:15, Kevin Wolf wrote:
> A bdrv_drain operation must ensure that all parents are quiesced, this
> includes BlockBackends. Otherwise, callbacks called by requests that are
> completed on the BDS layer, but not quite yet on the BlockBackend layer
> could still create new requests.
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [PATCH 09/14] block-backend: Fix potential double blk_delete()

[Fam Zheng]

On Fri, 09/07 18:15, Kevin Wolf wrote:
> blk_unref() first decreases the refcount of the BlockBackend and calls
> blk_delete() if the refcount reaches zero. Requests can still be in
> flight at this point, they are only drained during blk_delete():
> 
> At this point, arbitrary callbacks can run. If any callback takes a
> temporary BlockBackend reference, it will first increase the refcount to
> 1 and then decrease it to 0 again, triggering another blk_delete(). This
> will cause a use-after-free crash in the outer blk_delete().
> 
> Fix it by draining the BlockBackend before decreasing to refcount to 0.
> Assert in blk_ref() that it never takes the first refcount (which would
> mean that the BlockBackend is already being deleted).
> 
> Signed-off-by: Kevin Wolf 

Good one!

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [PATCH 10/14] block-backend: Decrease in_flight only after callback

[Fam Zheng]

On Fri, 09/07 18:15, Kevin Wolf wrote:
> Request callbacks can do pretty much anything, including operations that
> will yield from the coroutine (such as draining the backend). In that
> case, a decreased in_flight would be visible to other code and could
> lead to a drain completing while the callback hasn't actually completed
> yet.
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [PATCH 11/14] mirror: Fix potential use-after-free in active commit

[Fam Zheng]

On Fri, 09/07 18:15, Kevin Wolf wrote:
> When starting an active commit job, other callbacks can run before
> mirror_start_job() calls bdrv_ref() where needed and cause the nodes to
> go away. Add another pair of bdrv_ref/unref() around it to protect
> against this case.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/mirror.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 6cc10df5c9..c42999eadf 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1679,6 +1679,11 @@ void commit_active_start(const char *job_id, 
> BlockDriverState *bs,
>  
>  orig_base_flags = bdrv_get_flags(base);
>  
> +/* bdrv_reopen() drains, which might make the BDSes go away before a
> + * reference is taken in mirror_start_job(). */
> +bdrv_ref(bs);
> +bdrv_ref(base);
> +
>  if (bdrv_reopen(base, bs->open_flags, errp)) {

Doesn't it need bdrv_unref's in this branch?

>  return;
>  }
> @@ -1689,6 +1694,10 @@ void commit_active_start(const char *job_id, 
> BlockDriverState *bs,
>   &commit_active_job_driver, false, base, auto_complete,
>   filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
>   &local_err);
> +
> +bdrv_unref(bs);
> +bdrv_unref(base);
> +
>  if (local_err) {
>  error_propagate(errp, local_err);
>  goto error_restore_flags;
> -- 
> 2.13.6
> 

Re: [Qemu-block] [PATCH 12/14] blockjob: Lie better in child_job_drained_poll()

[Fam Zheng]

On Fri, 09/07 18:15, Kevin Wolf wrote:
> Block jobs claim in .drained_poll() that they are in a quiescent state
> as soon as job->deferred_to_main_loop is true. This is obviously wrong,
> they still have a completion BH to run. We only get away with this
> because commit 91af091f923 added an unconditional aio_poll(false) to the
> drain functions, but this is bypassing the regular drain mechanisms.
> 
> However, just removing this and telling that the job is still active
> doesn't work either: The completion callbacks themselves call drain
> functions (directly, or indirectly with bdrv_reopen), so they would
> deadlock then.
> 
> As a better lie, tell that the job is active as long as the BH is
> pending, but falsely call it quiescent from the point in the BH when the
> completion callback is called. At this point, nested drain calls won't
> deadlock because they ignore the job, and outer drains will wait for the
> job to really reach a quiescent state because the callback is already
> running.
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [PATCH 13/14] block: Remove aio_poll() in bdrv_drain_poll variants

[Fam Zheng]

On Fri, 09/07 18:15, Kevin Wolf wrote:
> bdrv_drain_poll_top_level() was buggy because it didn't release the
> AioContext lock of the node to be drained before calling aio_poll().
> This way, callbacks called by aio_poll() would possibly take the lock a
> second time and run into a deadlock with a nested AIO_WAIT_WHILE() call.
> 
> However, it turns out that the aio_poll() call isn't actually needed any
> more. It was introduced in commit 91af091f923, which is effectively
> reverted by this patch. The cases it was supposed to fix are now covered
> by bdrv_drain_poll(), which waits for block jobs to reach a quiescent
> state.
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [PATCH 14/14] test-bdrv-drain: Test nested poll in bdrv_drain_poll_top_level()

[Fam Zheng]

On Fri, 09/07 18:15, Kevin Wolf wrote:
> This is a regression test for a deadlock that could occur in callbacks
> called from the aio_poll() in bdrv_drain_poll_top_level(). The
> AioContext lock wasn't released and therefore would be taken a second
> time in the callback. This would cause a possible AIO_WAIT_WHILE() in
> the callback to hang.
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [PATCH for-3.1 v10 04/31] block: Add BDS.auto_backing_file

[Alberto Garcia]

On Thu 09 Aug 2018 11:35:01 PM CEST, Max Reitz wrote:
> @@ -295,11 +296,13 @@ static int qcow_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  goto fail;
>  }
>  ret = bdrv_pread(bs->file, header.backing_file_offset,
> -   bs->backing_file, len);
> +   bs->auto_backing_file, len);
>  if (ret < 0) {
>  goto fail;
>  }
> -bs->backing_file[len] = '\0';
> +bs->auto_backing_file[len] = '\0';
> +pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> +bs->auto_backing_file);
>  }

The patch looks fine, I just wonder why you don't simply copy
bs->backing_file into bs->auto_backing_file instead of the other way
around, it would make the patch smaller.

Either way,

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues

[Kevin Wolf]

Am 10.09.2018 um 18:51 hat Max Reitz geschrieben:
> On 10.09.18 17:18, Kevin Wolf wrote:
> > Am 09.08.2018 um 23:34 hat Max Reitz geschrieben:
> >> Once more, I’ll spare both me and you another iteration of the cover
> >> letter, so see here:
> >>
> >> http://lists.nongnu.org/archive/html/qemu-block/2017-09/msg01030.html
> >>
> >> (Although this series no longer includes a @base-directory option.)
> >>
> >> In regards to the last version, the biggest change is that I dropped
> >> backing_overridden and instead try to compare the filename from the
> >> image header with the filename of the actual backing BDS to find out
> >> whether the backing file has been overridden.
> >>
> >> In order that this doesn’t break whenever the header contains a slightly
> >> unusual (“non-canonical”) backing filename (e.g. “file:foo.qcow2” or
> >> “nbd:localhost:10809” instead of “nbd://localhost:10809”, i.e. something
> >> different from what bdrv_refresh_filename() would generate), when the
> >> reference filename in the BDS (auto_backing_file) is used to open the
> >> backing file, it is updated from the backing BDS's resulting filename.
> > 
> > This doesn't seem to apply cleanly any more.
> 
> I know, but is that a "Please send your current v11 to the list"?

Depends on how urgently you want me to review this. I don't like
reviewing patches directly from emails, without being able to apply
them.

Or maybe if you just can give me the commit ID this is based on? I
wouldn't mind applying it to an old tree as long as I can apply it
somewhere.

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 06/14] block: Add missing locking in bdrv_co_drain_bh_cb()

[Kevin Wolf]

Am 11.09.2018 um 10:23 hat Fam Zheng geschrieben:
> On Fri, 09/07 18:15, Kevin Wolf wrote:
> > bdrv_do_drained_begin/end() assume that they are called with the
> > AioContext lock of bs held. If we call drain functions from a coroutine
> > with the AioContext lock held, we yield and schedule a BH to move out of
> > coroutine context. This means that the lock for the home context of the
> > coroutine is released and must be re-acquired in the bottom half.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  include/qemu/coroutine.h |  5 +
> >  block/io.c   | 15 +++
> >  util/qemu-coroutine.c|  5 +
> >  3 files changed, 25 insertions(+)
> > 
> > diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> > index 6f8a487041..9801e7f5a4 100644
> > --- a/include/qemu/coroutine.h
> > +++ b/include/qemu/coroutine.h
> > @@ -90,6 +90,11 @@ void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine 
> > *co);
> >  void coroutine_fn qemu_coroutine_yield(void);
> >  
> >  /**
> > + * Get the AioContext of the given coroutine
> > + */
> > +AioContext *coroutine_fn qemu_coroutine_get_aio_context(Coroutine *co);
> > +
> > +/**
> >   * Get the currently executing coroutine
> >   */
> >  Coroutine *coroutine_fn qemu_coroutine_self(void);
> > diff --git a/block/io.c b/block/io.c
> > index 7100344c7b..914ba78f1a 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -288,6 +288,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)
> >  BlockDriverState *bs = data->bs;
> >  
> >  if (bs) {
> > +AioContext *ctx = bdrv_get_aio_context(bs);
> > +AioContext *co_ctx = qemu_coroutine_get_aio_context(co);
> > +
> > +/*
> > + * When the coroutine yielded, the lock for its home context was
> > + * released, so we need to re-acquire it here. If it explicitly
> > + * acquired a different context, the lock is still held and we 
> > don't
> > + * want to lock it a second time (or AIO_WAIT_WHILE() would hang).
> > + */
> 
> This condition is rather obscure. When is ctx not equal to co_ctx?

Whenever you drain a BlockDriverState that is in a different AioContext.
The common case is a bdrv_drain() from the main loop thread for a BDS in
an iothread.

I didn't have this condition at first and ran into deadlocks (because
AIO_WAIT_WHILE() dropped the lock only once, but it was locked twice).

Kevin

> > +if (ctx == co_ctx) {
> > +aio_context_acquire(ctx);
> > +}
> >  bdrv_dec_in_flight(bs);
> >  if (data->begin) {
> >  bdrv_do_drained_begin(bs, data->recursive, data->parent,
> > @@ -296,6 +308,9 @@ static void bdrv_co_drain_bh_cb(void *opaque)
> >  bdrv_do_drained_end(bs, data->recursive, data->parent,
> >  data->ignore_bds_parents);
> >  }
> > +if (ctx == co_ctx) {
> > +aio_context_release(ctx);
> > +}
> >  } else {
> >  assert(data->begin);
> >  bdrv_drain_all_begin();

Re: [Qemu-block] [PATCH 11/14] mirror: Fix potential use-after-free in active commit

[Kevin Wolf]

Am 11.09.2018 um 10:31 hat Fam Zheng geschrieben:
> On Fri, 09/07 18:15, Kevin Wolf wrote:
> > When starting an active commit job, other callbacks can run before
> > mirror_start_job() calls bdrv_ref() where needed and cause the nodes to
> > go away. Add another pair of bdrv_ref/unref() around it to protect
> > against this case.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  block/mirror.c | 9 +
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 6cc10df5c9..c42999eadf 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -1679,6 +1679,11 @@ void commit_active_start(const char *job_id, 
> > BlockDriverState *bs,
> >  
> >  orig_base_flags = bdrv_get_flags(base);
> >  
> > +/* bdrv_reopen() drains, which might make the BDSes go away before a
> > + * reference is taken in mirror_start_job(). */
> > +bdrv_ref(bs);
> > +bdrv_ref(base);
> > +
> >  if (bdrv_reopen(base, bs->open_flags, errp)) {
> 
> Doesn't it need bdrv_unref's in this branch?

Yes, of course. Thanks for catching this!

Kevin

Re: [Qemu-block] [PATCH 06/14] block: Add missing locking in bdrv_co_drain_bh_cb()

[Sergio Lopez]

On Tue, Sep 11, 2018 at 11:17:20AM +0200, Kevin Wolf wrote:
> Am 11.09.2018 um 10:23 hat Fam Zheng geschrieben:
> > On Fri, 09/07 18:15, Kevin Wolf wrote:
> > > bdrv_do_drained_begin/end() assume that they are called with the
> > > AioContext lock of bs held. If we call drain functions from a coroutine
> > > with the AioContext lock held, we yield and schedule a BH to move out of
> > > coroutine context. This means that the lock for the home context of the
> > > coroutine is released and must be re-acquired in the bottom half.
> > > 
> > > Signed-off-by: Kevin Wolf 
> > > ---
> > >  include/qemu/coroutine.h |  5 +
> > >  block/io.c   | 15 +++
> > >  util/qemu-coroutine.c|  5 +
> > >  3 files changed, 25 insertions(+)
> > > 
> > > diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> > > index 6f8a487041..9801e7f5a4 100644
> > > --- a/include/qemu/coroutine.h
> > > +++ b/include/qemu/coroutine.h
> > > @@ -90,6 +90,11 @@ void qemu_aio_coroutine_enter(AioContext *ctx, 
> > > Coroutine *co);
> > >  void coroutine_fn qemu_coroutine_yield(void);
> > >  
> > >  /**
> > > + * Get the AioContext of the given coroutine
> > > + */
> > > +AioContext *coroutine_fn qemu_coroutine_get_aio_context(Coroutine *co);
> > > +
> > > +/**
> > >   * Get the currently executing coroutine
> > >   */
> > >  Coroutine *coroutine_fn qemu_coroutine_self(void);
> > > diff --git a/block/io.c b/block/io.c
> > > index 7100344c7b..914ba78f1a 100644
> > > --- a/block/io.c
> > > +++ b/block/io.c
> > > @@ -288,6 +288,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)
> > >  BlockDriverState *bs = data->bs;
> > >  
> > >  if (bs) {
> > > +AioContext *ctx = bdrv_get_aio_context(bs);
> > > +AioContext *co_ctx = qemu_coroutine_get_aio_context(co);
> > > +
> > > +/*
> > > + * When the coroutine yielded, the lock for its home context was
> > > + * released, so we need to re-acquire it here. If it explicitly
> > > + * acquired a different context, the lock is still held and we 
> > > don't
> > > + * want to lock it a second time (or AIO_WAIT_WHILE() would 
> > > hang).
> > > + */
> > 
> > This condition is rather obscure. When is ctx not equal to co_ctx?
> 
> Whenever you drain a BlockDriverState that is in a different AioContext.
> The common case is a bdrv_drain() from the main loop thread for a BDS in
> an iothread.

Isn't this a consequence of using qemu_coroutine_enter in co_schedule_bh
[1]?

AFAIK, even if an IOThread's AioContext is being polled by the main loop
thread, all coroutines should be running with the IOThread/BDS
AioContext.

Sergio.

[1] https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg00450.html

Re: [Qemu-block] [PATCH 06/14] block: Add missing locking in bdrv_co_drain_bh_cb()

[Kevin Wolf]

Am 11.09.2018 um 11:28 hat Sergio Lopez geschrieben:
> On Tue, Sep 11, 2018 at 11:17:20AM +0200, Kevin Wolf wrote:
> > Am 11.09.2018 um 10:23 hat Fam Zheng geschrieben:
> > > On Fri, 09/07 18:15, Kevin Wolf wrote:
> > > > bdrv_do_drained_begin/end() assume that they are called with the
> > > > AioContext lock of bs held. If we call drain functions from a coroutine
> > > > with the AioContext lock held, we yield and schedule a BH to move out of
> > > > coroutine context. This means that the lock for the home context of the
> > > > coroutine is released and must be re-acquired in the bottom half.
> > > > 
> > > > Signed-off-by: Kevin Wolf 
> > > > ---
> > > >  include/qemu/coroutine.h |  5 +
> > > >  block/io.c   | 15 +++
> > > >  util/qemu-coroutine.c|  5 +
> > > >  3 files changed, 25 insertions(+)
> > > > 
> > > > diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> > > > index 6f8a487041..9801e7f5a4 100644
> > > > --- a/include/qemu/coroutine.h
> > > > +++ b/include/qemu/coroutine.h
> > > > @@ -90,6 +90,11 @@ void qemu_aio_coroutine_enter(AioContext *ctx, 
> > > > Coroutine *co);
> > > >  void coroutine_fn qemu_coroutine_yield(void);
> > > >  
> > > >  /**
> > > > + * Get the AioContext of the given coroutine
> > > > + */
> > > > +AioContext *coroutine_fn qemu_coroutine_get_aio_context(Coroutine *co);
> > > > +
> > > > +/**
> > > >   * Get the currently executing coroutine
> > > >   */
> > > >  Coroutine *coroutine_fn qemu_coroutine_self(void);
> > > > diff --git a/block/io.c b/block/io.c
> > > > index 7100344c7b..914ba78f1a 100644
> > > > --- a/block/io.c
> > > > +++ b/block/io.c
> > > > @@ -288,6 +288,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)
> > > >  BlockDriverState *bs = data->bs;
> > > >  
> > > >  if (bs) {
> > > > +AioContext *ctx = bdrv_get_aio_context(bs);
> > > > +AioContext *co_ctx = qemu_coroutine_get_aio_context(co);
> > > > +
> > > > +/*
> > > > + * When the coroutine yielded, the lock for its home context 
> > > > was
> > > > + * released, so we need to re-acquire it here. If it explicitly
> > > > + * acquired a different context, the lock is still held and we 
> > > > don't
> > > > + * want to lock it a second time (or AIO_WAIT_WHILE() would 
> > > > hang).
> > > > + */
> > > 
> > > This condition is rather obscure. When is ctx not equal to co_ctx?
> > 
> > Whenever you drain a BlockDriverState that is in a different AioContext.
> > The common case is a bdrv_drain() from the main loop thread for a BDS in
> > an iothread.
> 
> Isn't this a consequence of using qemu_coroutine_enter in co_schedule_bh
> [1]?
> 
> AFAIK, even if an IOThread's AioContext is being polled by the main loop
> thread, all coroutines should be running with the IOThread/BDS
> AioContext.

You're right, bdrv_co_yield_to_drain() does schedule the BH in the
AioContext of the BDS, so in theory this shouldn't happen. If it was
called from a coroutine with a wrong co->ctx (due to the bug you
mentioned), that would explain the behaviour. Maybe the condition isn't
necessary any more after your fix.

Kevin

Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler

[Paolo Bonzini]

On 10/09/2018 16:56, Fam Zheng wrote:
> We have this unwanted call stack:
> 
>   > ...
>   > #13 0x5586602b7793 in virtio_scsi_handle_cmd_vq
>   > #14 0x5586602b8d66 in virtio_scsi_data_plane_handle_cmd
>   > #15 0x5586602ddab7 in virtio_queue_notify_aio_vq
>   > #16 0x5586602dfc9f in virtio_queue_host_notifier_aio_poll
>   > #17 0x5586607885da in run_poll_handlers_once
>   > #18 0x55866078880e in try_poll_mode
>   > #19 0x5586607888eb in aio_poll
>   > #20 0x558660784561 in aio_wait_bh_oneshot
>   > #21 0x5586602b9582 in virtio_scsi_dataplane_stop
>   > #22 0x5586605a7110 in virtio_bus_stop_ioeventfd
>   > #23 0x5586605a9426 in virtio_pci_stop_ioeventfd
>   > #24 0x5586605ab808 in virtio_pci_common_write
>   > #25 0x558660242396 in memory_region_write_accessor
>   > #26 0x5586602425ab in access_with_adjusted_size
>   > #27 0x558660245281 in memory_region_dispatch_write
>   > #28 0x5586601e008e in flatview_write_continue
>   > #29 0x5586601e01d8 in flatview_write
>   > #30 0x5586601e04de in address_space_write
>   > #31 0x5586601e052f in address_space_rw
>   > #32 0x5586602607f2 in kvm_cpu_exec
>   > #33 0x558660227148 in qemu_kvm_cpu_thread_fn
>   > #34 0x55866078bde7 in qemu_thread_start
>   > #35 0x7f5784906594 in start_thread
>   > #36 0x7f5784639e6f in clone
> 
> Avoid it with the aio_disable_external/aio_enable_external pair, so that
> no vq poll handlers can be called in aio_wait_bh_oneshot.

I don't understand.  We are in the vCPU thread, so not in the
AioContext's home thread.  Why is aio_wait_bh_oneshot polling rather
than going through the aio_wait_bh path?

Thanks,

Paolo

[Qemu-block] [PATCH v1 1/1] qemu-img: add new function to remove bitmap in image

[Ma Haocong]

Signed-off-by: Ma Haocong 
---
 qemu-img-cmds.hx |   6 +++
 qemu-img.c   | 119 +++
 2 files changed, 125 insertions(+)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 1526f327a5..cc397b64e4 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -97,6 +97,12 @@ STEXI
 @item resize [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] 
[--preallocation=@var{prealloc}] [-q] [--shrink] @var{filename} [+ | 
-]@var{size}
 ETEXI
 
+DEF("removebmp", img_removebmp,
+"removebmp [--object objectdef] [--image-opts] [-q] [-f fmt] filename 
dirtybitmap")
+STEXI
+@item removebmp [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] 
@var{filename} @var{dirtybitmap}
+ETEXI
+
 STEXI
 @end table
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index b12f4cd19b..fdafb4a131 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -835,6 +835,125 @@ fail:
 return ret;
 }
 
+/*
+ * Remove a named dirty bitmap in image.
+ * This command should be used with no other QEMU process
+ * open the image at the same time. Otherwise it may be
+ * croputed the bitmap even the image.
+ */
+static int img_removebmp(int argc, char **argv)
+{
+int   c, ret;
+const char   *filename, *fmt, *bitmapname;
+bool  quiet = false;
+BlockBackend *blk;
+BlockDriverState *bs;
+BdrvDirtyBitmap  *bitmap;
+Error*local_err = NULL;
+bool image_opts = false;
+fmt = NULL;
+
+for (;;) {
+int option_index = 0;
+static const struct option long_options[] = {
+{"help", no_argument, 0, 'h'},
+{"format", required_argument, 0, 'f'},
+{"object", required_argument, 0, OPTION_OBJECT},
+{"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+{0, 0, 0, 0}
+};
+c = getopt_long(argc, argv, ":hf:q",
+long_options, &option_index);
+if (c == -1) {
+break;
+}
+switch (c) {
+case ':':
+missing_argument(argv[optind - 1]);
+break;
+case '?':
+unrecognized_option(argv[optind - 1]);
+break;
+case 'h':
+help();
+break;
+case 'f':
+fmt = optarg;
+break;
+case 'q':
+quiet = true;
+break;
+case OPTION_OBJECT: {
+QemuOpts *opts;
+opts = qemu_opts_parse_noisily(&qemu_object_opts,
+   optarg, true);
+if (!opts) {
+return 1;
+}
+}   break;
+case OPTION_IMAGE_OPTS:
+image_opts = true;
+break;
+}
+}
+
+if (optind != argc - 2) {
+error_exit("Expecting two parameters");
+}
+filename = argv[optind++];
+bitmapname = argv[optind++];
+
+if (qemu_opts_foreach(&qemu_object_opts,
+  user_creatable_add_opts_foreach,
+  NULL, NULL)) {
+return 1;
+}
+
+blk = img_open(image_opts, filename, fmt,
+   BDRV_O_RDWR, false, quiet, false);
+if (!blk) {
+ret = -1;
+goto out;
+}
+bs = blk_bs(blk);
+if (!bs) {
+ret = -1;
+goto out;
+}
+
+bitmap = bdrv_find_dirty_bitmap(bs, bitmapname);
+
+/*
+ * Dirty bitmap may not be load if the 'IN_USE' flag is set (e.g. the
+ * qemu thread is corrupted and the 'IN_USE' flag is not be cleared),
+ * so the result of bdrv_find_dirty_bitmap is null. In this case,
+ * we delete bitmap in qcow2 file directly.
+*/
+if (!bitmap) {
+bdrv_remove_persistent_dirty_bitmap(bs, bitmapname, &local_err);
+if (local_err != NULL) {
+ret = -1;
+goto out;
+}
+} else {
+if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
+bdrv_remove_persistent_dirty_bitmap(bs, bitmapname, &local_err);
+if (local_err != NULL) {
+ret = -1;
+goto out;
+}
+}
+bdrv_release_dirty_bitmap(bs, bitmap);
+}
+
+out:
+blk_unref(blk);
+if (ret) {
+return 1;
+}
+return 0;
+}
+
 typedef struct CommonBlockJobCBInfo {
 BlockDriverState *bs;
 Error **errp;
-- 
2.14.1

[Qemu-block] [PATCH v1 0/1] qemu-img: add new function to remove bitmap in image

[Ma Haocong]

Hello,

In our scene, we need to delete dirty-bitmap created by using qmp command 
'block-dirty-bitmap-add'. we can use qmp command 'block-dirty-bitmap-remove'
to remove bitmap. Then I think that we should add a new function in qemu-img
to do the same work. 

The command format is: qemu-img removebmp file-name bitmap-name

I test the function by using qmp command to create a named dirty-bitmap in
qcow2 image, then shutdown the vm and using qemu-img to remove it. Then I
test to remove a bitmap with 'IN_USE' flags in qcow2 file, and it works.
I alse test to do the same job to the qcow2 image encrypted by luks.

Please help to review. Thanks.

mahaocong (1):
  Add new command 'removebmp' to remove bitmap in qcow2 file. The format
is: qemu-img removebmp qcow2-file-name bitmap-name

 qemu-img-cmds.hx |   6 +++
 qemu-img.c   | 119 +++
 2 files changed, 125 insertions(+)

-- 
2.14.1

[Qemu-block] [PATCH] block: Don't call update_flags_from_options() if the options are wrong

[Alberto Garcia]

If qemu_opts_absorb_qdict() fails and we carry on and call
update_flags_from_options() then that can result on a failed
assertion:

   $ qemu-io -c 'reopen -o read-only=foo' hd.qcow2
   block.c:1101: update_flags_from_options: Assertion `qemu_opt_find(opts, 
BDRV_OPT_CACHE_DIRECT)' failed.
   Aborted

This only happens in bdrv_reopen_queue_child(). Although this function
doesn't return errors, we can simply keep the wrong options in the
reopen queue and later bdrv_reopen_prepare() will fail.

Signed-off-by: Alberto Garcia 
---
 block.c| 11 +--
 tests/qemu-iotests/133 |  6 ++
 tests/qemu-iotests/133.out |  4 
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 0dbb1fcc7b..739d9073a6 100644
--- a/block.c
+++ b/block.c
@@ -2931,12 +2931,19 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 if (parent_options) {
 QemuOpts *opts;
 QDict *options_copy;
+Error *local_err = NULL;
 assert(!flags);
 role->inherit_options(&flags, options, parent_flags, parent_options);
 options_copy = qdict_clone_shallow(options);
 opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
-qemu_opts_absorb_qdict(opts, options_copy, NULL);
-update_flags_from_options(&flags, opts);
+qemu_opts_absorb_qdict(opts, options_copy, &local_err);
+/* Don't call update_flags_from_options() if the options are wrong.
+ * bdrv_reopen_prepare() will later return an error. */
+if (!local_err) {
+update_flags_from_options(&flags, opts);
+} else {
+error_free(local_err);
+}
 qemu_opts_del(opts);
 qobject_unref(options_copy);
 }
diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
index af6b3e1dd4..b9f17c996f 100755
--- a/tests/qemu-iotests/133
+++ b/tests/qemu-iotests/133
@@ -92,6 +92,12 @@ echo
 IMGOPTSSYNTAX=false $QEMU_IO -f null-co -c 'reopen' -c 'info' \
 "json:{'driver': 'null-co', 'size': 65536}"
 
+echo
+echo "=== Check that invalid options are handled correctly ==="
+echo
+
+$QEMU_IO -c 'reopen -o read-only=foo' $TEST_IMG
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
index f4a85aeb63..570f623b6b 100644
--- a/tests/qemu-iotests/133.out
+++ b/tests/qemu-iotests/133.out
@@ -24,4 +24,8 @@ Cannot change the option 'driver'
 
 format name: null-co
 format name: null-co
+
+=== Check that invalid options are handled correctly ===
+
+Parameter 'read-only' expects 'on' or 'off'
 *** done
-- 
2.11.0

Re: [Qemu-block] [PATCH 0/3] util: add qemu_write_pidfile()

[Paolo Bonzini]

On 31/08/2018 16:53, Marc-André Lureau wrote:
> Hi,
> 
> Here are a few PID file related patches extracted from "[PATCH v4
> 00/29] vhost-user for input & GPU" series, with suggestions from
> Daniel Berrangé.
> 
> thanks
> 
> Marc-André Lureau (3):
>   util: add qemu_write_pidfile()
>   util: use fcntl() for qemu_write_pidfile() locking
>   RFC: delete PID file on exit
> 
>  include/qemu/osdep.h  |  3 +-
>  os-posix.c| 24 --
>  os-win32.c| 25 ---
>  qga/main.c| 54 ++--
>  scsi/qemu-pr-helper.c | 40 +++-
>  util/oslib-posix.c| 73 +++
>  util/oslib-win32.c| 27 
>  vl.c  | 15 +++--
>  8 files changed, 129 insertions(+), 132 deletions(-)
> 

Queued 1-2, thanks.

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/3] util: add qemu_write_pidfile()

[Paolo Bonzini]

On 07/09/2018 16:03, Eric Blake wrote:
> On 09/07/2018 07:13 AM, Marc-André Lureau wrote:
>> There are variants of qemu_create_pidfile() in qemu-pr-helper and
>> qemu-ga. Let's have a common implementation in libqemuutil.
>>
> 
> Unrelated to this patch, but a question that this raises: should
> 'qemu-nbd' also have a mode for creating a pid file, since it has --fork
> for daemonizing the server process?

Yes, it's probably a good idea.  --fork mode is used mostly with -c/-d,
but why not.

Paolo

Re: [Qemu-block] [PATCH v2 3/3] Delete PID file on exit

[Paolo Bonzini]

On 07/09/2018 14:13, Marc-André Lureau wrote:
> Register an exit notifier to remove the PID file. By the time atexit()
> is called, qemu_write_pidfile() guarantees QEMU owns the PID file,
> thus we could safely remove it when exiting.
> 
> Signed-off-by: Marc-André Lureau 

Queued this one, too.

Paolo

> ---
>  vl.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index 0eaf948d32..51af016602 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2603,6 +2603,16 @@ static void qemu_run_exit_notifiers(void)
>  notifier_list_notify(&exit_notifiers, NULL);
>  }
>  
> +static const char *pid_file;
> +static Notifier qemu_unlink_pidfile_notifier;
> +
> +static void qemu_unlink_pidfile(Notifier *n, void *data)
> +{
> +if (pid_file) {
> +unlink(pid_file);
> +}
> +}
> +
>  bool machine_init_done;
>  
>  void qemu_add_machine_init_done_notifier(Notifier *notify)
> @@ -2927,7 +2937,6 @@ int main(int argc, char **argv, char **envp)
>  const char *vga_model = NULL;
>  const char *qtest_chrdev = NULL;
>  const char *qtest_log = NULL;
> -const char *pid_file = NULL;
>  const char *incoming = NULL;
>  bool userconfig = true;
>  bool nographic = false;
> @@ -4001,6 +4010,9 @@ int main(int argc, char **argv, char **envp)
>  exit(1);
>  }
>  
> +qemu_unlink_pidfile_notifier.notify = qemu_unlink_pidfile;
> +qemu_add_exit_notifier(&qemu_unlink_pidfile_notifier);
> +
>  if (qemu_init_main_loop(&main_loop_err)) {
>  error_report_err(main_loop_err);
>  exit(1);
> 

Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/1] qemu-img: add new function to remove bitmap in image

[Eric Blake]

On 9/11/18 3:37 AM, Ma Haocong wrote:

Signed-off-by: Ma Haocong 
---
  qemu-img-cmds.hx |   6 +++
  qemu-img.c   | 119 +++
  2 files changed, 125 insertions(+)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 1526f327a5..cc397b64e4 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -97,6 +97,12 @@ STEXI
  @item resize [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] 
[--preallocation=@var{prealloc}] [-q] [--shrink] @var{filename} [+ | 
-]@var{size}
  ETEXI
  
+DEF("removebmp", img_removebmp,


Not alphabetical - if we keep this name (which I'm already questioning), 
this should come prior to 'resize'.



+"removebmp [--object objectdef] [--image-opts] [-q] [-f fmt] filename 
dirtybitmap")
+STEXI
+@item removebmp [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] 
@var{filename} @var{dirtybitmap}
+ETEXI
+
  STEXI
  @end table
  ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index b12f4cd19b..fdafb4a131 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -835,6 +835,125 @@ fail:
  return ret;
  }
  
+/*

+ * Remove a named dirty bitmap in image.
+ * This command should be used with no other QEMU process
+ * open the image at the same time. Otherwise it may be
+ * croputed the bitmap even the image.


s/be croputed/corrupt/
s/even/or even/

However, the last sentence is not adding anything useful - we already 
have image locking that should make it impossible to attempt to remove a 
bitmap while some other qemu process has the image open.



+ */
+static int img_removebmp(int argc, char **argv)
+{
+int   c, ret;
+const char   *filename, *fmt, *bitmapname;
+bool  quiet = false;
+BlockBackend *blk;
+BlockDriverState *bs;
+BdrvDirtyBitmap  *bitmap;
+Error*local_err = NULL;
+bool image_opts = false;
+fmt = NULL;
+
+for (;;) {
+int option_index = 0;
+static const struct option long_options[] = {
+{"help", no_argument, 0, 'h'},
+{"format", required_argument, 0, 'f'},
+{"object", required_argument, 0, OPTION_OBJECT},
+{"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+{0, 0, 0, 0}
+};
+c = getopt_long(argc, argv, ":hf:q",
+long_options, &option_index);



+bitmap = bdrv_find_dirty_bitmap(bs, bitmapname);
+
+/*
+ * Dirty bitmap may not be load if the 'IN_USE' flag is set (e.g. the
+ * qemu thread is corrupted and the 'IN_USE' flag is not be cleared),
+ * so the result of bdrv_find_dirty_bitmap is null. In this case,
+ * we delete bitmap in qcow2 file directly.
+*/
+if (!bitmap) {
+bdrv_remove_persistent_dirty_bitmap(bs, bitmapname, &local_err);
+if (local_err != NULL) {
+ret = -1;
+goto out;
+}
+} else {
+if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
+bdrv_remove_persistent_dirty_bitmap(bs, bitmapname, &local_err);
+if (local_err != NULL) {
+ret = -1;
+goto out;
+}
+}
+bdrv_release_dirty_bitmap(bs, bitmap);
+}


Why aren't you calling bdrv_block_dirty_bitmap_remove()?  In general, 
HMP commands that are mere wrappers around counterpart QMP commands are 
easier to maintain, rather than open-coding the same work in two places.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [Qemu-devel] [PATCH v1 0/1] qemu-img: add new function to remove bitmap in image

[Eric Blake]

On 9/11/18 3:37 AM, Ma Haocong wrote:

Hello,

In our scene, we need to delete dirty-bitmap created by using qmp command
'block-dirty-bitmap-add'. we can use qmp command 'block-dirty-bitmap-remove'
to remove bitmap. Then I think that we should add a new function in qemu-img
to do the same work.

The command format is: qemu-img removebmp file-name bitmap-name


John was working on a more general 'qemu-img bitmap' command that did 
multiple operations from qemu-img, rather than just remove. That feels 
like a more extensible approach than adding a new command for every 
individual bitmap operation.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/1] qemu-img: add new function to remove bitmap in image

[Eric Blake]

On 9/11/18 8:56 AM, Eric Blake wrote:


+    bitmap = bdrv_find_dirty_bitmap(bs, bitmapname);
+
+    /*
+ * Dirty bitmap may not be load if the 'IN_USE' flag is set (e.g. 
the
+ * qemu thread is corrupted and the 'IN_USE' flag is not be 
cleared),

+ * so the result of bdrv_find_dirty_bitmap is null. In this case,
+ * we delete bitmap in qcow2 file directly.
+    */
+    if (!bitmap) {
+    bdrv_remove_persistent_dirty_bitmap(bs, bitmapname, &local_err);
+    if (local_err != NULL) {
+    ret = -1;
+    goto out;
+    }
+    } else {
+    if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
+    bdrv_remove_persistent_dirty_bitmap(bs, bitmapname, 
&local_err);

+    if (local_err != NULL) {
+    ret = -1;
+    goto out;
+    }
+    }
+    bdrv_release_dirty_bitmap(bs, bitmap);
+    }


Why aren't you calling bdrv_block_dirty_bitmap_remove()?  In general, 


It helps if I ask my actual intended question: Why aren't you calling 
qmp_block_dirty_bitmap_remove()?


HMP commands that are mere wrappers around counterpart QMP commands are 
easier to maintain, rather than open-coding the same work in two places.




--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler

[Fam Zheng]

On Tue, 09/11 13:32, Paolo Bonzini wrote:
> On 10/09/2018 16:56, Fam Zheng wrote:
> > We have this unwanted call stack:
> > 
> >   > ...
> >   > #13 0x5586602b7793 in virtio_scsi_handle_cmd_vq
> >   > #14 0x5586602b8d66 in virtio_scsi_data_plane_handle_cmd
> >   > #15 0x5586602ddab7 in virtio_queue_notify_aio_vq
> >   > #16 0x5586602dfc9f in virtio_queue_host_notifier_aio_poll
> >   > #17 0x5586607885da in run_poll_handlers_once
> >   > #18 0x55866078880e in try_poll_mode
> >   > #19 0x5586607888eb in aio_poll
> >   > #20 0x558660784561 in aio_wait_bh_oneshot
> >   > #21 0x5586602b9582 in virtio_scsi_dataplane_stop
> >   > #22 0x5586605a7110 in virtio_bus_stop_ioeventfd
> >   > #23 0x5586605a9426 in virtio_pci_stop_ioeventfd
> >   > #24 0x5586605ab808 in virtio_pci_common_write
> >   > #25 0x558660242396 in memory_region_write_accessor
> >   > #26 0x5586602425ab in access_with_adjusted_size
> >   > #27 0x558660245281 in memory_region_dispatch_write
> >   > #28 0x5586601e008e in flatview_write_continue
> >   > #29 0x5586601e01d8 in flatview_write
> >   > #30 0x5586601e04de in address_space_write
> >   > #31 0x5586601e052f in address_space_rw
> >   > #32 0x5586602607f2 in kvm_cpu_exec
> >   > #33 0x558660227148 in qemu_kvm_cpu_thread_fn
> >   > #34 0x55866078bde7 in qemu_thread_start
> >   > #35 0x7f5784906594 in start_thread
> >   > #36 0x7f5784639e6f in clone
> > 
> > Avoid it with the aio_disable_external/aio_enable_external pair, so that
> > no vq poll handlers can be called in aio_wait_bh_oneshot.
> 
> I don't understand.  We are in the vCPU thread, so not in the
> AioContext's home thread.  Why is aio_wait_bh_oneshot polling rather
> than going through the aio_wait_bh path?

What do you mean by 'aio_wait_bh path'? Here is aio_wait_bh_oneshot:

void aio_wait_bh_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque)
{
AioWaitBHData data = {
.cb = cb,
.opaque = opaque,
};

assert(qemu_get_current_aio_context() == qemu_get_aio_context());

aio_bh_schedule_oneshot(ctx, aio_wait_bh, &data);
AIO_WAIT_WHILE(&data.wait, ctx, !data.done);
}

ctx is qemu_aio_context here, so there's no interaction with IOThread. This is
the full backtrace:

https://paste.ubuntu.com/p/Dm7zGzmmRG/

Fam

Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler

[Paolo Bonzini]

On 11/09/2018 16:12, Fam Zheng wrote:
> On Tue, 09/11 13:32, Paolo Bonzini wrote:
>> On 10/09/2018 16:56, Fam Zheng wrote:
>>> We have this unwanted call stack:
>>>
>>>   > ...
>>>   > #13 0x5586602b7793 in virtio_scsi_handle_cmd_vq
>>>   > #14 0x5586602b8d66 in virtio_scsi_data_plane_handle_cmd
>>>   > #15 0x5586602ddab7 in virtio_queue_notify_aio_vq
>>>   > #16 0x5586602dfc9f in virtio_queue_host_notifier_aio_poll
>>>   > #17 0x5586607885da in run_poll_handlers_once
>>>   > #18 0x55866078880e in try_poll_mode
>>>   > #19 0x5586607888eb in aio_poll
>>>   > #20 0x558660784561 in aio_wait_bh_oneshot
>>>   > #21 0x5586602b9582 in virtio_scsi_dataplane_stop
>>>   > #22 0x5586605a7110 in virtio_bus_stop_ioeventfd
>>>   > #23 0x5586605a9426 in virtio_pci_stop_ioeventfd
>>>   > #24 0x5586605ab808 in virtio_pci_common_write
>>>   > #25 0x558660242396 in memory_region_write_accessor
>>>   > #26 0x5586602425ab in access_with_adjusted_size
>>>   > #27 0x558660245281 in memory_region_dispatch_write
>>>   > #28 0x5586601e008e in flatview_write_continue
>>>   > #29 0x5586601e01d8 in flatview_write
>>>   > #30 0x5586601e04de in address_space_write
>>>   > #31 0x5586601e052f in address_space_rw
>>>   > #32 0x5586602607f2 in kvm_cpu_exec
>>>   > #33 0x558660227148 in qemu_kvm_cpu_thread_fn
>>>   > #34 0x55866078bde7 in qemu_thread_start
>>>   > #35 0x7f5784906594 in start_thread
>>>   > #36 0x7f5784639e6f in clone
>>>
>>> Avoid it with the aio_disable_external/aio_enable_external pair, so that
>>> no vq poll handlers can be called in aio_wait_bh_oneshot.
>>
>> I don't understand.  We are in the vCPU thread, so not in the
>> AioContext's home thread.  Why is aio_wait_bh_oneshot polling rather
>> than going through the aio_wait_bh path?
> 
> What do you mean by 'aio_wait_bh path'? Here is aio_wait_bh_oneshot:

Sorry, I meant the "atomic_inc(&wait_->num_waiters);" path.  But if this
backtrace is obtained without dataplane, that's the answer I was seeking.

> void aio_wait_bh_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque)
> {
> AioWaitBHData data = {
> .cb = cb,
> .opaque = opaque,
> };
> 
> assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> 
> aio_bh_schedule_oneshot(ctx, aio_wait_bh, &data);
> AIO_WAIT_WHILE(&data.wait, ctx, !data.done);
> }
> 
> ctx is qemu_aio_context here, so there's no interaction with IOThread.

In this case, it should be okay to have the reentrancy, what is the bug
that this patch is fixing?

Thanks,

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] block/rbd: pull out qemu_rbd_convert_options

[Eric Blake]

On 9/11/18 12:15 AM, Jeff Cody wrote:

Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
into a helper function.

Signed-off-by: Jeff Cody 
---
  block/rbd.c | 36 
  1 file changed, 24 insertions(+), 12 deletions(-)




-
+r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
  if (local_err) {
  error_propagate(errp, local_err);
-r = -EINVAL;
  goto out;
  }


You could simplify this to:

r = qemu_rbd_convert_options(bs, options, &opts, errp);
if (r < 0) {
goto out;
}

Either way, this refactoring looks correct.

Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[Jeff Cody]

On Tue, Sep 11, 2018 at 01:15:49AM -0400, Jeff Cody wrote:
> When we converted rbd to get rid of the older key/value-centric
> encoding format, we broke compatibility with image files with backing
> file strings encoded in the old format.
> 
> This leaves a bit of an ugly conundrum, and a hacky solution.
> 
> If the initial attempt to parse the "proper" options fails, it assumes
> that we may have an older key/value encoded filename.  Fall back to
> attempting to parse the filename, and extract the required options from
> it.  If that fails, pass along the original error message.
> 
> This approach has a potential drawback: if for some reason there are
> some options supplied the new way, and some the old way, we may not
> catch all the old options if they are not required options (since it
> won't cause the initial failure).
> 
> Signed-off-by: Jeff Cody 
> ---
>  block/rbd.c | 30 +++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index a8e79d01d2..bce86b8bde 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  BlockdevOptionsRbd *opts = NULL;
>  const QDictEntry *e;
>  Error *local_err = NULL;
> -char *keypairs, *secretid;
> +char *keypairs, *secretid, *filename;
>  int r;
>  
>  keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> @@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  
>  r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
>  if (local_err) {
> -error_propagate(errp, local_err);
> -goto out;
> +/* If the initial attempt to convert and process the options failed,
> + * we may be attempting to open an image file that has the rbd 
> options
> + * specified in the older format consisting of all key/value pairs
> + * encoded in the filename.  Go ahead and attempt to parse the
> + * filename, and see if we can pull out the required options */
> +Error *parse_err = NULL;
> +
> +filename = g_strdup(qdict_get_try_str(options, "filename"));

I'm leaking filename, I'll clean that up (along with any other comments) in
v2.

> +qdict_del(options, "filename");
> +
> +qemu_rbd_parse_filename(filename, options, NULL);
> +
> +g_free(keypairs);
> +keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> +if (keypairs) {
> +qdict_del(options, "=keyvalue-pairs");
> +}
> +
> +r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
> +if (parse_err) {
> +/* if the second attempt failed, pass along the original error
> + * message for the current format */
> +error_propagate(errp, local_err);
> +error_free(parse_err);
> +goto out;
> +}
>  }
>  
>  /* Remove the processed options from the QDict (the visitor processes
> -- 
> 2.17.1
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[Eric Blake]

On 9/11/18 12:15 AM, Jeff Cody wrote:

When we converted rbd to get rid of the older key/value-centric
encoding format, we broke compatibility with image files with backing
file strings encoded in the old format.

This leaves a bit of an ugly conundrum, and a hacky solution.

If the initial attempt to parse the "proper" options fails, it assumes
that we may have an older key/value encoded filename.  Fall back to
attempting to parse the filename, and extract the required options from
it.  If that fails, pass along the original error message.

This approach has a potential drawback: if for some reason there are
some options supplied the new way, and some the old way, we may not
catch all the old options if they are not required options (since it
won't cause the initial failure).


No one should be mixing new and old, though.



Signed-off-by: Jeff Cody 
---
  block/rbd.c | 30 +++---
  1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index a8e79d01d2..bce86b8bde 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
  BlockdevOptionsRbd *opts = NULL;
  const QDictEntry *e;
  Error *local_err = NULL;
-char *keypairs, *secretid;
+char *keypairs, *secretid, *filename;
  int r;
  
  keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));

@@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
  
  r = qemu_rbd_convert_options(bs, options, &opts, &local_err);

  if (local_err) {
-error_propagate(errp, local_err);
-goto out;


Oh, my comment about simplifying this in 1/2 is probably moot, now that 
you are doing a lot more based on local_err rather than just blindly 
propagating it.



+/* If the initial attempt to convert and process the options failed,
+ * we may be attempting to open an image file that has the rbd options
+ * specified in the older format consisting of all key/value pairs
+ * encoded in the filename.  Go ahead and attempt to parse the
+ * filename, and see if we can pull out the required options */
+Error *parse_err = NULL;
+
+filename = g_strdup(qdict_get_try_str(options, "filename"));


You already spotted your leak.


+qdict_del(options, "filename");
+
+qemu_rbd_parse_filename(filename, options, NULL);
+
+g_free(keypairs);


Wait. Why are you unilaterally freeing any previously-parsed keypairs in 
favor of the ones parsed out of the filename?  I'd rather that we insist 
on only old behavior, or only new, and not some mix.  Thus, if we 
already detected keypairs at all, we should declare this situation as an 
error, rather than throwing them away.



+keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
+if (keypairs) {
+qdict_del(options, "=keyvalue-pairs");
+}
+
+r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
+if (parse_err) {
+/* if the second attempt failed, pass along the original error
+ * message for the current format */
+error_propagate(errp, local_err);
+error_free(parse_err);
+goto out;
+}
  }


The idea of trying two parses makes sense, but I'm hoping v2 better 
handles the case of detecting bad attempts to mix-and-match behavior. 
Furthermore, is there an iotests that you can modify (or add) as a 
regression test for this working the way we want?


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] block/rbd: pull out qemu_rbd_convert_options

[John Snow]

On 09/11/2018 01:15 AM, Jeff Cody wrote:
> Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
> into a helper function.
> 
> Signed-off-by: Jeff Cody 
> ---
>  block/rbd.c | 36 
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index ca8e5bbace..a8e79d01d2 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -655,12 +655,34 @@ failed_opts:
>  return r;
>  }
>  
> +static int qemu_rbd_convert_options(BlockDriverState *bs, QDict *options,
> +BlockdevOptionsRbd **opts, Error **errp)
> +{
> +Visitor *v;
> +Error *local_err = NULL;
> +
> +/* Convert the remaining options into a QAPI object */
> +v = qobject_input_visitor_new_flat_confused(options, errp);
> +if (!v) {
> +return -EINVAL;
> +}
> +
> +visit_type_BlockdevOptionsRbd(v, NULL, opts, &local_err);
> +visit_free(v);
> +
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return -EINVAL;
> +}
> +
> +return 0;
> +}
> +
>  static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>   Error **errp)
>  {
>  BDRVRBDState *s = bs->opaque;
>  BlockdevOptionsRbd *opts = NULL;
> -Visitor *v;
>  const QDictEntry *e;
>  Error *local_err = NULL;
>  char *keypairs, *secretid;
> @@ -676,19 +698,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  qdict_del(options, "password-secret");
>  }
>  
> -/* Convert the remaining options into a QAPI object */
> -v = qobject_input_visitor_new_flat_confused(options, errp);
> -if (!v) {
> -r = -EINVAL;
> -goto out;
> -}
> -
> -visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
> -visit_free(v);
> -
> +r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
>  if (local_err) {
>  error_propagate(errp, local_err);
> -r = -EINVAL;
>  goto out;
>  }
>  
> 

Reviewed-by: John Snow 

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[John Snow]

On 09/11/2018 01:15 AM, Jeff Cody wrote:
> When we converted rbd to get rid of the older key/value-centric
> encoding format, we broke compatibility with image files with backing
> file strings encoded in the old format.
> 
> This leaves a bit of an ugly conundrum, and a hacky solution.
> 
> If the initial attempt to parse the "proper" options fails, it assumes
> that we may have an older key/value encoded filename.  Fall back to
> attempting to parse the filename, and extract the required options from
> it.  If that fails, pass along the original error message.
> 
> This approach has a potential drawback: if for some reason there are
> some options supplied the new way, and some the old way, we may not
> catch all the old options if they are not required options (since it
> won't cause the initial failure).
> 
> Signed-off-by: Jeff Cody 
> ---
>  block/rbd.c | 30 +++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index a8e79d01d2..bce86b8bde 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  BlockdevOptionsRbd *opts = NULL;
>  const QDictEntry *e;
>  Error *local_err = NULL;
> -char *keypairs, *secretid;
> +char *keypairs, *secretid, *filename;
>  int r;
>  
>  keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> @@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  
>  r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
>  if (local_err) {
> -error_propagate(errp, local_err);
> -goto out;
> +/* If the initial attempt to convert and process the options failed,
> + * we may be attempting to open an image file that has the rbd 
> options
> + * specified in the older format consisting of all key/value pairs
> + * encoded in the filename.  Go ahead and attempt to parse the
> + * filename, and see if we can pull out the required options */

Is it worth splitting out the legacy parsing routine here into its own
function, given that we will generally depend on it not being invoked?
i.e., for readability, it doesn't need to distract us.

> +Error *parse_err = NULL;
> +
> +filename = g_strdup(qdict_get_try_str(options, "filename"));
> +qdict_del(options, "filename");
> +
> +qemu_rbd_parse_filename(filename, options, NULL);
> +
> +g_free(keypairs);

As Eric already noticed, better to just return with error if this is
already set.

> +keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> +if (keypairs) {
> +qdict_del(options, "=keyvalue-pairs");
> +}
> +
> +r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
> +if (parse_err) {
> +/* if the second attempt failed, pass along the original error
> + * message for the current format */
> +error_propagate(errp, local_err);
> +error_free(parse_err);
> +goto out;
> +}

If it does succeed, though, ought we emit a deprecated warning that the
old specification syntax is not supported?

Once we load the image, will the header get rewritten into a compliant
format?

--js

>  }
>  
>  /* Remove the processed options from the QDict (the visitor processes
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[Jeff Cody]

On Tue, Sep 11, 2018 at 01:03:44PM -0500, Eric Blake wrote:
> On 9/11/18 12:15 AM, Jeff Cody wrote:
> >When we converted rbd to get rid of the older key/value-centric
> >encoding format, we broke compatibility with image files with backing
> >file strings encoded in the old format.
> >
> >This leaves a bit of an ugly conundrum, and a hacky solution.
> >
> >If the initial attempt to parse the "proper" options fails, it assumes
> >that we may have an older key/value encoded filename.  Fall back to
> >attempting to parse the filename, and extract the required options from
> >it.  If that fails, pass along the original error message.
> >
> >This approach has a potential drawback: if for some reason there are
> >some options supplied the new way, and some the old way, we may not
> >catch all the old options if they are not required options (since it
> >won't cause the initial failure).
> 
> No one should be mixing new and old, though.
> 
> >
> >Signed-off-by: Jeff Cody 
> >---
> >  block/rbd.c | 30 +++---
> >  1 file changed, 27 insertions(+), 3 deletions(-)
> >
> >diff --git a/block/rbd.c b/block/rbd.c
> >index a8e79d01d2..bce86b8bde 100644
> >--- a/block/rbd.c
> >+++ b/block/rbd.c
> >@@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> >*options, int flags,
> >  BlockdevOptionsRbd *opts = NULL;
> >  const QDictEntry *e;
> >  Error *local_err = NULL;
> >-char *keypairs, *secretid;
> >+char *keypairs, *secretid, *filename;
> >  int r;
> >  keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> >@@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> >*options, int flags,
> >  r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
> >  if (local_err) {
> >-error_propagate(errp, local_err);
> >-goto out;
> 
> Oh, my comment about simplifying this in 1/2 is probably moot, now that you
> are doing a lot more based on local_err rather than just blindly propagating
> it.
> 
> >+/* If the initial attempt to convert and process the options failed,
> >+ * we may be attempting to open an image file that has the rbd 
> >options
> >+ * specified in the older format consisting of all key/value pairs
> >+ * encoded in the filename.  Go ahead and attempt to parse the
> >+ * filename, and see if we can pull out the required options */
> >+Error *parse_err = NULL;
> >+
> >+filename = g_strdup(qdict_get_try_str(options, "filename"));
> 
> You already spotted your leak.
> 
> >+qdict_del(options, "filename");
> >+
> >+qemu_rbd_parse_filename(filename, options, NULL);
> >+
> >+g_free(keypairs);
> 
> Wait. Why are you unilaterally freeing any previously-parsed keypairs in
> favor of the ones parsed out of the filename?  I'd rather that we insist on
> only old behavior, or only new, and not some mix.  Thus, if we already
> detected keypairs at all, we should declare this situation as an error,
> rather than throwing them away.
> 

Good point.  I'll flag (local_err && keypairs) as an error.

I also just realized I need to check for NULL filename, and error out as
well in that case.

> >+keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> >+if (keypairs) {
> >+qdict_del(options, "=keyvalue-pairs");
> >+}
> >+
> >+r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
> >+if (parse_err) {
> >+/* if the second attempt failed, pass along the original error
> >+ * message for the current format */
> >+error_propagate(errp, local_err);
> >+error_free(parse_err);
> >+goto out;
> >+}
> >  }
> 
> The idea of trying two parses makes sense, but I'm hoping v2 better handles
> the case of detecting bad attempts to mix-and-match behavior. Furthermore,
> is there an iotests that you can modify (or add) as a regression test for
> this working the way we want?

Hmm... yes, that should be doable.  I'm not sure if I can do it without the
'success' path being a timeout when there is no ceph server present, though.
I'll see what I can do.

-Jeff

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[Jeff Cody]

On Tue, Sep 11, 2018 at 02:22:31PM -0400, John Snow wrote:
> 
> 
> On 09/11/2018 01:15 AM, Jeff Cody wrote:
> > When we converted rbd to get rid of the older key/value-centric
> > encoding format, we broke compatibility with image files with backing
> > file strings encoded in the old format.
> > 
> > This leaves a bit of an ugly conundrum, and a hacky solution.
> > 
> > If the initial attempt to parse the "proper" options fails, it assumes
> > that we may have an older key/value encoded filename.  Fall back to
> > attempting to parse the filename, and extract the required options from
> > it.  If that fails, pass along the original error message.
> > 
> > This approach has a potential drawback: if for some reason there are
> > some options supplied the new way, and some the old way, we may not
> > catch all the old options if they are not required options (since it
> > won't cause the initial failure).
> > 
> > Signed-off-by: Jeff Cody 
> > ---
> >  block/rbd.c | 30 +++---
> >  1 file changed, 27 insertions(+), 3 deletions(-)
> > 
> > diff --git a/block/rbd.c b/block/rbd.c
> > index a8e79d01d2..bce86b8bde 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >  BlockdevOptionsRbd *opts = NULL;
> >  const QDictEntry *e;
> >  Error *local_err = NULL;
> > -char *keypairs, *secretid;
> > +char *keypairs, *secretid, *filename;
> >  int r;
> >  
> >  keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> > @@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >  
> >  r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
> >  if (local_err) {
> > -error_propagate(errp, local_err);
> > -goto out;
> > +/* If the initial attempt to convert and process the options 
> > failed,
> > + * we may be attempting to open an image file that has the rbd 
> > options
> > + * specified in the older format consisting of all key/value pairs
> > + * encoded in the filename.  Go ahead and attempt to parse the
> > + * filename, and see if we can pull out the required options */
> 
> Is it worth splitting out the legacy parsing routine here into its own
> function, given that we will generally depend on it not being invoked?
> i.e., for readability, it doesn't need to distract us.
> 

Yeah, that would probably be good.

> > +Error *parse_err = NULL;
> > +
> > +filename = g_strdup(qdict_get_try_str(options, "filename"));
> > +qdict_del(options, "filename");
> > +
> > +qemu_rbd_parse_filename(filename, options, NULL);
> > +
> > +g_free(keypairs);
> 
> As Eric already noticed, better to just return with error if this is
> already set.
> 
> > +keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> > +if (keypairs) {
> > +qdict_del(options, "=keyvalue-pairs");
> > +}
> > +
> > +r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
> > +if (parse_err) {
> > +/* if the second attempt failed, pass along the original error
> > + * message for the current format */
> > +error_propagate(errp, local_err);
> > +error_free(parse_err);
> > +goto out;
> > +}
> 
> If it does succeed, though, ought we emit a deprecated warning that the
> old specification syntax is not supported?
>

I don't know.  Without this support, we can't open some existing images.  At
what point would we actually remove that support?

> Once we load the image, will the header get rewritten into a compliant
> format?
> 

Hmm - I think in some code paths, but not all.  I don't think the answer is
'yes' universally, alas.

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames

[John Snow]

On 09/11/2018 02:37 PM, Jeff Cody wrote:
> On Tue, Sep 11, 2018 at 02:22:31PM -0400, John Snow wrote:
>>
>>
>> On 09/11/2018 01:15 AM, Jeff Cody wrote:
>>> When we converted rbd to get rid of the older key/value-centric
>>> encoding format, we broke compatibility with image files with backing
>>> file strings encoded in the old format.
>>>
>>> This leaves a bit of an ugly conundrum, and a hacky solution.
>>>
>>> If the initial attempt to parse the "proper" options fails, it assumes
>>> that we may have an older key/value encoded filename.  Fall back to
>>> attempting to parse the filename, and extract the required options from
>>> it.  If that fails, pass along the original error message.
>>>
>>> This approach has a potential drawback: if for some reason there are
>>> some options supplied the new way, and some the old way, we may not
>>> catch all the old options if they are not required options (since it
>>> won't cause the initial failure).
>>>
>>> Signed-off-by: Jeff Cody 
>>> ---
>>>  block/rbd.c | 30 +++---
>>>  1 file changed, 27 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block/rbd.c b/block/rbd.c
>>> index a8e79d01d2..bce86b8bde 100644
>>> --- a/block/rbd.c
>>> +++ b/block/rbd.c
>>> @@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
>>> *options, int flags,
>>>  BlockdevOptionsRbd *opts = NULL;
>>>  const QDictEntry *e;
>>>  Error *local_err = NULL;
>>> -char *keypairs, *secretid;
>>> +char *keypairs, *secretid, *filename;
>>>  int r;
>>>  
>>>  keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
>>> @@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
>>> *options, int flags,
>>>  
>>>  r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
>>>  if (local_err) {
>>> -error_propagate(errp, local_err);
>>> -goto out;
>>> +/* If the initial attempt to convert and process the options 
>>> failed,
>>> + * we may be attempting to open an image file that has the rbd 
>>> options
>>> + * specified in the older format consisting of all key/value pairs
>>> + * encoded in the filename.  Go ahead and attempt to parse the
>>> + * filename, and see if we can pull out the required options */
>>
>> Is it worth splitting out the legacy parsing routine here into its own
>> function, given that we will generally depend on it not being invoked?
>> i.e., for readability, it doesn't need to distract us.
>>
> 
> Yeah, that would probably be good.
> 
>>> +Error *parse_err = NULL;
>>> +
>>> +filename = g_strdup(qdict_get_try_str(options, "filename"));
>>> +qdict_del(options, "filename");
>>> +
>>> +qemu_rbd_parse_filename(filename, options, NULL);
>>> +
>>> +g_free(keypairs);
>>
>> As Eric already noticed, better to just return with error if this is
>> already set.
>>
>>> +keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
>>> +if (keypairs) {
>>> +qdict_del(options, "=keyvalue-pairs");
>>> +}
>>> +
>>> +r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
>>> +if (parse_err) {
>>> +/* if the second attempt failed, pass along the original error
>>> + * message for the current format */
>>> +error_propagate(errp, local_err);
>>> +error_free(parse_err);
>>> +goto out;
>>> +}
>>
>> If it does succeed, though, ought we emit a deprecated warning that the
>> old specification syntax is not supported?
>>
> 
> I don't know.  Without this support, we can't open some existing images.  At
> what point would we actually remove that support?
> 

Yeah, probably never -- but there are some versions of QEMU in the wild
that now simply don't support this format, so some urging to switch is
probably not harmful was my thought.

(At least, that was my read. When did we switch RBD formats? Just for
3.1? In that case, no warning is necessary.)

>> Once we load the image, will the header get rewritten into a compliant
>> format?
>>
> 
> Hmm - I think in some code paths, but not all.  I don't think the answer is
> 'yes' universally, alas.
> 

Re: [Qemu-block] [Qemu-devel] [PATCH v2 4/4] qapi: add transaction support for x-block-dirty-bitmap-merge

[John Snow]

On 07/06/2018 07:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> New action is like clean action: do the whole thing in .prepare and
> undo in .abort. This behavior for bitmap-changing actions is needed
> because backup job actions use bitmap in .prepare.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  qapi/transaction.json |  2 ++
>  blockdev.c| 38 +-
>  2 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/transaction.json b/qapi/transaction.json
> index d7e4274550..5875cdb16c 100644
> --- a/qapi/transaction.json
> +++ b/qapi/transaction.json
> @@ -48,6 +48,7 @@
>  # - @block-dirty-bitmap-clear: since 2.5
>  # - @x-block-dirty-bitmap-enable: since 3.0
>  # - @x-block-dirty-bitmap-disable: since 3.0
> +# - @x-block-dirty-bitmap-merge: since 3.1
>  # - @blockdev-backup: since 2.3
>  # - @blockdev-snapshot: since 2.5
>  # - @blockdev-snapshot-internal-sync: since 1.7
> @@ -63,6 +64,7 @@
> 'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
> 'x-block-dirty-bitmap-enable': 'BlockDirtyBitmap',
> 'x-block-dirty-bitmap-disable': 'BlockDirtyBitmap',
> +   'x-block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
> 'blockdev-backup': 'BlockdevBackup',
> 'blockdev-snapshot': 'BlockdevSnapshot',
> 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
> diff --git a/blockdev.c b/blockdev.c
> index 5348e8ba9b..feebbb9a9a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1940,6 +1940,7 @@ static void blockdev_backup_clean(BlkActionState 
> *common)
>  typedef struct BlockDirtyBitmapState {
>  BlkActionState common;
>  BdrvDirtyBitmap *bitmap;
> +BdrvDirtyBitmap *merge_source;

Is this necessary?

>  BlockDriverState *bs;
>  HBitmap *backup;
>  bool prepared;
> @@ -2112,6 +2113,35 @@ static void 
> block_dirty_bitmap_disable_abort(BlkActionState *common)
>  }
>  }
>  
> +static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
> + Error **errp)
> +{
> +BlockDirtyBitmapMerge *action;
> +BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> +
> +if (action_check_completion_mode(common, errp) < 0) {
> +return;
> +}
> +
> +action = common->action->u.x_block_dirty_bitmap_merge.data;
> +state->bitmap = block_dirty_bitmap_lookup(action->node,
> +  action->dst_name,
> +  &state->bs,
> +  errp);
> +if (!state->bitmap) {
> +return;
> +}
> +
> +state->merge_source = bdrv_find_dirty_bitmap(state->bs, 
> action->src_name);
> +if (!state->merge_source) {
> +return;
> +}
> +
> +bdrv_merge_dirty_bitmap(state->bitmap, state->merge_source, 
> &state->backup,
> +errp);
> +}
> +
>  static void abort_prepare(BlkActionState *common, Error **errp)
>  {
>  error_setg(errp, "Transaction aborted using Abort action");
> @@ -2182,7 +2212,13 @@ static const BlkActionOps actions[] = {
>  .instance_size = sizeof(BlockDirtyBitmapState),
>  .prepare = block_dirty_bitmap_disable_prepare,
>  .abort = block_dirty_bitmap_disable_abort,
> - }
> +},
> +[TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_MERGE] = {
> +.instance_size = sizeof(BlockDirtyBitmapState),
> +.prepare = block_dirty_bitmap_merge_prepare,
> +.commit = block_dirty_bitmap_free_backup,
> +.abort = block_dirty_bitmap_restore,
> +}
>  };
>  
>  /**
> 

If the new state is not necessary and you remove it:

Reviewed-by: John Snow 

[Qemu-block] [PATCH v2 2/3] block/rbd: Attempt to parse legacy filenames

[Jeff Cody]

When we converted rbd to get rid of the older key/value-centric
encoding format, we broke compatibility with image files with backing
file strings encoded in the old format.

This leaves a bit of an ugly conundrum, and a hacky solution.

If the initial attempt to parse the "proper" options fails, it assumes
that we may have an older key/value encoded filename.  Fall back to
attempting to parse the filename, and extract the required options from
it.  If that fails, pass along the original error message.

We do not support mixed modern usage alongside legacy keyvalue pair
usage.

A deprecation warning has been added, although care should be taken
when actually deprecating since the impact is not limited to
commandline or qapi usage, but also opening existing images.

Signed-off-by: Jeff Cody 
---
 block/rbd.c | 53 +++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index b199450f9f..5090e4f662 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options, 
BlockdevOptionsRbd **opts,
 return 0;
 }
 
+static int qemu_rbd_attempt_legacy_options(QDict *options,
+   BlockdevOptionsRbd **opts,
+   char **keypairs)
+{
+char *filename;
+int r;
+
+filename = g_strdup(qdict_get_try_str(options, "filename"));
+if (!filename) {
+return -EINVAL;
+}
+qdict_del(options, "filename");
+
+qemu_rbd_parse_filename(filename, options, NULL);
+
+/* keypairs freed by caller */
+*keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
+if (*keypairs) {
+qdict_del(options, "=keyvalue-pairs");
+}
+
+r = qemu_rbd_convert_options(options, opts, NULL);
+
+g_free(filename);
+return r;
+}
+
 static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
  Error **errp)
 {
@@ -700,8 +727,30 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 r = qemu_rbd_convert_options(options, &opts, &local_err);
 if (local_err) {
-error_propagate(errp, local_err);
-goto out;
+/* If keypairs are present, that means some options are present in
+ * the modern option format.  Don't attempt to parse legacy option
+ * formats, as we won't support mixed usage. */
+if (keypairs) {
+error_propagate(errp, local_err);
+goto out;
+}
+
+/* If the initial attempt to convert and process the options failed,
+ * we may be attempting to open an image file that has the rbd options
+ * specified in the older format consisting of all key/value pairs
+ * encoded in the filename.  Go ahead and attempt to parse the
+ * filename, and see if we can pull out the required options. */
+r = qemu_rbd_attempt_legacy_options(options, &opts, &keypairs);
+if (r < 0) {
+error_propagate(errp, local_err);
+goto out;
+}
+/* Take care whenever deciding to actually deprecate; once this ability
+ * is removed, we will not be able to open any images with 
legacy-styled
+ * backing image strings. */
+error_report("RBD options encoded in the filename as keyvalue pairs "
+ "is deprecated.  Future versions may cease to parse "
+ "these options in the future.");
 }
 
 /* Remove the processed options from the QDict (the visitor processes
-- 
2.17.1

[Qemu-block] [PATCH v2 0/3] block/rbd: enable filename parsing on open

[Jeff Cody]

Changes from v1:

Patch 1: Don't pass unused BlockDriverState to helper function

Patch 2: Do not allow mixed usage; fail if keyvalue is present [Eric]
 Add deprecation warning [John]
 Pull legacy parsing code into function [John]
 Fixed filename leak

Patch 3: New; iotest 231. [Eric]


iotest failure on current master:

 QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated. 
 Future versions may cease to parse these options in the future.
-unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
-qemu-img: Could not open 
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': 
error connecting: No such file or directory
+qemu-img: Could not open 
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': 
Parameter 'pool' is missing
 unable to get monitor info from DNS SRV with service name: ceph-mon
 no monitors specified to connect to.
 qemu-img: Could not open 
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
 error connecting: No such file or directory
Failures: 231
Failed 1 of 1 tests



Jeff Cody (3):
  block/rbd: pull out qemu_rbd_convert_options
  block/rbd: Attempt to parse legacy filenames
  block/rbd: add iotest for rbd legacy keyvalue filename parsing

 block/rbd.c| 89 --
 tests/qemu-iotests/231 | 62 ++
 tests/qemu-iotests/231.out |  9 
 tests/qemu-iotests/group   |  1 +
 4 files changed, 147 insertions(+), 14 deletions(-)
 create mode 100755 tests/qemu-iotests/231
 create mode 100644 tests/qemu-iotests/231.out

-- 
2.17.1

[Qemu-block] [PATCH v2 1/3] block/rbd: pull out qemu_rbd_convert_options

[Jeff Cody]

Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
into a helper function.

Reviewed-by: Eric Blake 
Reviewed-by: John Snow 
Signed-off-by: Jeff Cody 
---
 block/rbd.c | 36 
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index ca8e5bbace..b199450f9f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -655,12 +655,34 @@ failed_opts:
 return r;
 }
 
+static int qemu_rbd_convert_options(QDict *options, BlockdevOptionsRbd **opts,
+Error **errp)
+{
+Visitor *v;
+Error *local_err = NULL;
+
+/* Convert the remaining options into a QAPI object */
+v = qobject_input_visitor_new_flat_confused(options, errp);
+if (!v) {
+return -EINVAL;
+}
+
+visit_type_BlockdevOptionsRbd(v, NULL, opts, &local_err);
+visit_free(v);
+
+if (local_err) {
+error_propagate(errp, local_err);
+return -EINVAL;
+}
+
+return 0;
+}
+
 static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
  Error **errp)
 {
 BDRVRBDState *s = bs->opaque;
 BlockdevOptionsRbd *opts = NULL;
-Visitor *v;
 const QDictEntry *e;
 Error *local_err = NULL;
 char *keypairs, *secretid;
@@ -676,19 +698,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
 qdict_del(options, "password-secret");
 }
 
-/* Convert the remaining options into a QAPI object */
-v = qobject_input_visitor_new_flat_confused(options, errp);
-if (!v) {
-r = -EINVAL;
-goto out;
-}
-
-visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
-visit_free(v);
-
+r = qemu_rbd_convert_options(options, &opts, &local_err);
 if (local_err) {
 error_propagate(errp, local_err);
-r = -EINVAL;
 goto out;
 }
 
-- 
2.17.1

[Qemu-block] [PATCH v2 3/3] block/rbd: add iotest for rbd legacy keyvalue filename parsing

[Jeff Cody]

This is a small test that will check for the ability to parse
both legacy and modern options for rbd.

The way the test is set up is for failure to occur, but without
having to wait to timeout on a non-existent rbd server.  The error
messages in the success path show that the arguments were parsed.

The failure behavior prior to the patch series that has this test, is
qemu-img complaining about mandatory options (e.g. 'pool') not being
provided.

Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/231 | 62 ++
 tests/qemu-iotests/231.out |  9 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 72 insertions(+)
 create mode 100755 tests/qemu-iotests/231
 create mode 100644 tests/qemu-iotests/231.out

diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
new file mode 100755
index 00..3e283708b4
--- /dev/null
+++ b/tests/qemu-iotests/231
@@ -0,0 +1,62 @@
+#!/bin/bash
+#
+# Test legacy and modern option parsing for rbd/ceph.  This will not
+# actually connect to a ceph server, but rather looks for the appropriate
+# error message that indicates we parsed the options correctly.
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=jc...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+rm "${BOGUS_CONF}"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto rbd
+_supported_os Linux
+
+BOGUS_CONF=${TEST_DIR}/ceph-$$.conf
+touch "${BOGUS_CONF}"
+
+_filter_conf()
+{
+sed -e "s#$BOGUS_CONF#BOGUS_CONF#g"
+}
+
+# We expect this to fail, with no monitor ip provided and a null conf file.  
Just want it
+# to fail in the right way.
+$QEMU_IMG info 
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}" 
2>&1 | _filter_conf
+$QEMU_IMG info 
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
 2>&1 | _filter_conf
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
new file mode 100644
index 00..579ba11c16
--- /dev/null
+++ b/tests/qemu-iotests/231.out
@@ -0,0 +1,9 @@
+QA output created by 231
+qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated. 
 Future versions may cease to parse these options in the future.
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open 
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': 
error connecting: No such file or directory
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open 
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
 error connecting: No such file or directory
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 743790745b..31f6e77dcb 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -226,3 +226,4 @@
 226 auto quick
 227 auto quick
 229 auto quick
+231 auto quick
-- 
2.17.1

Re: [Qemu-block] [PATCH v2 2/3] block/rbd: Attempt to parse legacy filenames

[Eric Blake]

On 9/11/18 3:43 PM, Jeff Cody wrote:

When we converted rbd to get rid of the older key/value-centric
encoding format, we broke compatibility with image files with backing
file strings encoded in the old format.

This leaves a bit of an ugly conundrum, and a hacky solution.

If the initial attempt to parse the "proper" options fails, it assumes
that we may have an older key/value encoded filename.  Fall back to
attempting to parse the filename, and extract the required options from
it.  If that fails, pass along the original error message.

We do not support mixed modern usage alongside legacy keyvalue pair
usage.

A deprecation warning has been added, although care should be taken
when actually deprecating since the impact is not limited to
commandline or qapi usage, but also opening existing images.

Signed-off-by: Jeff Cody 
---
  block/rbd.c | 53 +++--
  1 file changed, 51 insertions(+), 2 deletions(-)


Where's the patch to qemu/qemu-deprecated.texi to mention the new 
message and our advice on upgrading images to avoid triggering it?




diff --git a/block/rbd.c b/block/rbd.c
index b199450f9f..5090e4f662 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options, 
BlockdevOptionsRbd **opts,
  return 0;
  }
  
+static int qemu_rbd_attempt_legacy_options(QDict *options,

+   BlockdevOptionsRbd **opts,
+   char **keypairs)
+{
+char *filename;
+int r;
+
+filename = g_strdup(qdict_get_try_str(options, "filename"));
+if (!filename) {
+return -EINVAL;
+}
+qdict_del(options, "filename");
+
+qemu_rbd_parse_filename(filename, options, NULL);


Intentionally ignoring errors here,


+
+/* keypairs freed by caller */
+*keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
+if (*keypairs) {
+qdict_del(options, "=keyvalue-pairs");
+}
+
+r = qemu_rbd_convert_options(options, opts, NULL);


and here. I guess we'll see how the caller is expecting things to behave.


+
+g_free(filename);
+return r;
+}
+
  static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
   Error **errp)
  {
@@ -700,8 +727,30 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
  
  r = qemu_rbd_convert_options(options, &opts, &local_err);

  if (local_err) {
-error_propagate(errp, local_err);
-goto out;
+/* If keypairs are present, that means some options are present in
+ * the modern option format.  Don't attempt to parse legacy option
+ * formats, as we won't support mixed usage. */
+if (keypairs) {
+error_propagate(errp, local_err);
+goto out;
+}
+
+/* If the initial attempt to convert and process the options failed,
+ * we may be attempting to open an image file that has the rbd options
+ * specified in the older format consisting of all key/value pairs
+ * encoded in the filename.  Go ahead and attempt to parse the
+ * filename, and see if we can pull out the required options. */
+r = qemu_rbd_attempt_legacy_options(options, &opts, &keypairs);
+if (r < 0) {
+error_propagate(errp, local_err);
+goto out;
+}
+/* Take care whenever deciding to actually deprecate; once this ability
+ * is removed, we will not be able to open any images with 
legacy-styled
+ * backing image strings. */
+error_report("RBD options encoded in the filename as keyvalue pairs "
+ "is deprecated.  Future versions may cease to parse "
+ "these options in the future.");


Okay, so you're making a best-effort fallback to scrape out any 
remaining keyvalue pairs. If there was no filename (and hence no 
keyvalue pairs populated), we know up front that we lack enough 
information, so attempt_legacy_options() returns failure right away; if 
there was a filename, we don't know if we got all the required options 
(so ignoring errors was okay) - that will be up to later code to 
decipher, after we emit our warning that we already relied on legacy 
options (if the later code has all it needed, then only the warning is 
emitted; if later code fails, the user now has the warning in addition 
to the later failure message to help them resolve their images).


A bit confusing how it all fits together, but it seems to work out in 
the end.


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH v2 2/3] block/rbd: Attempt to parse legacy filenames

[Jeff Cody]

On Tue, Sep 11, 2018 at 04:05:45PM -0500, Eric Blake wrote:
> On 9/11/18 3:43 PM, Jeff Cody wrote:
> >When we converted rbd to get rid of the older key/value-centric
> >encoding format, we broke compatibility with image files with backing
> >file strings encoded in the old format.
> >
> >This leaves a bit of an ugly conundrum, and a hacky solution.
> >
> >If the initial attempt to parse the "proper" options fails, it assumes
> >that we may have an older key/value encoded filename.  Fall back to
> >attempting to parse the filename, and extract the required options from
> >it.  If that fails, pass along the original error message.
> >
> >We do not support mixed modern usage alongside legacy keyvalue pair
> >usage.
> >
> >A deprecation warning has been added, although care should be taken
> >when actually deprecating since the impact is not limited to
> >commandline or qapi usage, but also opening existing images.
> >
> >Signed-off-by: Jeff Cody 
> >---
> >  block/rbd.c | 53 +++--
> >  1 file changed, 51 insertions(+), 2 deletions(-)
> 
> Where's the patch to qemu/qemu-deprecated.texi to mention the new message
> and our advice on upgrading images to avoid triggering it?
> 

Argh, thanks - forgot that.  I'll spin a v3 with that as patch 4.

> >
> >diff --git a/block/rbd.c b/block/rbd.c
> >index b199450f9f..5090e4f662 100644
> >--- a/block/rbd.c
> >+++ b/block/rbd.c
> >@@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options, 
> >BlockdevOptionsRbd **opts,
> >  return 0;
> >  }
> >+static int qemu_rbd_attempt_legacy_options(QDict *options,
> >+   BlockdevOptionsRbd **opts,
> >+   char **keypairs)
> >+{
> >+char *filename;
> >+int r;
> >+
> >+filename = g_strdup(qdict_get_try_str(options, "filename"));
> >+if (!filename) {
> >+return -EINVAL;
> >+}
> >+qdict_del(options, "filename");
> >+
> >+qemu_rbd_parse_filename(filename, options, NULL);
> 
> Intentionally ignoring errors here,
> 
> >+
> >+/* keypairs freed by caller */
> >+*keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> >+if (*keypairs) {
> >+qdict_del(options, "=keyvalue-pairs");
> >+}
> >+
> >+r = qemu_rbd_convert_options(options, opts, NULL);
> 
> and here. I guess we'll see how the caller is expecting things to behave.
> 
> >+
> >+g_free(filename);
> >+return r;
> >+}
> >+
> >  static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> >   Error **errp)
> >  {
> >@@ -700,8 +727,30 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> >*options, int flags,
> >  r = qemu_rbd_convert_options(options, &opts, &local_err);
> >  if (local_err) {
> >-error_propagate(errp, local_err);
> >-goto out;
> >+/* If keypairs are present, that means some options are present in
> >+ * the modern option format.  Don't attempt to parse legacy option
> >+ * formats, as we won't support mixed usage. */
> >+if (keypairs) {
> >+error_propagate(errp, local_err);
> >+goto out;
> >+}
> >+
> >+/* If the initial attempt to convert and process the options failed,
> >+ * we may be attempting to open an image file that has the rbd 
> >options
> >+ * specified in the older format consisting of all key/value pairs
> >+ * encoded in the filename.  Go ahead and attempt to parse the
> >+ * filename, and see if we can pull out the required options. */
> >+r = qemu_rbd_attempt_legacy_options(options, &opts, &keypairs);
> >+if (r < 0) {
> >+error_propagate(errp, local_err);
> >+goto out;
> >+}
> >+/* Take care whenever deciding to actually deprecate; once this 
> >ability
> >+ * is removed, we will not be able to open any images with 
> >legacy-styled
> >+ * backing image strings. */
> >+error_report("RBD options encoded in the filename as keyvalue pairs 
> >"
> >+ "is deprecated.  Future versions may cease to parse "
> >+ "these options in the future.");
> 
> Okay, so you're making a best-effort fallback to scrape out any remaining
> keyvalue pairs. If there was no filename (and hence no keyvalue pairs
> populated), we know up front that we lack enough information, so
> attempt_legacy_options() returns failure right away; if there was a
> filename, we don't know if we got all the required options (so ignoring
> errors was okay) - that will be up to later code to decipher, after we emit
> our warning that we already relied on legacy options (if the later code has
> all it needed, then only the warning is emitted; if later code fails, the
> user now has the warning in addition to the later failure message to help
> them resolve their images).
> 
> A bit confusing how 

Re: [Qemu-block] [PATCH v2 3/3] block/rbd: add iotest for rbd legacy keyvalue filename parsing

[Eric Blake]

On 9/11/18 3:43 PM, Jeff Cody wrote:

This is a small test that will check for the ability to parse
both legacy and modern options for rbd.

The way the test is set up is for failure to occur, but without
having to wait to timeout on a non-existent rbd server.  The error
messages in the success path show that the arguments were parsed.

The failure behavior prior to the patch series that has this test, is
qemu-img complaining about mandatory options (e.g. 'pool') not being
provided.

Signed-off-by: Jeff Cody 
---
  tests/qemu-iotests/231 | 62 ++
  tests/qemu-iotests/231.out |  9 ++
  tests/qemu-iotests/group   |  1 +
  3 files changed, 72 insertions(+)
  create mode 100755 tests/qemu-iotests/231
  create mode 100644 tests/qemu-iotests/231.out


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-block] [PATCH v3 1/4] block/rbd: pull out qemu_rbd_convert_options

[Jeff Cody]

Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
into a helper function.

Reviewed-by: Eric Blake 
Reviewed-by: John Snow 
Signed-off-by: Jeff Cody 
---
 block/rbd.c | 36 
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index ca8e5bbace..b199450f9f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -655,12 +655,34 @@ failed_opts:
 return r;
 }
 
+static int qemu_rbd_convert_options(QDict *options, BlockdevOptionsRbd **opts,
+Error **errp)
+{
+Visitor *v;
+Error *local_err = NULL;
+
+/* Convert the remaining options into a QAPI object */
+v = qobject_input_visitor_new_flat_confused(options, errp);
+if (!v) {
+return -EINVAL;
+}
+
+visit_type_BlockdevOptionsRbd(v, NULL, opts, &local_err);
+visit_free(v);
+
+if (local_err) {
+error_propagate(errp, local_err);
+return -EINVAL;
+}
+
+return 0;
+}
+
 static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
  Error **errp)
 {
 BDRVRBDState *s = bs->opaque;
 BlockdevOptionsRbd *opts = NULL;
-Visitor *v;
 const QDictEntry *e;
 Error *local_err = NULL;
 char *keypairs, *secretid;
@@ -676,19 +698,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
 qdict_del(options, "password-secret");
 }
 
-/* Convert the remaining options into a QAPI object */
-v = qobject_input_visitor_new_flat_confused(options, errp);
-if (!v) {
-r = -EINVAL;
-goto out;
-}
-
-visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
-visit_free(v);
-
+r = qemu_rbd_convert_options(options, &opts, &local_err);
 if (local_err) {
 error_propagate(errp, local_err);
-r = -EINVAL;
 goto out;
 }
 
-- 
2.17.1

[Qemu-block] [PATCH v3 4/4] block/rbd: add deprecation documenation for filename keyvalue pairs

[Jeff Cody]

Signed-off-by: Jeff Cody 
---
 qemu-deprecated.texi | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 1b9c007f12..4df8ac442d 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -190,6 +190,13 @@ used instead.
 In order to prevent QEMU from automatically opening an image's backing
 chain, use ``"backing": null'' instead.
 
+@subsubsection "rbd keyvalue pair encoded filenames": "" (since 3.1.0)
+
+Options for ``rbd'' should be specified according to its runtime options,
+like other block drivers.  Legacy parsing of keyvalue pair encoded
+filenames is useful to open images with the old format for backing files;
+These image files should be updated to use the current format.
+
 @subsection vio-spapr-device device options
 
 @subsubsection "irq": "" (since 3.0.0)
-- 
2.17.1

[Qemu-block] [PATCH v3 2/4] block/rbd: Attempt to parse legacy filenames

[Jeff Cody]

When we converted rbd to get rid of the older key/value-centric
encoding format, we broke compatibility with image files with backing
file strings encoded in the old format.

This leaves a bit of an ugly conundrum, and a hacky solution.

If the initial attempt to parse the "proper" options fails, it assumes
that we may have an older key/value encoded filename.  Fall back to
attempting to parse the filename, and extract the required options from
it.  If that fails, pass along the original error message.

We do not support mixed modern usage alongside legacy keyvalue pair
usage.

A deprecation warning has been added, although care should be taken
when actually deprecating since the impact is not limited to
commandline or qapi usage, but also opening existing images.

Reviewed-by: Eric Blake 
Signed-off-by: Jeff Cody 
---
 block/rbd.c | 53 +++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index b199450f9f..5090e4f662 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options, 
BlockdevOptionsRbd **opts,
 return 0;
 }
 
+static int qemu_rbd_attempt_legacy_options(QDict *options,
+   BlockdevOptionsRbd **opts,
+   char **keypairs)
+{
+char *filename;
+int r;
+
+filename = g_strdup(qdict_get_try_str(options, "filename"));
+if (!filename) {
+return -EINVAL;
+}
+qdict_del(options, "filename");
+
+qemu_rbd_parse_filename(filename, options, NULL);
+
+/* keypairs freed by caller */
+*keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
+if (*keypairs) {
+qdict_del(options, "=keyvalue-pairs");
+}
+
+r = qemu_rbd_convert_options(options, opts, NULL);
+
+g_free(filename);
+return r;
+}
+
 static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
  Error **errp)
 {
@@ -700,8 +727,30 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 r = qemu_rbd_convert_options(options, &opts, &local_err);
 if (local_err) {
-error_propagate(errp, local_err);
-goto out;
+/* If keypairs are present, that means some options are present in
+ * the modern option format.  Don't attempt to parse legacy option
+ * formats, as we won't support mixed usage. */
+if (keypairs) {
+error_propagate(errp, local_err);
+goto out;
+}
+
+/* If the initial attempt to convert and process the options failed,
+ * we may be attempting to open an image file that has the rbd options
+ * specified in the older format consisting of all key/value pairs
+ * encoded in the filename.  Go ahead and attempt to parse the
+ * filename, and see if we can pull out the required options. */
+r = qemu_rbd_attempt_legacy_options(options, &opts, &keypairs);
+if (r < 0) {
+error_propagate(errp, local_err);
+goto out;
+}
+/* Take care whenever deciding to actually deprecate; once this ability
+ * is removed, we will not be able to open any images with 
legacy-styled
+ * backing image strings. */
+error_report("RBD options encoded in the filename as keyvalue pairs "
+ "is deprecated.  Future versions may cease to parse "
+ "these options in the future.");
 }
 
 /* Remove the processed options from the QDict (the visitor processes
-- 
2.17.1

[Qemu-block] [PATCH v3 0/4] block/rbd: enable filename parsing on open

[Jeff Cody]

Changes from v2:
=

Patch 4: New, document deprecation. [Eric]
Patch 3,2: Add r-b's


Changes from v1:
=

Patch 1: Don't pass unused BlockDriverState to helper function

Patch 2: Do not allow mixed usage; fail if keyvalue is present [Eric]
 Add deprecation warning [John]
 Pull legacy parsing code into function [John]
 Fixed filename leak

Patch 3: New; iotest 231. [Eric]


iotest failure on current master:

 QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated. 
 Future versions may cease to parse these options in the future.
-unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
-qemu-img: Could not open 
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': 
error connecting: No such file or directory
+qemu-img: Could not open 
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': 
Parameter 'pool' is missing
 unable to get monitor info from DNS SRV with service name: ceph-mon
 no monitors specified to connect to.
 qemu-img: Could not open 
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
 error connecting: No such file or directory
Failures: 231
Failed 1 of 1 tests

Jeff Cody (4):
  block/rbd: pull out qemu_rbd_convert_options
  block/rbd: Attempt to parse legacy filenames
  block/rbd: add iotest for rbd legacy keyvalue filename parsing
  block/rbd: add deprecation documenation for filename keyvalue pairs

 block/rbd.c| 89 --
 qemu-deprecated.texi   |  7 +++
 tests/qemu-iotests/231 | 62 ++
 tests/qemu-iotests/231.out |  9 
 tests/qemu-iotests/group   |  1 +
 5 files changed, 154 insertions(+), 14 deletions(-)
 create mode 100755 tests/qemu-iotests/231
 create mode 100644 tests/qemu-iotests/231.out

-- 
2.17.1

[Qemu-block] [PATCH v3 3/4] block/rbd: add iotest for rbd legacy keyvalue filename parsing

[Jeff Cody]

This is a small test that will check for the ability to parse
both legacy and modern options for rbd.

The way the test is set up is for failure to occur, but without
having to wait to timeout on a non-existent rbd server.  The error
messages in the success path show that the arguments were parsed.

The failure behavior prior to the patch series that has this test, is
qemu-img complaining about mandatory options (e.g. 'pool') not being
provided.

Reviewed-by: Eric Blake 
Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/231 | 62 ++
 tests/qemu-iotests/231.out |  9 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 72 insertions(+)
 create mode 100755 tests/qemu-iotests/231
 create mode 100644 tests/qemu-iotests/231.out

diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
new file mode 100755
index 00..3e283708b4
--- /dev/null
+++ b/tests/qemu-iotests/231
@@ -0,0 +1,62 @@
+#!/bin/bash
+#
+# Test legacy and modern option parsing for rbd/ceph.  This will not
+# actually connect to a ceph server, but rather looks for the appropriate
+# error message that indicates we parsed the options correctly.
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=jc...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+rm "${BOGUS_CONF}"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto rbd
+_supported_os Linux
+
+BOGUS_CONF=${TEST_DIR}/ceph-$$.conf
+touch "${BOGUS_CONF}"
+
+_filter_conf()
+{
+sed -e "s#$BOGUS_CONF#BOGUS_CONF#g"
+}
+
+# We expect this to fail, with no monitor ip provided and a null conf file.  
Just want it
+# to fail in the right way.
+$QEMU_IMG info 
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}" 
2>&1 | _filter_conf
+$QEMU_IMG info 
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
 2>&1 | _filter_conf
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
new file mode 100644
index 00..579ba11c16
--- /dev/null
+++ b/tests/qemu-iotests/231.out
@@ -0,0 +1,9 @@
+QA output created by 231
+qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated. 
 Future versions may cease to parse these options in the future.
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open 
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': 
error connecting: No such file or directory
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open 
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
 error connecting: No such file or directory
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 743790745b..31f6e77dcb 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -226,3 +226,4 @@
 226 auto quick
 227 auto quick
 229 auto quick
+231 auto quick
-- 
2.17.1

Re: [Qemu-block] [PATCH v3 4/4] block/rbd: add deprecation documenation for filename keyvalue pairs

[Eric Blake]

[MAINTAINERS says libvir-list should have been cc'd; not sure why that 
didn't happen]


On 9/11/18 4:34 PM, Jeff Cody wrote:

Signed-off-by: Jeff Cody 


In the subject: s/documenation/documentation/


---
  qemu-deprecated.texi | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 1b9c007f12..4df8ac442d 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -190,6 +190,13 @@ used instead.
  In order to prevent QEMU from automatically opening an image's backing
  chain, use ``"backing": null'' instead.
  
+@subsubsection "rbd keyvalue pair encoded filenames": "" (since 3.1.0)

+
+Options for ``rbd'' should be specified according to its runtime options,
+like other block drivers.  Legacy parsing of keyvalue pair encoded
+filenames is useful to open images with the old format for backing files;
+These image files should be updated to use the current format.


Can we give an example?  Cribbing from patch 3, an example might look 
like changing:


json:{"file.driver":"rbd", "file.filename":"rbd:rbd/name"}

into:

json:{"file.driver":"rbd", "file.pool":"rbd", "file.image":"name"}

I'll let Peter or John comment on whether libvirt's RBD pool handler is 
impacted by this deprecation, but it seems reasonable to me.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH v3 4/4] block/rbd: add deprecation documenation for filename keyvalue pairs

[Jeff Cody]

On Tue, Sep 11, 2018 at 04:56:36PM -0500, Eric Blake wrote:
> [MAINTAINERS says libvir-list should have been cc'd; not sure why that
> didn't happen]
> 

Thanks

> On 9/11/18 4:34 PM, Jeff Cody wrote:
> >Signed-off-by: Jeff Cody 
> 
> In the subject: s/documenation/documentation/
> 
> >---
> >  qemu-deprecated.texi | 7 +++
> >  1 file changed, 7 insertions(+)
> >
> >diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> >index 1b9c007f12..4df8ac442d 100644
> >--- a/qemu-deprecated.texi
> >+++ b/qemu-deprecated.texi
> >@@ -190,6 +190,13 @@ used instead.
> >  In order to prevent QEMU from automatically opening an image's backing
> >  chain, use ``"backing": null'' instead.
> >+@subsubsection "rbd keyvalue pair encoded filenames": "" (since 3.1.0)
> >+
> >+Options for ``rbd'' should be specified according to its runtime options,
> >+like other block drivers.  Legacy parsing of keyvalue pair encoded
> >+filenames is useful to open images with the old format for backing files;
> >+These image files should be updated to use the current format.
> 
> Can we give an example?  Cribbing from patch 3, an example might look like
> changing:
> 
> json:{"file.driver":"rbd", "file.filename":"rbd:rbd/name"}
> 
> into:
> 
> json:{"file.driver":"rbd", "file.pool":"rbd", "file.image":"name"}
> 

That is a good example, I'll include it.

> I'll let Peter or John comment on whether libvirt's RBD pool handler is
> impacted by this deprecation, but it seems reasonable to me.
> 

Thanks!

[Qemu-block] [PATCH v4 1/4] block/rbd: pull out qemu_rbd_convert_options

[Jeff Cody]

Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
into a helper function.

Reviewed-by: Eric Blake 
Reviewed-by: John Snow 
Signed-off-by: Jeff Cody 
---
 block/rbd.c | 36 
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index ca8e5bbace..b199450f9f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -655,12 +655,34 @@ failed_opts:
 return r;
 }
 
+static int qemu_rbd_convert_options(QDict *options, BlockdevOptionsRbd **opts,
+Error **errp)
+{
+Visitor *v;
+Error *local_err = NULL;
+
+/* Convert the remaining options into a QAPI object */
+v = qobject_input_visitor_new_flat_confused(options, errp);
+if (!v) {
+return -EINVAL;
+}
+
+visit_type_BlockdevOptionsRbd(v, NULL, opts, &local_err);
+visit_free(v);
+
+if (local_err) {
+error_propagate(errp, local_err);
+return -EINVAL;
+}
+
+return 0;
+}
+
 static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
  Error **errp)
 {
 BDRVRBDState *s = bs->opaque;
 BlockdevOptionsRbd *opts = NULL;
-Visitor *v;
 const QDictEntry *e;
 Error *local_err = NULL;
 char *keypairs, *secretid;
@@ -676,19 +698,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
 qdict_del(options, "password-secret");
 }
 
-/* Convert the remaining options into a QAPI object */
-v = qobject_input_visitor_new_flat_confused(options, errp);
-if (!v) {
-r = -EINVAL;
-goto out;
-}
-
-visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
-visit_free(v);
-
+r = qemu_rbd_convert_options(options, &opts, &local_err);
 if (local_err) {
 error_propagate(errp, local_err);
-r = -EINVAL;
 goto out;
 }
 
-- 
2.17.1

[Qemu-block] [PATCH v4 2/4] block/rbd: Attempt to parse legacy filenames

[Jeff Cody]

When we converted rbd to get rid of the older key/value-centric
encoding format, we broke compatibility with image files with backing
file strings encoded in the old format.

This leaves a bit of an ugly conundrum, and a hacky solution.

If the initial attempt to parse the "proper" options fails, it assumes
that we may have an older key/value encoded filename.  Fall back to
attempting to parse the filename, and extract the required options from
it.  If that fails, pass along the original error message.

We do not support mixed modern usage alongside legacy keyvalue pair
usage.

A deprecation warning has been added, although care should be taken
when actually deprecating since the impact is not limited to
commandline or qapi usage, but also opening existing images.

Reviewed-by: Eric Blake 
Signed-off-by: Jeff Cody 
---
 block/rbd.c | 53 +++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index b199450f9f..5090e4f662 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options, 
BlockdevOptionsRbd **opts,
 return 0;
 }
 
+static int qemu_rbd_attempt_legacy_options(QDict *options,
+   BlockdevOptionsRbd **opts,
+   char **keypairs)
+{
+char *filename;
+int r;
+
+filename = g_strdup(qdict_get_try_str(options, "filename"));
+if (!filename) {
+return -EINVAL;
+}
+qdict_del(options, "filename");
+
+qemu_rbd_parse_filename(filename, options, NULL);
+
+/* keypairs freed by caller */
+*keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
+if (*keypairs) {
+qdict_del(options, "=keyvalue-pairs");
+}
+
+r = qemu_rbd_convert_options(options, opts, NULL);
+
+g_free(filename);
+return r;
+}
+
 static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
  Error **errp)
 {
@@ -700,8 +727,30 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 r = qemu_rbd_convert_options(options, &opts, &local_err);
 if (local_err) {
-error_propagate(errp, local_err);
-goto out;
+/* If keypairs are present, that means some options are present in
+ * the modern option format.  Don't attempt to parse legacy option
+ * formats, as we won't support mixed usage. */
+if (keypairs) {
+error_propagate(errp, local_err);
+goto out;
+}
+
+/* If the initial attempt to convert and process the options failed,
+ * we may be attempting to open an image file that has the rbd options
+ * specified in the older format consisting of all key/value pairs
+ * encoded in the filename.  Go ahead and attempt to parse the
+ * filename, and see if we can pull out the required options. */
+r = qemu_rbd_attempt_legacy_options(options, &opts, &keypairs);
+if (r < 0) {
+error_propagate(errp, local_err);
+goto out;
+}
+/* Take care whenever deciding to actually deprecate; once this ability
+ * is removed, we will not be able to open any images with 
legacy-styled
+ * backing image strings. */
+error_report("RBD options encoded in the filename as keyvalue pairs "
+ "is deprecated.  Future versions may cease to parse "
+ "these options in the future.");
 }
 
 /* Remove the processed options from the QDict (the visitor processes
-- 
2.17.1

[Qemu-block] [PATCH v4 0/4] block/rbd: enable filename parsing on open

[Jeff Cody]

Changes from v3:


Patch 4: Typo fixed [Eric]
 Added examples [Eric]

Changes from v2:
=

Patch 4: New, document deprecation. [Eric]
Patch 3,2: Add r-b's


Changes from v1:
=

Patch 1: Don't pass unused BlockDriverState to helper function

Patch 2: Do not allow mixed usage; fail if keyvalue is present [Eric]
 Add deprecation warning [John]
 Pull legacy parsing code into function [John]
 Fixed filename leak

Patch 3: New; iotest 231. [Eric]


iotest failure on current master:

 QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated. 
 Future versions may cease to parse these options in the future.
-unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
-qemu-img: Could not open 
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': 
error connecting: No such file or directory
+qemu-img: Could not open 
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': 
Parameter 'pool' is missing
 unable to get monitor info from DNS SRV with service name: ceph-mon
 no monitors specified to connect to.
 qemu-img: Could not open 
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
 error connecting: No such file or directory
Failures: 231
Failed 1 of 1 tests

Jeff Cody (4):
  block/rbd: pull out qemu_rbd_convert_options
  block/rbd: Attempt to parse legacy filenames
  block/rbd: add iotest for rbd legacy keyvalue filename parsing
  block/rbd: add deprecation documentation for filename keyvalue pairs

 block/rbd.c| 89 --
 qemu-deprecated.texi   | 15 +++
 tests/qemu-iotests/231 | 62 ++
 tests/qemu-iotests/231.out |  9 
 tests/qemu-iotests/group   |  1 +
 5 files changed, 162 insertions(+), 14 deletions(-)
 create mode 100755 tests/qemu-iotests/231
 create mode 100644 tests/qemu-iotests/231.out

-- 
2.17.1

[Qemu-block] [PATCH v4 3/4] block/rbd: add iotest for rbd legacy keyvalue filename parsing

[Jeff Cody]

This is a small test that will check for the ability to parse
both legacy and modern options for rbd.

The way the test is set up is for failure to occur, but without
having to wait to timeout on a non-existent rbd server.  The error
messages in the success path show that the arguments were parsed.

The failure behavior prior to the patch series that has this test, is
qemu-img complaining about mandatory options (e.g. 'pool') not being
provided.

Reviewed-by: Eric Blake 
Signed-off-by: Jeff Cody 
---
 tests/qemu-iotests/231 | 62 ++
 tests/qemu-iotests/231.out |  9 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 72 insertions(+)
 create mode 100755 tests/qemu-iotests/231
 create mode 100644 tests/qemu-iotests/231.out

diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
new file mode 100755
index 00..3e283708b4
--- /dev/null
+++ b/tests/qemu-iotests/231
@@ -0,0 +1,62 @@
+#!/bin/bash
+#
+# Test legacy and modern option parsing for rbd/ceph.  This will not
+# actually connect to a ceph server, but rather looks for the appropriate
+# error message that indicates we parsed the options correctly.
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=jc...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+rm "${BOGUS_CONF}"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto rbd
+_supported_os Linux
+
+BOGUS_CONF=${TEST_DIR}/ceph-$$.conf
+touch "${BOGUS_CONF}"
+
+_filter_conf()
+{
+sed -e "s#$BOGUS_CONF#BOGUS_CONF#g"
+}
+
+# We expect this to fail, with no monitor ip provided and a null conf file.  
Just want it
+# to fail in the right way.
+$QEMU_IMG info 
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}" 
2>&1 | _filter_conf
+$QEMU_IMG info 
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
 2>&1 | _filter_conf
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
new file mode 100644
index 00..579ba11c16
--- /dev/null
+++ b/tests/qemu-iotests/231.out
@@ -0,0 +1,9 @@
+QA output created by 231
+qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated. 
 Future versions may cease to parse these options in the future.
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open 
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': 
error connecting: No such file or directory
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open 
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
 error connecting: No such file or directory
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 743790745b..31f6e77dcb 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -226,3 +226,4 @@
 226 auto quick
 227 auto quick
 229 auto quick
+231 auto quick
-- 
2.17.1

[Qemu-block] [PATCH v4 4/4] block/rbd: add deprecation documentation for filename keyvalue pairs

[Jeff Cody]

Signed-off-by: Jeff Cody 
---
 qemu-deprecated.texi | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 1b9c007f12..8d285b281e 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -190,6 +190,21 @@ used instead.
 In order to prevent QEMU from automatically opening an image's backing
 chain, use ``"backing": null'' instead.
 
+@subsubsection rbd keyvalue pair encoded filenames: "" (since 3.1.0)
+
+Options for ``rbd'' should be specified according to its runtime options,
+like other block drivers.  Legacy parsing of keyvalue pair encoded
+filenames is useful to open images with the old format for backing files;
+These image files should be updated to use the current format.
+
+Example of legacy encoding:
+
+@code{json:@{"file.driver":"rbd", "file.filename":"rbd:rbd/name"@}}
+
+The above, converted to the current supported format:
+
+@code{json:@{"file.driver":"rbd", "file.pool":"rbd", "file.image":"name"@}}
+
 @subsection vio-spapr-device device options
 
 @subsubsection "irq": "" (since 3.0.0)
-- 
2.17.1

Re: [Qemu-block] [PATCH 2/2] virtio-scsi/virtio-blk: Disable poll handlers when stopping vq handler

[Fam Zheng]

On Tue, 09/11 17:30, Paolo Bonzini wrote:
> On 11/09/2018 16:12, Fam Zheng wrote:
> > On Tue, 09/11 13:32, Paolo Bonzini wrote:
> >> On 10/09/2018 16:56, Fam Zheng wrote:
> >>> We have this unwanted call stack:
> >>>
> >>>   > ...
> >>>   > #13 0x5586602b7793 in virtio_scsi_handle_cmd_vq
> >>>   > #14 0x5586602b8d66 in virtio_scsi_data_plane_handle_cmd
> >>>   > #15 0x5586602ddab7 in virtio_queue_notify_aio_vq
> >>>   > #16 0x5586602dfc9f in virtio_queue_host_notifier_aio_poll
> >>>   > #17 0x5586607885da in run_poll_handlers_once
> >>>   > #18 0x55866078880e in try_poll_mode
> >>>   > #19 0x5586607888eb in aio_poll
> >>>   > #20 0x558660784561 in aio_wait_bh_oneshot
> >>>   > #21 0x5586602b9582 in virtio_scsi_dataplane_stop
> >>>   > #22 0x5586605a7110 in virtio_bus_stop_ioeventfd
> >>>   > #23 0x5586605a9426 in virtio_pci_stop_ioeventfd
> >>>   > #24 0x5586605ab808 in virtio_pci_common_write
> >>>   > #25 0x558660242396 in memory_region_write_accessor
> >>>   > #26 0x5586602425ab in access_with_adjusted_size
> >>>   > #27 0x558660245281 in memory_region_dispatch_write
> >>>   > #28 0x5586601e008e in flatview_write_continue
> >>>   > #29 0x5586601e01d8 in flatview_write
> >>>   > #30 0x5586601e04de in address_space_write
> >>>   > #31 0x5586601e052f in address_space_rw
> >>>   > #32 0x5586602607f2 in kvm_cpu_exec
> >>>   > #33 0x558660227148 in qemu_kvm_cpu_thread_fn
> >>>   > #34 0x55866078bde7 in qemu_thread_start
> >>>   > #35 0x7f5784906594 in start_thread
> >>>   > #36 0x7f5784639e6f in clone
> >>>
> >>> Avoid it with the aio_disable_external/aio_enable_external pair, so that
> >>> no vq poll handlers can be called in aio_wait_bh_oneshot.
> >>
> >> I don't understand.  We are in the vCPU thread, so not in the
> >> AioContext's home thread.  Why is aio_wait_bh_oneshot polling rather
> >> than going through the aio_wait_bh path?
> > 
> > What do you mean by 'aio_wait_bh path'? Here is aio_wait_bh_oneshot:
> 
> Sorry, I meant the "atomic_inc(&wait_->num_waiters);" path.  But if this
> backtrace is obtained without dataplane, that's the answer I was seeking.
> 
> > void aio_wait_bh_oneshot(AioContext *ctx, QEMUBHFunc *cb, void *opaque)
> > {
> > AioWaitBHData data = {
> > .cb = cb,
> > .opaque = opaque,
> > };
> > 
> > assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> > 
> > aio_bh_schedule_oneshot(ctx, aio_wait_bh, &data);
> > AIO_WAIT_WHILE(&data.wait, ctx, !data.done);
> > }
> > 
> > ctx is qemu_aio_context here, so there's no interaction with IOThread.
> 
> In this case, it should be okay to have the reentrancy, what is the bug
> that this patch is fixing?

The same symptom as in the previous patch: virtio_scsi_handle_cmd_vq hangs. The
reason it hangs is fixed by the previous patch, but I don't think it should be
invoked as we're in the middle of virtio_scsi_dataplane_stop(). Applying either
one of the two patches avoids the problem, but this one is more superficial.
What do you think?

Fam