Re: [Qemu-devel] [PATCH 12/42] job: Maintain a list of all jobs

[Eric Blake]

On 05/09/2018 11:26 AM, Kevin Wolf wrote:
> This moves the job list from BlockJob to Job. Now we can check for
> duplicate IDs in job_create().
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>   include/block/blockjob.h |  3 ---
>   include/qemu/job.h       | 19 +++++++++++++++++++
>   blockjob.c               | 47 +++++++++++++++++++++++++----------------------
>   job.c                    | 31 +++++++++++++++++++++++++++++++
>   4 files changed, 75 insertions(+), 25 deletions(-)

> @@ -71,4 +75,19 @@ JobType job_type(Job *job);
>   /** Returns the enum string for the JobType of a given Job. */
>   const char *job_type_str(Job *job);
>   
> +/**
> + * Get the next element from the list of block jobs after @job, or the
> + * first one if @job is %NULL.
> + *
> + * Returns the requested job, or %NULL if there are no more jobs left.
> + */
> +Job *job_next(Job *job);

The new code is supposed to allow you to prime the search by passing in 
NULL.  This looks similar to the semantics originally held by 
block_job_next(), where


> -BlockJob *block_job_next(BlockJob *job)
> +static bool is_block_job(Job *job)
>   {
> -    if (!job) {
> -        return QLIST_FIRST(&block_jobs);
> -    }

the old code did so by special-casing an incoming NULL.

> -    return QLIST_NEXT(job, job_list);
> +    return job_type(job) == JOB_TYPE_BACKUP ||
> +           job_type(job) == JOB_TYPE_COMMIT ||
> +           job_type(job) == JOB_TYPE_MIRROR ||
> +           job_type(job) == JOB_TYPE_STREAM;
> +}
> +
> +BlockJob *block_job_next(BlockJob *bjob)
> +{
> +    Job *job = &bjob->job;

But the new code blindly dereferences what may be a NULL bjob.  Taking 
the address of a member of the NULL pointer may happen to work if that 
member is at offset 0, but is also likely to trigger compiler or 
sanitizer warnings; and does not work if the member is not at offset 0.

I think all you have to do is:

Job *job = bjob ? &bjob->job : NULL;

> +
> +    do {
> +        job = job_next(job);
> +    } while (job && !is_block_job(job));
> +
> +
> +    return job ? container_of(job, BlockJob, job) : NULL;

Why two blank lines?

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