Re: [PATCH 08/14] blockjob: return status from block_job_set_speed()

[Greg Kurz]

On Wed,  9 Sep 2020 21:59:24 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> Better to return status together with setting errp. It allows to avoid
> error propagation in the caller.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  include/block/blockjob.h |  2 +-
>  blockjob.c               | 18 ++++++++----------
>  2 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 35faa3aa26..d200f33c10 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -139,7 +139,7 @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState 
> *bs);
>   * Set a rate-limiting parameter for the job; the actual meaning may
>   * vary depending on the job type.
>   */
> -void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
> +bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
>  
>  /**
>   * block_job_query:
> diff --git a/blockjob.c b/blockjob.c
> index 470facfd47..afddf7a1fb 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -254,28 +254,30 @@ static bool job_timer_pending(Job *job)
>      return timer_pending(&job->sleep_timer);
>  }
>  
> -void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
> +bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
>  {
>      int64_t old_speed = job->speed;
>  
> -    if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp)) {
> -        return;
> +    if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) {
> +        return false;
>      }
>      if (speed < 0) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
>                     "a non-negative value");
> -        return;
> +        return false;
>      }
>  
>      ratelimit_set_speed(&job->limit, speed, BLOCK_JOB_SLICE_TIME);
>  
>      job->speed = speed;
>      if (speed && speed <= old_speed) {
> -        return;
> +        return true;
>      }
>  
>      /* kick only if a timer is pending */
>      job_enter_cond(&job->job, job_timer_pending);
> +
> +    return true;
>  }
>  
>  int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n)
> @@ -448,12 +450,8 @@ void *block_job_create(const char *job_id, const 
> BlockJobDriver *driver,
>  
>      /* Only set speed when necessary to avoid NotSupported error */
>      if (speed != 0) {
> -        Error *local_err = NULL;
> -
> -        block_job_set_speed(job, speed, &local_err);
> -        if (local_err) {
> +        if (!block_job_set_speed(job, speed, errp)) {
>              job_early_fail(&job->job);
> -            error_propagate(errp, local_err);
>              return NULL;
>          }
>      }