| Message ID | 20170323173928.14439-2-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <pbonzini@redhat.com> > --- > 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 <jsnow@redhat.com>
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 <pbonzini@redhat.com> > --- > 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) >
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 <pbonzini@redhat.com> >> --- >> 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) >>
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 <pbonzini@redhat.com> > --- > blockjob.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
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)
!job is always checked prior to the call, drop it from here. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- blockjob.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)