Coverity doesn't like that the return value of bdrv_check_update_perm()
stays unused only in this place (CID 1399710).

Even if checking local_err should be equivalent to checking ret < 0,
let's switch to using the return value to be more consistent (and in
case of a bug somewhere down the call chain, forgetting to assign errp
is more likely than returning 0 for an error case).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index ed9253c786..0a93ee9ac8 100644
--- a/block.c
+++ b/block.c
@@ -4350,11 +4350,10 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
     QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
         /* Check whether we are allowed to switch c from top to base */
         GSList *ignore_children = g_slist_prepend(NULL, c);
-        bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
-                               ignore_children, &local_err);
+        ret = bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
+                                     ignore_children, &local_err);
         g_slist_free(ignore_children);
-        if (local_err) {
-            ret = -EPERM;
+        if (ret < 0) {
             error_report_err(local_err);
             goto exit;
         }
-- 
2.20.1

On Fri 15 Mar 2019 12:18:43 PM CET, Kevin Wolf <kwolf@redhat.com> wrote:
> Coverity doesn't like that the return value of bdrv_check_update_perm()
> stays unused only in this place (CID 1399710).
>
> Even if checking local_err should be equivalent to checking ret < 0,
> let's switch to using the return value to be more consistent (and in
> case of a bug somewhere down the call chain, forgetting to assign errp
> is more likely than returning 0 for an error case).
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

On Fri, 15 Mar 2019 at 14:27, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Coverity doesn't like that the return value of bdrv_check_update_perm()
> stays unused only in this place (CID 1399710).
>
> Even if checking local_err should be equivalent to checking ret < 0,
> let's switch to using the return value to be more consistent (and in
> case of a bug somewhere down the call chain, forgetting to assign errp
> is more likely than returning 0 for an error case).
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Kevin Wolf <kwolf@redhat.com> writes:

> Coverity doesn't like that the return value of bdrv_check_update_perm()
> stays unused only in this place (CID 1399710).
>
> Even if checking local_err should be equivalent to checking ret < 0,
> let's switch to using the return value to be more consistent (and in
> case of a bug somewhere down the call chain, forgetting to assign errp
> is more likely than returning 0 for an error case).
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/block.c b/block.c
> index ed9253c786..0a93ee9ac8 100644
> --- a/block.c
> +++ b/block.c
> @@ -4350,11 +4350,10 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
>      QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
>          /* Check whether we are allowed to switch c from top to base */
>          GSList *ignore_children = g_slist_prepend(NULL, c);
> -        bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
> -                               ignore_children, &local_err);
> +        ret = bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
> +                                     ignore_children, &local_err);
>          g_slist_free(ignore_children);
> -        if (local_err) {
> -            ret = -EPERM;
> +        if (ret < 0) {
>              error_report_err(local_err);
>              goto exit;
>          }

Reviewed-by: Markus Armbruster <armbru@redhat.com>