[Qemu-devel] [PATCH] commit: Add NULL check for overlay_bs

[Qemu-devel] [PATCH] commit: Add NULL check for overlay_bs

[Kevin Wolf]

I can't see how overlay_bs could become NULL with the current code, but
other code in this function already checks it and we can make Coverity
happy with this check, so let's add it.

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

diff --git a/block/commit.c b/block/commit.c
index 774a8a5..50e7950 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -90,7 +90,9 @@ static void commit_complete(BlockJob *job, void *opaque)
 
     /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
     bdrv_ref(top);
-    bdrv_ref(overlay_bs);
+    if (overlay_bs) {
+        bdrv_ref(overlay_bs);
+    }
 
     /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
      * the normal backing chain can be restored. */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] commit: Add NULL check for overlay_bs

[Peter Maydell]

On 10 July 2017 at 12:45, Kevin Wolf <kwolf@redhat.com> wrote:
> I can't see how overlay_bs could become NULL with the current code, but
> other code in this function already checks it and we can make Coverity
> happy with this check, so let's add it.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/commit.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index 774a8a5..50e7950 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -90,7 +90,9 @@ static void commit_complete(BlockJob *job, void *opaque)
>
>      /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
>      bdrv_ref(top);
> -    bdrv_ref(overlay_bs);
> +    if (overlay_bs) {
> +        bdrv_ref(overlay_bs);
> +    }
>
>      /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
>       * the normal backing chain can be restored. */
> --
> 1.8.3.1

Commit 19ebd13ed45ad5d5f2 which added the bdrv_ref() call
was cc qemu-stable; should we cc this fix too, or are you
happy that it's definitely not actually triggerable in stable?

thanks
-- PMM

Re: [Qemu-devel] [Qemu-block] [PATCH] commit: Add NULL check for overlay_bs

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

On Mon, Jul 10, 2017 at 01:45:25PM +0200, Kevin Wolf wrote:
> I can't see how overlay_bs could become NULL with the current code, but
> other code in this function already checks it and we can make Coverity
> happy with this check, so let's add it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/commit.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] commit: Add NULL check for overlay_bs

[Kevin Wolf]

Am 10.07.2017 um 14:09 hat Peter Maydell geschrieben:
> On 10 July 2017 at 12:45, Kevin Wolf <kwolf@redhat.com> wrote:
> > I can't see how overlay_bs could become NULL with the current code, but
> > other code in this function already checks it and we can make Coverity
> > happy with this check, so let's add it.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/commit.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/commit.c b/block/commit.c
> > index 774a8a5..50e7950 100644
> > --- a/block/commit.c
> > +++ b/block/commit.c
> > @@ -90,7 +90,9 @@ static void commit_complete(BlockJob *job, void *opaque)
> >
> >      /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
> >      bdrv_ref(top);
> > -    bdrv_ref(overlay_bs);
> > +    if (overlay_bs) {
> > +        bdrv_ref(overlay_bs);
> > +    }
> >
> >      /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
> >       * the normal backing chain can be restored. */
> 
> Commit 19ebd13ed45ad5d5f2 which added the bdrv_ref() call
> was cc qemu-stable; should we cc this fix too, or are you
> happy that it's definitely not actually triggerable in stable?

Okay, I'll add the CC, better safe than sorry.

Kevin