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