Re: [Qemu-devel] [PATCH v3 04/11] block: Use block_job_get() in find_block_job()

[Kevin Wolf]

Am 02.07.2016 um 16:02 hat Max Reitz geschrieben:
> On 01.07.2016 17:52, Alberto Garcia wrote:
> > find_block_job() looks for a block backend with a specified name,
> > checks whether it has a block job and acquires its AioContext.
> > 
> > We want to identify jobs by their ID and not by the block backend
> > they're attached to, so this patch ignores the backends altogether and
> > gets the job directly. Apart from making the code simpler, this will
> > allow us to find block jobs once they start having user-specified IDs.
> > 
> > To ensure backward compatibility we keep ERROR_CLASS_DEVICE_NOT_ACTIVE
> > as the error class if the job doesn't exist. In subsequent patches
> > we'll also need to keep the device name as the default job ID if the
> > user doesn't specify a different one.
> > 
> > Signed-off-by: Alberto Garcia <address@hidden>
> > ---
> >  blockdev.c | 43 ++++++++++++++++---------------------------
> >  1 file changed, 16 insertions(+), 27 deletions(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 3a104a0..8cedb60 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -3704,42 +3704,31 @@ void qmp_blockdev_mirror(const char *device, const 
> > char *target,
> >      aio_context_release(aio_context);
> >  }
> >  
> > -/* Get the block job for a given device name and acquire its AioContext */
> > -static BlockJob *find_block_job(const char *device, AioContext 
> > **aio_context,
> > +/* Get a block job using its ID and acquire its AioContext */
> > +static BlockJob *find_block_job(const char *id, AioContext **aio_context,
> >                                  Error **errp)
> >  {
> > -    BlockBackend *blk;
> > -    BlockDriverState *bs;
> > +    BlockJob *job;
> >  
> >      *aio_context = NULL;
> >  
> > -    blk = blk_by_name(device);
> > -    if (!blk) {
> > -        goto notfound;
> > +    if (!id) {
> > +        error_setg(errp, "Unspecified job ID when looking for a block 
> > job");
> > +        return NULL;
> >      }
> 
> Why no plain assertion? Do you expect callers who may pass a NULL ID?
> 
> >  
> > -    *aio_context = blk_get_aio_context(blk);
> > +    job = block_job_get(id);
> > +
> > +    if (!job) {
> > +        error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
> > +                  "Block job '%s' not found", id);
> 
> This error class seems a bit weird now... I know I advocated for it in
> v2, but that was because you could actually specifically pass a device
> name to find block jobs on that device, but now you're just looking for
> block jobs with a certain ID (that happens to default to the device name).

libvirt uses the error class, so I don't think we can drop it.

Kevin

pgpHXI3o1mnnj.pgp
Description: PGP signature