Re: [Qemu-devel] [PATCH v5 2/4] block.c: adding bdrv_delete_file

[Daniel Henrique Barboza]

On 8/28/19 11:07 PM, John Snow wrote:
> On 8/7/19 10:21 AM, Daniel Henrique Barboza wrote:
> > Using the new 'bdrv_co_delete_file' interface, bdrv_delete_file
> > can be used in a way similar of the existing bdrv_create_file to
> > to clean up a created file.
> >
> > The logic is also similar to what is already done in bdrv_create_file:
> > a qemu_coroutine is created if needed, a specialized function
> > bdrv_delete_co_entry is used to call the bdrv_co_delete_file
> > co-routine of the driver, if the driver implements it.
> >
> > Suggested-by: Daniel P. Berrangé <address@hidden>
> > Signed-off-by: Daniel Henrique Barboza <address@hidden>
> > ---
> >   block.c               | 77 +++++++++++++++++++++++++++++++++++++++++++
> >   include/block/block.h |  1 +
> >   2 files changed, 78 insertions(+)
> >
> > diff --git a/block.c b/block.c
> > index cbd8da5f3b..1e20250627 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -547,6 +547,83 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, 
> > Error **errp)
> >       return ret;
> >   }
> >   
> > +typedef struct DeleteCo {
> > +    BlockDriver *drv;
> > +    BlockDriverState *bs;
> > +    int ret;
> > +    Error *err;
> > +} DeleteCo;
> > +
> > +static void coroutine_fn bdrv_delete_co_entry(void *opaque)
> > +{
> > +    Error *local_err = NULL;
> > +    DeleteCo *dco = opaque;
> > +
> > +    assert(dco->bs);
> > +
> > +    dco->ret = dco->drv->bdrv_co_delete_file(dco->bs, &local_err);
> > +    error_propagate(&dco->err, local_err);
> > +}
> > +
> > +int bdrv_delete_file(const char *filename, Error **errp)
> > +{
> > +    BlockDriver *drv = bdrv_find_protocol(filename, true, NULL);
> > +    BlockDriverState *bs = bdrv_open(filename, NULL, NULL,
> > +                                     BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL);
> > +    DeleteCo dco = {
> > +        .drv = drv,
> > +        .bs = bs,
> > +        .ret = NOT_DONE,
> > +        .err = NULL,
> > +    };
> > +    Coroutine *co;
> > +    int ret;
> > +
> > +    if (!drv) {
> > +        error_setg(errp, "File '%s' has unknown format", filename);
> > +        ret = -ENOENT;
> > +        goto out;
> > +    }
> > +
> I was going to say that ENOENT is a weird error here, but I see it used
> for !drv a few other places in block.c too, alongside EINVAL and
> ENOMEDIUM. ENOMEDIUM loks like the most popular.

Didn't spend too much time thinking about it. I copied the same behavior 
from
bdrv_create_file:

---------

int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
{
    (...)

    drv = bdrv_find_protocol(filename, true, errp);
    if (drv == NULL) {
        return -ENOENT;
    }
-----

I can change to ENOMEDIUM if it's indeed more informative than ENOENT.


> > +    if (!drv->bdrv_co_delete_file) {
> > +        error_setg(errp, "Driver '%s' does not support image delete",
> > +                   drv->format_name);
> > +        ret = -ENOTSUP;
> > +        goto out;
> > +    }
> > +
> > +    if (!bs) {
> > +        error_setg(errp, "Could not open image '%s' for erasing",
> > +                   filename);
> > +        ret = 1;
> Please keep all errors negative (or at least consistent within a function).

Got it. I'll fix it in the re-spin.


> I'm also wondering if we want a version of delete that doesn't try to
> open a file directly -- i.e. a version that exists like this:
>
> bdrv_co_delete_file(BlockDriverState *bs, Error **errp);
>
> That simply dispatches based on bs->drv to the correct routine.
>
> Then, you are free to have bdrv_delete_file handle the open (and let the
> opening figure out what driver it needs), and just hand off the bds to
> bdrv_co_delete_file.
>
> I'm not the authority for block.c, though, so maaaybe I'm giving you bad
> advice here. Kevin's away on PTO for a bit and gave you advice most
> recently, so I might try to gently ask him for more feedback next week.

I appreciate. I'm not acquainted with the block code at all - I'm playing
by ear since the first version. Any tip is appreciated :)


Thanks,


DHB

> > +        goto out;
> > +    }
> > +
> > +    if (qemu_in_coroutine()) {
> > +        /* Fast-path if already in coroutine context */
> > +        bdrv_delete_co_entry(&dco);
> > +    } else {
> > +        co = qemu_coroutine_create(bdrv_delete_co_entry, &dco);
> > +        qemu_coroutine_enter(co);
> > +        while (dco.ret == NOT_DONE) {
> > +            aio_poll(qemu_get_aio_context(), true);
> > +        }
> > +    }
> > +
> > +    ret = dco.ret;
> > +    if (ret < 0) {
> > +        if (dco.err) {
> > +            error_propagate(errp, dco.err);
> > +        } else {
> > +            error_setg_errno(errp, -ret, "Could not delete image");
> > +        }
> > +    }
> > +
> > +out:
> > +    bdrv_unref(bs);
> > +    return ret;
> > +}
> > +
> >   /**
> >    * Try to get @bs's logical and physical block size.
> >    * On success, store them in @bsz struct and return 0.
> > diff --git a/include/block/block.h b/include/block/block.h
> > index 50a07c1c33..5e83532364 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -369,6 +369,7 @@ bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, 
> > BlockDriverState *base,
> >   int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
> >                                 Error **errp);
> >   void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState 
> > *base);
> > +int bdrv_delete_file(const char *filename, Error **errp);
> >   
> >   
> >   typedef struct BdrvCheckResult {