Re: [Qemu-devel] [PATCH 37/42] job: Move progress fields to Job

[Eric Blake]

On 05/09/2018 11:26 AM, Kevin Wolf wrote:
> BlockJob has fields .offset and .len, which are actually misnomers today
> because they are no longer tied to block device sizes, but just progress
> counters. As such they make a lot of sense in generic Jobs.
>
> This patch moves the fields to Job and renames them to .progress_current
> and .progress_total to describe their function better.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---

> +++ b/include/qemu/job.h
> @@ -114,6 +114,16 @@ typedef struct Job {
>       /** True if this job should automatically dismiss itself */
>       bool auto_dismiss;
>   
> +    /**
> +     * Current progress. The unit is arbitrary as long as the ratio between
> +     * progress_current and progress_total represents the estimated percentage
> +     * of work already done.
> +     */
> +    int64_t progress_current;
> +
> +    /** Estimated progress_current value at the completion of the job */
> +    int64_t progress_total;
> +
>       /** ret code passed to block_job_completed. */
>       int ret;
>   
> @@ -304,6 +314,23 @@ void job_ref(Job *job);
>    */
>   void job_unref(Job *job);
>   
> +/**
> + * @job: The job that has made progress
> + * @done: How much progress the job made
> + *
> + * Updates the progress counter of the job.

Which progress counter? It might be worth calling out that this 
specifically updates the progress_current counter.  Also, it might be 
worth mentioning that the value is added to the current value of 
progress_counter, rather than directly assigned.

> + */
> +void job_progress_update(Job *job, uint64_t done);
> +
> +/**
> + * @job: The job whose expected progress end value is set
> + * @remaining: Expected end value of the progress counter of the job
> + *
> + * Sets the expected end value of the progress counter of a job so that a
> + * completion percentage can be calculated when the progress is updated.
> + */
> +void job_progress_set_remaining(Job *job, uint64_t remaining);

The name 'remaining' sounds like this is a delta (that progress_total is 
computed by summing the current progress_current + remaining) rather 
than directly the new value of progress_total.  Is that intended?

> +++ b/job.c
> @@ -364,6 +364,16 @@ void job_unref(Job *job)
>       }
>   }
>   
> +void job_progress_update(Job *job, uint64_t done)
> +{
> +    job->progress_current += done;
> +}
> +
> +void job_progress_set_remaining(Job *job, uint64_t remaining)
> +{
> +    job->progress_total = job->progress_current + remaining;

Okay, so implementation-wise, both functions ARE taking a delta, rather 
than directly assigning the internal field.  The delta can only be 
positive, but job->progress_total can still shrink over time as needed.

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