Re: [Qemu-devel] [PATCH 01/10] blockjob: remove unnecessary check

[Stefan Hajnoczi]

On Thu, Mar 23, 2017 at 06:39:19PM +0100, Paolo Bonzini wrote:
> !job is always checked prior to the call, drop it from here.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  blockjob.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 01/10] blockjob: remove unnecessary check

[John Snow]

On 04/07/2017 06:54 PM, Philippe Mathieu-Daudé wrote:
> Hi Paolo,
> 
> On 03/23/2017 02:39 PM, Paolo Bonzini wrote:
>> !job is always checked prior to the call, drop it from here.
> 
> I agree with you this is currently true, *but* block_job_user_paused()
> is exported in "block/blockjob.h" so any future access (external to
> blockdev.c) could eventually happen with job == NULL.
> I'd  NACK this patch for for this reason, but I checked and there is no
> access to this function outside of blockdev.c, so I think the best is to
> make block_job_user_paused() static (removing the public declaration)
> and safely drop the !job check, what do you think?
> 

Sure, but I would consider this a strict improvement as asking for the
paused status of NULL should be an error, not zero.

Anyway, this function exists almost entirely for the sake of blockdev,
so deleting it out of the public jobs interface isn't a very nice thing
to do.

The "proper" thing might be to add *errp, but that's a lot of paint for
a really tiny shed.

"IMO, etc." I've already spent more time on this email than it'd take to
code that, so whichever way the wind blows is fine with me.

--js

>>
>> Signed-off-by: Paolo Bonzini 
>> ---
>>  blockjob.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/blockjob.c b/blockjob.c
>> index 9b619f385..a9fb624 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)
>>

Re: [Qemu-devel] [PATCH 01/10] blockjob: remove unnecessary check

[Philippe Mathieu-Daudé]

Hi Paolo,

On 03/23/2017 02:39 PM, Paolo Bonzini wrote:

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


I agree with you this is currently true, *but* block_job_user_paused() 
is exported in "block/blockjob.h" so any future access (external to 
blockdev.c) could eventually happen with job == NULL.
I'd  NACK this patch for for this reason, but I checked and there is no 
access to this function outside of blockdev.c, so I think the best is to 
make block_job_user_paused() static (removing the public declaration) 
and safely drop the !job check, what do you think?




Signed-off-by: Paolo Bonzini 
---
 blockjob.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockjob.c b/blockjob.c
index 9b619f385..a9fb624 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)

Re: [Qemu-devel] [PATCH 01/10] blockjob: remove unnecessary check

[John Snow]

On 03/23/2017 01:39 PM, Paolo Bonzini wrote:
> !job is always checked prior to the call, drop it from here.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  blockjob.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 9b619f385..a9fb624 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 

[Qemu-devel] [PATCH 01/10] blockjob: remove unnecessary check

[Paolo Bonzini]

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

Signed-off-by: Paolo Bonzini 
---
 blockjob.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockjob.c b/blockjob.c
index 9b619f385..a9fb624 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