[Qemu-devel] [PATCH 0/2] small fix of block job

[Qemu-devel] [PATCH 0/2] small fix of block job

[Changlong Xie]

Changlong Xie (2):
  blockjob: assert(cb) in the entry functions of blockjob
  mirror: fix misleading comments

 block/commit.c | 1 +
 block/mirror.c | 4 +++-
 block/stream.c | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/2] blockjob: assert(cb) in the entry functions of blockjob

[Changlong Xie]

Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
---
 block/commit.c | 1 +
 block/mirror.c | 2 ++
 block/stream.c | 1 +
 3 files changed, 4 insertions(+)

diff --git a/block/commit.c b/block/commit.c
index 444333b..13b55c1 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -223,6 +223,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
     BlockDriverState *overlay_bs;
     Error *local_err = NULL;
 
+    assert(cb);
     assert(top != bs);
     if (top == base) {
         error_setg(errp, "Invalid files for merge: top and base are the same");
diff --git a/block/mirror.c b/block/mirror.c
index a04ed9c..fa2bdab 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -912,6 +912,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
     bool is_none_mode;
     BlockDriverState *base;
 
+    assert(cb);
     if (mode == MIRROR_SYNC_MODE_INCREMENTAL) {
         error_setg(errp, "Sync mode 'incremental' not supported");
         return;
@@ -935,6 +936,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
     int ret;
     Error *local_err = NULL;
 
+    assert(cb);
     orig_base_flags = bdrv_get_flags(base);
 
     if (bdrv_reopen(base, bs->open_flags, errp)) {
diff --git a/block/stream.c b/block/stream.c
index c0efbda..fc34c63 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -226,6 +226,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
 {
     StreamBlockJob *s;
 
+    assert(cb);
     s = block_job_create(&stream_job_driver, bs, speed, cb, opaque, errp);
     if (!s) {
         return;
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/2] blockjob: assert(cb) in the entry functions of blockjob

[Paolo Bonzini]

On 22/06/2016 11:16, Changlong Xie wrote:
> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> ---
>  block/commit.c | 1 +
>  block/mirror.c | 2 ++
>  block/stream.c | 1 +
>  3 files changed, 4 insertions(+)

Why is this useful?

Paolo

> diff --git a/block/commit.c b/block/commit.c
> index 444333b..13b55c1 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -223,6 +223,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
>      BlockDriverState *overlay_bs;
>      Error *local_err = NULL;
>  
> +    assert(cb);
>      assert(top != bs);
>      if (top == base) {
>          error_setg(errp, "Invalid files for merge: top and base are the same");
> diff --git a/block/mirror.c b/block/mirror.c
> index a04ed9c..fa2bdab 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -912,6 +912,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
>      bool is_none_mode;
>      BlockDriverState *base;
>  
> +    assert(cb);
>      if (mode == MIRROR_SYNC_MODE_INCREMENTAL) {
>          error_setg(errp, "Sync mode 'incremental' not supported");
>          return;
> @@ -935,6 +936,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
>      int ret;
>      Error *local_err = NULL;
>  
> +    assert(cb);
>      orig_base_flags = bdrv_get_flags(base);
>  
>      if (bdrv_reopen(base, bs->open_flags, errp)) {
> diff --git a/block/stream.c b/block/stream.c
> index c0efbda..fc34c63 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -226,6 +226,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
>  {
>      StreamBlockJob *s;
>  
> +    assert(cb);
>      s = block_job_create(&stream_job_driver, bs, speed, cb, opaque, errp);
>      if (!s) {
>          return;
> 

Re: [Qemu-devel] [PATCH 1/2] blockjob: assert(cb) in the entry functions of blockjob

[Changlong Xie]

On 06/22/2016 05:50 PM, Paolo Bonzini wrote:
>
>
> On 22/06/2016 11:16, Changlong Xie wrote:
>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>> ---
>>   block/commit.c | 1 +
>>   block/mirror.c | 2 ++
>>   block/stream.c | 1 +
>>   3 files changed, 4 insertions(+)
>
> Why is this useful?
>

commit/mirror/stream/backup use block_job_create(..., cb,..) to create 
relevant blockjob. When they finished, these jobs will invoke 
block_job_completed, then invoke job->cb() unconditionally. So i think
we need this to avoid segment fault. Actually backup has implemented this.

Thanks
	-Xie

> Paolo
>
>> diff --git a/block/commit.c b/block/commit.c
>> index 444333b..13b55c1 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -223,6 +223,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
>>       BlockDriverState *overlay_bs;
>>       Error *local_err = NULL;
>>
>> +    assert(cb);
>>       assert(top != bs);
>>       if (top == base) {
>>           error_setg(errp, "Invalid files for merge: top and base are the same");
>> diff --git a/block/mirror.c b/block/mirror.c
>> index a04ed9c..fa2bdab 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -912,6 +912,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
>>       bool is_none_mode;
>>       BlockDriverState *base;
>>
>> +    assert(cb);
>>       if (mode == MIRROR_SYNC_MODE_INCREMENTAL) {
>>           error_setg(errp, "Sync mode 'incremental' not supported");
>>           return;
>> @@ -935,6 +936,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
>>       int ret;
>>       Error *local_err = NULL;
>>
>> +    assert(cb);
>>       orig_base_flags = bdrv_get_flags(base);
>>
>>       if (bdrv_reopen(base, bs->open_flags, errp)) {
>> diff --git a/block/stream.c b/block/stream.c
>> index c0efbda..fc34c63 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -226,6 +226,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
>>   {
>>       StreamBlockJob *s;
>>
>> +    assert(cb);
>>       s = block_job_create(&stream_job_driver, bs, speed, cb, opaque, errp);
>>       if (!s) {
>>           return;
>>
>
>
>

Re: [Qemu-devel] [PATCH 1/2] blockjob: assert(cb) in the entry functions of blockjob

[Paolo Bonzini]

On 22/06/2016 12:12, Changlong Xie wrote:
> 
> commit/mirror/stream/backup use block_job_create(..., cb,..) to create
> relevant blockjob. When they finished, these jobs will invoke
> block_job_completed, then invoke job->cb() unconditionally. So i think
> we need this to avoid segment fault. Actually backup has implemented this.

So this suggests that the right place to put the assertion would be
block_job_create.  But it's even better to add a

#define QEMU_NONNULL __attribute__((__nonnull__))

to include/qemu/compiler.h and declare the arguments as non-null.

Paolo

Re: [Qemu-devel] [PATCH 1/2] blockjob: assert(cb) in the entry functions of blockjob

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

On 06/22/2016 04:19 AM, Paolo Bonzini wrote:
> 
> 
> On 22/06/2016 12:12, Changlong Xie wrote:
>>
>> commit/mirror/stream/backup use block_job_create(..., cb,..) to create
>> relevant blockjob. When they finished, these jobs will invoke
>> block_job_completed, then invoke job->cb() unconditionally. So i think
>> we need this to avoid segment fault. Actually backup has implemented this.
> 
> So this suggests that the right place to put the assertion would be
> block_job_create.  But it's even better to add a
> 
> #define QEMU_NONNULL __attribute__((__nonnull__))
> 
> to include/qemu/compiler.h and declare the arguments as non-null.

Or alternatively fix things to only invoke job->cb() if it is non-NULL,
so that callers don't have to pass in a no-op callback just to appease a
non-NULL attribute.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/2] blockjob: assert(cb) in the entry functions of blockjob

[Changlong Xie]

On 06/23/2016 01:31 AM, Eric Blake wrote:
> On 06/22/2016 04:19 AM, Paolo Bonzini wrote:
>>
>>
>> On 22/06/2016 12:12, Changlong Xie wrote:
>>>
>>> commit/mirror/stream/backup use block_job_create(..., cb,..) to create
>>> relevant blockjob. When they finished, these jobs will invoke
>>> block_job_completed, then invoke job->cb() unconditionally. So i think
>>> we need this to avoid segment fault. Actually backup has implemented this.
>>
>> So this suggests that the right place to put the assertion would be
>> block_job_create.  But it's even better to add a
>>
>> #define QEMU_NONNULL __attribute__((__nonnull__))
>>
>> to include/qemu/compiler.h and declare the arguments as non-null.
>
> Or alternatively fix things to only invoke job->cb() if it is non-NULL,
> so that callers don't have to pass in a no-op callback just to appease a
> non-NULL attribute.
>

Is there any reason, that we should invoke job->cb() unconditionally 
without nonnull check? There is no relevant clue in the historical 
commit messages. If yes, i prefer paolo's suggestion; otherwise eric's 
solution is better.

Thanks
	-Xie

Re: [Qemu-devel] [PATCH 1/2] blockjob: assert(cb) in the entry functions of blockjob

[Kevin Wolf]

Am 23.06.2016 um 03:04 hat Changlong Xie geschrieben:
> On 06/23/2016 01:31 AM, Eric Blake wrote:
> >On 06/22/2016 04:19 AM, Paolo Bonzini wrote:
> >>
> >>
> >>On 22/06/2016 12:12, Changlong Xie wrote:
> >>>
> >>>commit/mirror/stream/backup use block_job_create(..., cb,..) to create
> >>>relevant blockjob. When they finished, these jobs will invoke
> >>>block_job_completed, then invoke job->cb() unconditionally. So i think
> >>>we need this to avoid segment fault. Actually backup has implemented this.
> >>
> >>So this suggests that the right place to put the assertion would be
> >>block_job_create.  But it's even better to add a
> >>
> >>#define QEMU_NONNULL __attribute__((__nonnull__))
> >>
> >>to include/qemu/compiler.h and declare the arguments as non-null.
> >
> >Or alternatively fix things to only invoke job->cb() if it is non-NULL,
> >so that callers don't have to pass in a no-op callback just to appease a
> >non-NULL attribute.
> 
> Is there any reason, that we should invoke job->cb() unconditionally
> without nonnull check? There is no relevant clue in the historical
> commit messages. If yes, i prefer paolo's suggestion; otherwise
> eric's solution is better.

I don't think no-op callbacks actually exist for jobs.

Kevin

Re: [Qemu-devel] [PATCH 1/2] blockjob: assert(cb) in the entry functions of blockjob

[Changlong Xie]

On 06/23/2016 02:21 PM, Kevin Wolf wrote:
> Am 23.06.2016 um 03:04 hat Changlong Xie geschrieben:
>> On 06/23/2016 01:31 AM, Eric Blake wrote:
>>> On 06/22/2016 04:19 AM, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 22/06/2016 12:12, Changlong Xie wrote:
>>>>>
>>>>> commit/mirror/stream/backup use block_job_create(..., cb,..) to create
>>>>> relevant blockjob. When they finished, these jobs will invoke
>>>>> block_job_completed, then invoke job->cb() unconditionally. So i think
>>>>> we need this to avoid segment fault. Actually backup has implemented this.
>>>>
>>>> So this suggests that the right place to put the assertion would be
>>>> block_job_create.  But it's even better to add a
>>>>
>>>> #define QEMU_NONNULL __attribute__((__nonnull__))
>>>>
>>>> to include/qemu/compiler.h and declare the arguments as non-null.
>>>
>>> Or alternatively fix things to only invoke job->cb() if it is non-NULL,
>>> so that callers don't have to pass in a no-op callback just to appease a
>>> non-NULL attribute.
>>
>> Is there any reason, that we should invoke job->cb() unconditionally
>> without nonnull check? There is no relevant clue in the historical
>> commit messages. If yes, i prefer paolo's suggestion; otherwise
>> eric's solution is better.
>
> I don't think no-op callbacks actually exist for jobs.
>

So, i'll put assert(cb) in block_job_create as Paolo suggested.

Thanks
	-Xie

> Kevin
>
>
> .
>

[Qemu-devel] [PATCH 2/2] mirror: fix misleading comments

[Changlong Xie]

s/target bs/to_replace/, also we check to_replace bs is not
blocked in qmp_drive_mirror() not here

Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
---
 block/mirror.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index fa2bdab..335ddd2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -769,7 +769,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
         }
     }
 
-    /* check the target bs is not blocked and block all operations on it */
+    /* block all operations on to_replace bs */
     if (s->replaces) {
         AioContext *replace_aio_context;
 
-- 
1.9.3