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

[Kevin Wolf]

Am 03.09.2019 um 11:55 hat Daniel Henrique Barboza geschrieben:
> 
> 
> On 9/3/19 6:22 AM, Kevin Wolf wrote:
> > Am 29.08.2019 um 04:07 hat John Snow geschrieben:
> > > 
> > > 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.
> > > 
> > > > +    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).
> > > 
> > > 
> > > 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.
> > Yes, this was definitely the idea I had in mind when I suggested that
> > bdrv_co_delete_file() should take a BDS.
> > 
> > And I think the callers that want to call this function (for failures
> > during image creation) all already have a BDS pointer, so nobody will
> > actually need the additional open.
> > 
> > const char *filename only works for the local filesystem (and even then
> > I think not for all filenames) and some URLs, so this is not what we
> > want to have in a public interface to identify an image file.
> 
> Hmpf, I understood your idead wrong in the v4 review and ended up
> changing the co_routine (bdrv_co_delete_file) to use the BlockDriverState
> instead of the public interface bdrv_delete_file that will be called inside
> crypto.c.
> 
> I'll respin it again with this change. Thanks for clarifying!

Yes, I see that I only really commented on the BlockDriver callback, so
it wasn't very clear what I actually meant. Sorry for that.

Kevin