Re: [Qemu-block] [PATCH 18/21] backup: new async architecture

[Stefan Hajnoczi]

On Fri, Dec 23, 2016 at 05:29:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:

Just a few comments.  I need to resume review of this series tomorrow.

> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 1acb256..7d24cf6 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -248,6 +248,15 @@ void block_job_user_pause(BlockJob *job);
>  bool block_job_user_paused(BlockJob *job);
>  
>  /**
> + * block_job_should_pause:
> + * @job: The job being queried.
> + *
> + * Returns whether the job is currently paused, or will pause
> + * as soon as it reaches a sleeping point.

s/sleeping point/pause point/

> + */
> +bool block_job_should_pause(BlockJob *job);
> +
> +/**
>   * block_job_resume:
>   * @job: The job to be resumed.
>   *
> @@ -309,7 +318,11 @@ int block_job_complete_sync(BlockJob *job, Error **errp);
>   */
>  void block_job_iostatus_reset(BlockJob *job);
>  
> -/**
> +
> +BlockErrorAction block_job_get_error_action(BlockJob *job,
> +                                            BlockdevOnError on_err, int 
> error);

Missing doc comment.

> +
> +/*
>   * block_job_txn_new:
>   *
>   * Allocate and return a new block job transaction.  Jobs can be added to the
> diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
> index 388b7b2..e15905f 100755
> --- a/tests/qemu-iotests/055
> +++ b/tests/qemu-iotests/055
> @@ -553,4 +553,4 @@ class TestDriveCompression(iotests.QMPTestCase):
>              self.do_test_compress_pause('blockdev-backup', format, 
> target='drive1')
>  
>  if __name__ == '__main__':
> -    iotests.main(supported_fmts=['raw', 'qcow2'])
> +    iotests.main(supported_fmts=['raw', 'qcow2'], 
> supported_cache_modes=['none'])

The test has no real dependency on cache=none.  Did you just add this to
influence performance in the hope that the test runs more
deterministically?

> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> index 9e87e1c..3d5e137 100644
> --- a/tests/qemu-iotests/129
> +++ b/tests/qemu-iotests/129
> @@ -66,8 +66,10 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
>          result = self.vm.qmp("stop")
>          self.assert_qmp(result, 'return', {})
>          result = self.vm.qmp("query-block-jobs")
> -        self.assert_qmp(result, 'return[0]/busy', True)
> -        self.assert_qmp(result, 'return[0]/ready', False)
> +        if result['return']:
> +            # make additional check if block job is not released yet
> +            self.assert_qmp(result, 'return[0]/busy', True)
> +            self.assert_qmp(result, 'return[0]/ready', False)

This is silencing a failure rather than improving the test case.  Why
does the test case fail for you?  The rate limit is 1 KB/s, so the job
should have no chance of completing.

signature.asc
Description: PGP signature