block.c: Add return value for bdrv_append_temp_snapshot() to avoid incorrect failure processing issue

When failure occurs, 'ret' need be set, or may return 0 to indicate success.

And error_propagate() also need be called only one time within a function.

It is abnormal to prevent bdrv_append_temp_snapshot() return value but still set errp when error occurs -- although it contents return value internally.

So let bdrv_append_temp_snapshot() internal return value outside, and let all things normal, then fix the issue too.

Signed-off-by Chen Gang <[email protected]>

---
 block.c               | 7 ++++---
 include/block/block.h | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

block.c +4 -3

index 74af8d7..5e4fb60 100644
@@ -1240,7 +1240,7 @@ done:
     return ret;
 }
 
-void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
+int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
 {
     /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
     char *tmp_filename = g_malloc0(PATH_MAX + 1);

@@ -1258,6 +1258,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
     /* Get the required size from the image */
     total_size = bdrv_getlength(bs);
     if (total_size < 0) {
+        ret = total_size;
         error_setg_errno(errp, -total_size, "Could not get image size");
         goto out;
     }

@@ -1304,6 +1305,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
 
 out:
     g_free(tmp_filename);
+    return ret;
 }
 
 static QDict *parse_json_filename(const char *filename, Error **errp)

@@ -1495,9 +1497,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
      * temporary snapshot afterwards. */
     if (snapshot_flags) {
-        bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
+        ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
         if (local_err) {
-            error_propagate(errp, local_err);
             goto close_and_fail;
         }
     }

include/block/block.h +0 -0

index f15b99b..7b3381c 100644

include/block/block.h +0 -0

 int bdrv_open(BlockDriverState **pbs, const char *filename,
               const char *reference, QDict *options, int flags,
               BlockDriver *drv, Error **errp);

Signature · 1 line

1.7.11.7

Quoted 1 line · Chen Gang

When failure occurs, 'ret' need be set, or may return 0 to indicate success.

s/need/needs/

Quoted 1 line

And error_propagate() also need be called only one time within a function.

s/need/needs/

Quoted 3 lines

It is abnormal to prevent bdrv_append_temp_snapshot() return value but still set errp when error occurs -- although it contents return value internally.

s/contents/has/

Quoted 55 lines

So let bdrv_append_temp_snapshot() internal return value outside, and let all things normal, then fix the issue too.

Signed-off-by Chen Gang <[email protected]>

---
 block.c               | 7 ++++---
 include/block/block.h | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

block.c +4 -3

index 74af8d7..5e4fb60 100644
@@ -1240,7 +1240,7 @@ done:
     return ret;
 }
 
-void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
+int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
 {
     /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
     char *tmp_filename = g_malloc0(PATH_MAX + 1);

@@ -1258,6 +1258,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
     /* Get the required size from the image */
     total_size = bdrv_getlength(bs);
     if (total_size < 0) {
+        ret = total_size;
         error_setg_errno(errp, -total_size, "Could not get image size");
         goto out;
     }

@@ -1304,6 +1305,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
 
 out:
     g_free(tmp_filename);
+    return ret;
 }
 
 static QDict *parse_json_filename(const char *filename, Error **errp)

@@ -1495,9 +1497,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
      * temporary snapshot afterwards. */
     if (snapshot_flags) {
-        bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
+        ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
         if (local_err) {
-            error_propagate(errp, local_err);
             goto close_and_fail;
         }
     }

include/block/block.h +0 -0

index f15b99b..7b3381c 100644

include/block/block.h int bdrv_open_image(BlockDriverState **pbs, const char *filename, · line 220

Quoted 6 lines

include/block/block.h +1 -1

@@ -217,7 +217,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
                     bool allow_none, Error **errp);
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
-void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);
+int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);

Not used outside block.c, but pre-existing.

Reviewed-by Fam Zheng <[email protected]>

When failure occurs, 'ret' need be set, or may return 0 to indicate success. And error_propagate() also need be called only one time within a function.

It is abnormal to prevent bdrv_append_temp_snapshot() return value but still set errp when error occurs -- although it contents return value internally.

So let bdrv_append_temp_snapshot() internal return value outside, and let all things normal, then fix the issue too.

Signed-off-by Chen Gang <[email protected]>

Am 23.06.2014 um 17:28 hat Chen Gang geschrieben:

Quoted 6 lines

When failure occurs, 'ret' need be set, or may return 0 to indicate success. And error_propagate() also need be called only one time within a function.

It is abnormal to prevent bdrv_append_temp_snapshot() return value but still set errp when error occurs -- although it contents return value internally.

Quoted 4 lines

So let bdrv_append_temp_snapshot() internal return value outside, and let all things normal, then fix the issue too.

Signed-off-by Chen Gang <[email protected]>

What does this fix?

Having both a return value and an Error* object is duplication and only a sign that a function hasn't been fully converted to the Error framework yet. We shouldn't introduce new instances of this without a very good reason.

Quoted 10 lines · Kevin Wolf

Am 23.06.2014 um 17:28 hat Chen Gang geschrieben:

Quoted 9 lines

When failure occurs, 'ret' need be set, or may return 0 to indicate success. And error_propagate() also need be called only one time within a function.

It is abnormal to prevent bdrv_append_temp_snapshot() return value but still set errp when error occurs -- although it contents return value internally.

So let bdrv_append_temp_snapshot() internal return value outside, and let all things normal, then fix the issue too.

Quoted 3 lines

Quoted 1 line

Signed-off-by Chen Gang <[email protected]>

What does this fix?

It fixes the return value of bdrv_open() when bdrv_append_temp_snapshot() fails. Before this patch, it returns a positive value, which is wrong. After the patch, it returns the negative error code bdrv_append_temp_snapshot() now returns.

Quoted 4 lines

Having both a return value and an Error* object is duplication and only a sign that a function hasn't been fully converted to the Error framework yet. We shouldn't introduce new instances of this without a very good reason.

Maybe. But I very much prefer

        ret = foo(arg, errp);
        if (ret < 0) {
            return ret;
        }

over

        Error *local_err = NULL;

        foo(arg, &local_err);
        if (local_err) {
            error_propagate(errp, local_err);
            return;
        }

Quoted 12 lines · Markus Armbruster

Quoted 10 lines · Kevin Wolf

Am 23.06.2014 um 17:28 hat Chen Gang geschrieben:

Quoted 9 lines

When failure occurs, 'ret' need be set, or may return 0 to indicate success. And error_propagate() also need be called only one time within a function.

It is abnormal to prevent bdrv_append_temp_snapshot() return value but still set errp when error occurs -- although it contents return value internally.

So let bdrv_append_temp_snapshot() internal return value outside, and let all things normal, then fix the issue too.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Signed-off-by Chen Gang <[email protected]>

What does this fix?

It fixes the return value of bdrv_open() when bdrv_append_temp_snapshot() fails. Before this patch, it returns a positive value, which is wrong. After the patch, it returns the negative error code bdrv_append_temp_snapshot() now returns.

So, what should be done there? Kevin, maybe you should pick this up
instead of going -trivial route?

Quoted 14 lines

Quoted 4 lines

Having both a return value and an Error* object is duplication and only a sign that a function hasn't been fully converted to the Error framework yet. We shouldn't introduce new instances of this without a very good reason.

Maybe. But I very much prefer

        ret = foo(arg, errp);
        if (ret < 0) {
            return ret;
        }

over

Quoted 7 lines

        Error *local_err = NULL;

        foo(arg, &local_err);
        if (local_err) {
            error_propagate(errp, local_err);
            return;
        }

Yes, this new error propagation is a bit ugly, I dislike it too.

Thanks,

/mjt

Quoted 10 lines · Kevin Wolf

Am 23.06.2014 um 17:28 hat Chen Gang geschrieben:

Quoted 9 lines

When failure occurs, 'ret' need be set, or may return 0 to indicate success. And error_propagate() also need be called only one time within a function.

It is abnormal to prevent bdrv_append_temp_snapshot() return value but still set errp when error occurs -- although it contents return value internally.

So let bdrv_append_temp_snapshot() internal return value outside, and let all things normal, then fix the issue too.

Quoted 3 lines

Quoted 1 line

Signed-off-by Chen Gang <[email protected]>

What does this fix?

It fixes the return value of bdrv_open() when bdrv_append_temp_snapshot() fails. Before this patch, it returns a positive value, which is wrong. After the patch, it returns the negative error code bdrv_append_temp_snapshot() now returns.

When failure occurs, 'ret' need be set, or may return 0 to indicate success. And error_propagate() also need be called only one time within a function.

It is abnormal to prevent bdrv_append_temp_snapshot() return value but still set errp when error occurs -- although it contents return value internally.

So let bdrv_append_temp_snapshot() internal return value outside, and let all things normal, then fix the issue too.

Signed-off-by Chen Gang <[email protected]>