!job is always checked prior to the call, drop it from here.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockjob.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockjob.c b/blockjob.c
index 6e48932..23022b3 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -480,7 +480,7 @@ static bool block_job_should_pause(BlockJob *job)
 
 bool block_job_user_paused(BlockJob *job)
 {
-    return job ? job->user_paused : 0;
+    return job->user_paused;
 }
 
 void coroutine_fn block_job_pause_point(BlockJob *job)
-- 
2.9.3

On 04/19/2017 09:42 AM, Paolo Bonzini wrote:
> !job is always checked prior to the call, drop it from here.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  blockjob.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 6e48932..23022b3 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -480,7 +480,7 @@ static bool block_job_should_pause(BlockJob *job)
>  
>  bool block_job_user_paused(BlockJob *job)
>  {

Is it worth using some form of attribute((nonnull)) annotations on
various functions, to both state our intentions and let compilers help
us catch obvious places where we are violating our intentions?  That's
more of a generic question to all of qemu, and doesn't affect your
particular patch, other than your patch is an instance where the
annotation would be useful if we wanted to use them.

> -    return job ? job->user_paused : 0;
> +    return job->user_paused;
>  }
>  
>  void coroutine_fn block_job_pause_point(BlockJob *job)
> 

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

On 19/04/2017 17:48, Eric Blake wrote:
>>  bool block_job_user_paused(BlockJob *job)
>>  {
> Is it worth using some form of attribute((nonnull)) annotations on
> various functions, to both state our intentions and let compilers help
> us catch obvious places where we are violating our intentions?  That's
> more of a generic question to all of qemu, and doesn't affect your
> particular patch, other than your patch is an instance where the
> annotation would be useful if we wanted to use them.

What kind of bug would the compiler catch?  I suppose Coverity would
catch all of them, and maybe -flto would as well.

Paolo

>> -    return job ? job->user_paused : 0;
>> +    return job->user_paused;

On 04/19/2017 11:12 AM, Paolo Bonzini wrote:
> 
> 
> On 19/04/2017 17:48, Eric Blake wrote:
>>>  bool block_job_user_paused(BlockJob *job)
>>>  {
>> Is it worth using some form of attribute((nonnull)) annotations on
>> various functions, to both state our intentions and let compilers help
>> us catch obvious places where we are violating our intentions?  That's
>> more of a generic question to all of qemu, and doesn't affect your
>> particular patch, other than your patch is an instance where the
>> annotation would be useful if we wanted to use them.
> 
> What kind of bug would the compiler catch?  I suppose Coverity would
> catch all of them, and maybe -flto would as well.

Newer gcc is (finally) getting smarts about warning about passing a
known-NULL value to a function parameter marked nonnull; I think Clang
has been doing it for a while.  Coverity definitely flags mismatches
like that.  Other bugs that can be caught include checking a parameter
for NULL after it has already been declared to be non-null.  And if
nothing else, the documentation factor makes it easier for developers to
remember contracts of various functions.

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

Worth a resend to CC qemu-block, include a cover letter, and all the
usual amenities?

On 04/19/2017 10:42 AM, Paolo Bonzini wrote:
> !job is always checked prior to the call, drop it from here.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  blockjob.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 6e48932..23022b3 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -480,7 +480,7 @@ static bool block_job_should_pause(BlockJob *job)
>  
>  bool block_job_user_paused(BlockJob *job)
>  {
> -    return job ? job->user_paused : 0;
> +    return job->user_paused;
>  }
>  
>  void coroutine_fn block_job_pause_point(BlockJob *job)
> 

Reviewed-by: John Snow <jsnow@redhat.com>

On 25/04/2017 01:19, John Snow wrote:
> Worth a resend to CC qemu-block, include a cover letter, and all the
> usual amenities?

The cover letter was there, but yes, I'll ensure Jeff and qemu-block are
CCed on all patches.

Paolo