[Qemu-commits] [qemu/qemu] 7b43db: tests: fix bdrv-drain leak

[GitHub]

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: 7b43db3cd08f722d743c443ac3713195875d0301
      
https://github.com/qemu/qemu/commit/7b43db3cd08f722d743c443ac3713195875d0301
  Author: Marc-André Lureau <address@hidden>
  Date:   2018-08-31 (Fri, 31 Aug 2018)

  Changed paths:
    M tests/test-bdrv-drain.c

  Log Message:
  -----------
  tests: fix bdrv-drain leak

Spotted by ASAN:

=================================================================
==5378==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 65536 byte(s) in 1 object(s) allocated from:
    #0 0x7f788f83bc48 in malloc (/lib64/libasan.so.5+0xeec48)
    #1 0x7f788c9923c5 in g_malloc (/lib64/libglib-2.0.so.0+0x523c5)
    #2 0x5622a1fe37bc in coroutine_trampoline 
/home/elmarco/src/qq/util/coroutine-ucontext.c:116
    #3 0x7f788a15d75f in __correctly_grouped_prefixwc (/lib64/libc.so.6+0x4c75f)

(Broken in commit 4c8158e359d.)

Signed-off-by: Marc-André Lureau <address@hidden>
Message-id: address@hidden
Signed-off-by: Max Reitz <address@hidden>


  Commit: f67432a2019caf05b57a146bf45c1024a5cb608e
      
https://github.com/qemu/qemu/commit/f67432a2019caf05b57a146bf45c1024a5cb608e
  Author: John Snow <address@hidden>
  Date:   2018-08-31 (Fri, 31 Aug 2018)

  Changed paths:
    M block/backup.c
    M block/commit.c
    M block/create.c
    M block/mirror.c
    M block/stream.c
    M include/qemu/job.h
    M job.c
    M tests/test-bdrv-drain.c
    M tests/test-blockjob-txn.c
    M tests/test-blockjob.c

  Log Message:
  -----------
  jobs: change start callback to run callback

Presently we codify the entry point for a job as the "start" callback,
but a more apt name would be "run" to clarify the idea that when this
function returns we consider the job to have "finished," except for
any cleanup which occurs in separate callbacks later.

As part of this clarification, change the signature to include an error
object and a return code. The error ptr is not yet used, and the return
code while captured, will be overwritten by actions in the job_completed
function.

Signed-off-by: John Snow <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Message-id: address@hidden
Reviewed-by: Jeff Cody <address@hidden>
Signed-off-by: Max Reitz <address@hidden>


  Commit: 3d1f8b07a4c241f81949eff507d9f3a8fd73b87b
      
https://github.com/qemu/qemu/commit/3d1f8b07a4c241f81949eff507d9f3a8fd73b87b
  Author: John Snow <address@hidden>
  Date:   2018-08-31 (Fri, 31 Aug 2018)

  Changed paths:
    M block/backup.c
    M block/commit.c
    M block/create.c
    M block/mirror.c
    M block/stream.c
    M include/qemu/job.h
    M job-qmp.c
    M job.c
    M tests/test-bdrv-drain.c
    M tests/test-blockjob-txn.c
    M tests/test-blockjob.c

  Log Message:
  -----------
  jobs: canonize Error object

Jobs presently use both an Error object in the case of the create job,
and char strings in the case of generic errors elsewhere.

Unify the two paths as just j->err, and remove the extra argument from
job_completed. The integer error code for job_completed is kept for now,
to be removed shortly in a separate patch.

Signed-off-by: John Snow <address@hidden>
Message-id: address@hidden
[mreitz: Dropped a superfluous g_strdup()]
Reviewed-by: Eric Blake <address@hidden>
Signed-off-by: Max Reitz <address@hidden>


  Commit: 00359a71d45a414ee47d8e423104dc0afd24ec65
      
https://github.com/qemu/qemu/commit/00359a71d45a414ee47d8e423104dc0afd24ec65
  Author: John Snow <address@hidden>
  Date:   2018-08-31 (Fri, 31 Aug 2018)

  Changed paths:
    M include/qemu/job.h
    M job.c

  Log Message:
  -----------
  jobs: add exit shim

All jobs do the same thing when they leave their running loop:
- Store the return code in a structure
- wait to receive this structure in the main thread
- signal job completion via job_completed

Few jobs do anything beyond exactly this. Consolidate this exit
logic for a net reduction in SLOC.

More seriously, when we utilize job_defer_to_main_loop_bh to call
a function that calls job_completed, job_finalize_single will run
in a context where it has recursively taken the aio_context lock,
which can cause hangs if it puts down a reference that causes a flush.

You can observe this in practice by looking at mirror_exit's careful
placement of job_completed and bdrv_unref calls.

If we centralize job exiting, we can signal job completion from outside
of the aio_context, which should allow for job cleanup code to run with
only one lock, which makes cleanup callbacks less tricky to write.

Signed-off-by: John Snow <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Message-id: address@hidden
Reviewed-by: Jeff Cody <address@hidden>
Signed-off-by: Max Reitz <address@hidden>


  Commit: f369b48dc4095861223f9bc4329935599e03b1c5
      
https://github.com/qemu/qemu/commit/f369b48dc4095861223f9bc4329935599e03b1c5
  Author: John Snow <address@hidden>
  Date:   2018-08-31 (Fri, 31 Aug 2018)

  Changed paths:
    M block/commit.c

  Log Message:
  -----------
  block/commit: utilize job_exit shim

Change the manual deferment to commit_complete into the implicit
callback to job_exit, renaming commit_complete to commit_exit.

This conversion does change the timing of when job_completed is
called to after the bdrv_replace_node and bdrv_unref calls, which
could have implications for bjob->blk which will now be put down
after this cleanup.

Kevin highlights that we did not take any permissions for that backend
at job creation time, so it is safe to reorder these operations.

Signed-off-by: John Snow <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Message-id: address@hidden
Reviewed-by: Jeff Cody <address@hidden>
Signed-off-by: Max Reitz <address@hidden>


  Commit: 7b508f6b7a38a8d9729772fa6e525da883fb120b
      
https://github.com/qemu/qemu/commit/7b508f6b7a38a8d9729772fa6e525da883fb120b
  Author: John Snow <address@hidden>
  Date:   2018-08-31 (Fri, 31 Aug 2018)

  Changed paths:
    M block/mirror.c

  Log Message:
  -----------
  block/mirror: utilize job_exit shim

Change the manual deferment to mirror_exit into the implicit
callback to job_exit and the mirror_exit callback.

This does change the order of some bdrv_unref calls and job_completed,
but thanks to the new context in which we call .exit, this is safe to
defer the possible flushing of any nodes to the job_finalize_single
cleanup stage.

Signed-off-by: John Snow <address@hidden>
Message-id: address@hidden
Reviewed-by: Max Reitz <address@hidden>
Reviewed-by: Jeff Cody <address@hidden>
Signed-off-by: Max Reitz <address@hidden>


  Commit: eb23654dbe43b549ea2a9ebff9d8edf544d34a73
      
https://github.com/qemu/qemu/commit/eb23654dbe43b549ea2a9ebff9d8edf544d34a73
  Author: John Snow <address@hidden>
  Date:   2018-08-31 (Fri, 31 Aug 2018)

  Changed paths:
    M block/backup.c
    M block/create.c
    M block/stream.c
    M tests/test-bdrv-drain.c
    M tests/test-blockjob-txn.c
    M tests/test-blockjob.c

  Log Message:
  -----------
  jobs: utilize job_exit shim

Utilize the job_exit shim by not calling job_defer_to_main_loop, and
where applicable, converting the deferred callback into the job_exit
callback.

This converts backup, stream, create, and the unit tests all at once.
Most of these jobs do not see any changes to the order in which they
clean up their resources, except the test-blockjob-txn test, which
now puts down its bs before job_completed is called.

This is safe for the same reason the reordering in the mirror job is
safe, because job_completed no longer runs under two locks, making
the unref safe even if it causes a flush.

Signed-off-by: John Snow <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Message-id: address@hidden
Signed-off-by: Max Reitz <address@hidden>


  Commit: 6870277535493fea31761d8d11ec23add2de0fb0
      
https://github.com/qemu/qemu/commit/6870277535493fea31761d8d11ec23add2de0fb0
  Author: John Snow <address@hidden>
  Date:   2018-08-31 (Fri, 31 Aug 2018)

  Changed paths:
    M block/backup.c

  Log Message:
  -----------
  block/backup: make function variables consistently named

Rename opaque_job to job to be consistent with other job implementations.
Rename 'job', the BackupBlockJob object, to 's' to also be consistent.

Suggested-by: Eric Blake <address@hidden>
Signed-off-by: John Snow <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Message-id: address@hidden
Signed-off-by: Max Reitz <address@hidden>


  Commit: 404ff28d6ae59fc1c24d631710d4063fc68aed03
      
https://github.com/qemu/qemu/commit/404ff28d6ae59fc1c24d631710d4063fc68aed03
  Author: John Snow <address@hidden>
  Date:   2018-08-31 (Fri, 31 Aug 2018)

  Changed paths:
    M include/qemu/job.h
    M job.c
    M trace-events

  Log Message:
  -----------
  jobs: remove ret argument to job_completed; privatize it

Jobs are now expected to return their retcode on the stack, from the
.run callback, so we can remove that argument.

job_cancel does not need to set -ECANCELED because job_completed will
update the return code itself if the job was canceled.

While we're here, make job_completed static to job.c and remove it from
job.h; move the documentation of return code to the .run() callback and
to the job->ret property, accordingly.

Signed-off-by: John Snow <address@hidden>
Message-id: address@hidden
Reviewed-by: Max Reitz <address@hidden>
Signed-off-by: Max Reitz <address@hidden>


  Commit: e21a1c9831fc80ae3f3c1affdfa43350035d8588
      
https://github.com/qemu/qemu/commit/e21a1c9831fc80ae3f3c1affdfa43350035d8588
  Author: John Snow <address@hidden>
  Date:   2018-08-31 (Fri, 31 Aug 2018)

  Changed paths:
    M include/qemu/job.h
    M job.c

  Log Message:
  -----------
  jobs: remove job_defer_to_main_loop

Now that the job infrastructure is handling the job_completed call for
all implemented jobs, we can remove the interface that allowed jobs to
schedule their own completion.

Signed-off-by: John Snow <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Message-id: address@hidden
Signed-off-by: Max Reitz <address@hidden>


  Commit: d6f71af65410d3e003ba331c5e57eddcf716cbcf
      
https://github.com/qemu/qemu/commit/d6f71af65410d3e003ba331c5e57eddcf716cbcf
  Author: Peter Maydell <address@hidden>
  Date:   2018-09-24 (Mon, 24 Sep 2018)

  Changed paths:
    M block/backup.c
    M block/commit.c
    M block/create.c
    M block/mirror.c
    M block/stream.c
    M include/qemu/job.h
    M job-qmp.c
    M job.c
    M tests/test-bdrv-drain.c
    M tests/test-blockjob-txn.c
    M tests/test-blockjob.c
    M trace-events

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/xanclic/tags/pull-block-2018-08-31-v2' 
into staging

Block patches:
- (Block) job exit refactoring, part 1
  (removing job_defer_to_main_loop())
- test-bdrv-drain leak fix

# gpg: Signature made Fri 31 Aug 2018 15:30:33 BST
# gpg:                using RSA key F407DB0061D5CF40
# gpg: Good signature from "Max Reitz <address@hidden>"
# Primary key fingerprint: 91BE B60A 30DB 3E88 57D1  1829 F407 DB00 61D5 CF40

* remotes/xanclic/tags/pull-block-2018-08-31-v2:
  jobs: remove job_defer_to_main_loop
  jobs: remove ret argument to job_completed; privatize it
  block/backup: make function variables consistently named
  jobs: utilize job_exit shim
  block/mirror: utilize job_exit shim
  block/commit: utilize job_exit shim
  jobs: add exit shim
  jobs: canonize Error object
  jobs: change start callback to run callback
  tests: fix bdrv-drain leak

Signed-off-by: Peter Maydell <address@hidden>


Compare: https://github.com/qemu/qemu/compare/539c251b64d2...d6f71af65410
      **NOTE:** This service has been marked for deprecation: 
https://developer.github.com/changes/2018-04-25-github-services-deprecation/

      Functionality will be removed from GitHub.com on January 31st, 2019.