09.12.2020 23:33, Maxim Levitsky wrote:
This function wraps bdrv_co_delete_file for the common case of removing a file,
which was just created by format driver, on an error condition.
It hides the -ENOTSUPP error, and reports all other errors otherwise.
I've looked at original commit added this logic, and didn't find exactly, why
should we hide it..
Signed-off-by: Maxim Levitsky <mlevi...@redhat.com>
Reviewed-by: Alberto Garcia <be...@igalia.com>
---
block.c | 24 ++++++++++++++++++++++++
include/block/block.h | 1 +
2 files changed, 25 insertions(+)
diff --git a/block.c b/block.c
index f1cedac362..5d35ba2fb8 100644
--- a/block.c
+++ b/block.c
@@ -704,6 +704,30 @@ int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs,
Error **errp)
return ret;
}
+void coroutine_fn bdrv_co_delete_file_noerr(BlockDriverState *bs)
+{
+ Error *local_err = NULL;
+ int ret;
+
+ if (!bs) {
+ return;
+ }
+
+ ret = bdrv_co_delete_file(bs, &local_err);
+ /*
+ * ENOTSUP will happen if the block driver doesn't support
+ * the 'bdrv_co_delete_file' interface. This is a predictable
+ * scenario and shouldn't be reported back to the user.
+ */
It's not predictable for user: user doesn't know internal processes and
interfaces.
The problem: we've left extra file on failure scenario and can't remove it. We
want to warn the user that there is a wrong file left. Why not to warn, when
the removement is unsupported internally by the driver?
I think, it's better to report it in any case.
Another reason: less code and logic on failure case. Optimizing failure
scenarios in different manner is seldom a good thing to do.
And finally: why to report the error at all? If we have errp parameter, it's better to
add the info to it using error_append.. something like error_append(errp, " (also
failed to remove unfinished file %s: %s)", file_name, error_get_pretty(local_err))
Probably I'm making mountains out of molehills. It should work, so take my r-b
anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
Side note: I'd not split patches 02 and 03: this way it would be simpler to see
the code movement.
+ if (ret == -ENOTSUP) {
+ error_free(local_err);
+ } else if (ret < 0) {
+ error_report_err(local_err);
+ }
+}
+
+
+
three empty lines..
/**
* 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 c9d7c58765..af03022723 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -428,6 +428,7 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs,
BlockDriverState *base,
Error **errp);
void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState
*base);
int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp);
+void coroutine_fn bdrv_co_delete_file_noerr(BlockDriverState *bs);
typedef struct BdrvCheckResult {
--
Best regards,
Vladimir