[v5,2/4] block.c: adding bdrv_delete_file

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é <berrange@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 block.c               | 77 +++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  1 +
 2 files changed, 78 insertions(+)

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é <berrange@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  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.

> +        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 {
>

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é <berrange@redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   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 {
>>

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é <berrange@redhat.com>
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> >  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.

Kevin

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é <berrange@redhat.com>
>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> ---
>>>   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!



>
> Kevin

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é <berrange@redhat.com>
> > > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > > ---
> > > >   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