Prevent compiler warning on block.c

Prevent compiler warning on block.c

[Miroslav Rezanina]

Commit 3108a15cf (block: introduce bdrv_drop_filter()) introduced uninitialized
variable to_cow_parent in bdrv_replace_node_common function that is used only when
detach_subchain is true. It is used in two places. First if block properly initialize
the variable and second block use it.

However, compiler treats this two blocks as two independent cases so it thinks first
block can fail test and second one pass (although both use same condition). This cause
warning that variable can be uninitialized in second block.

To prevent this warning, initialize the variable with NULL.

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
---
diff --git a/block.c b/block.c
index 874c22c43e..3ca27bd2d9 100644
--- a/block.c
+++ b/block.c
@@ -4851,7 +4851,7 @@ static int bdrv_replace_node_common(BlockDriverState *from,
     Transaction *tran = tran_new();
     g_autoptr(GHashTable) found = NULL;
     g_autoptr(GSList) refresh_list = NULL;
-    BlockDriverState *to_cow_parent;
+    BlockDriverState *to_cow_parent = NULL;
     int ret;

     if (detach_subchain) {

Re: Prevent compiler warning on block.c

[Vladimir Sementsov-Ogievskiy]

05.05.2021 10:59, Miroslav Rezanina wrote:
> Commit 3108a15cf (block: introduce bdrv_drop_filter()) introduced uninitialized
> variable to_cow_parent in bdrv_replace_node_common function that is used only when
> detach_subchain is true. It is used in two places. First if block properly initialize
> the variable and second block use it.
> 
> However, compiler treats this two blocks as two independent cases so it thinks first
> block can fail test and second one pass (although both use same condition). This cause
> warning that variable can be uninitialized in second block.
> 
> To prevent this warning, initialize the variable with NULL.
> 
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> ---
> diff --git a/block.c b/block.c
> index 874c22c43e..3ca27bd2d9 100644
> --- a/block.c
> +++ b/block.c
> @@ -4851,7 +4851,7 @@ static int bdrv_replace_node_common(BlockDriverState *from,
>       Transaction *tran = tran_new();
>       g_autoptr(GHashTable) found = NULL;
>       g_autoptr(GSList) refresh_list = NULL;
> -    BlockDriverState *to_cow_parent;
> +    BlockDriverState *to_cow_parent = NULL;

May be

+    BlockDriverState *to_cow_parent = NULL; /* Silence compiler warning */

>       int ret;
> 
>       if (detach_subchain) {
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: Prevent compiler warning on block.c

[Paolo Bonzini]

On 05/05/21 10:05, Vladimir Sementsov-Ogievskiy wrote:
>> diff --git a/block.c b/block.c
>> index 874c22c43e..3ca27bd2d9 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4851,7 +4851,7 @@ static int 
>> bdrv_replace_node_common(BlockDriverState *from,
>>       Transaction *tran = tran_new();
>>       g_autoptr(GHashTable) found = NULL;
>>       g_autoptr(GSList) refresh_list = NULL;
>> -    BlockDriverState *to_cow_parent;
>> +    BlockDriverState *to_cow_parent = NULL;
> 
> May be
> 
> +    BlockDriverState *to_cow_parent = NULL; /* Silence compiler warning */


We can also do something like this where the only caller with
to_detach==true takes care of passing the right CoW-parent, and the
for loop goes away completely if I'm not mistaken:

diff --git a/block.c b/block.c
index ae1a7e25aa..3f6fa8475c 100644
--- a/block.c
+++ b/block.c
@@ -4839,31 +4839,19 @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
   * With auto_skip=false the error is returned if from has a parent which should
   * not be updated.
   *
- * With @detach_subchain=true @to must be in a backing chain of @from. In this
- * case backing link of the cow-parent of @to is removed.
+ * With @to_detach is not #NULL its link to @to is removed.
   */
  static int bdrv_replace_node_common(BlockDriverState *from,
                                      BlockDriverState *to,
-                                    bool auto_skip, bool detach_subchain,
+                                    bool auto_skip, BlockDriverState *to_detach,
                                      Error **errp)
  {
      Transaction *tran = tran_new();
      g_autoptr(GHashTable) found = NULL;
      g_autoptr(GSList) refresh_list = NULL;
-    BlockDriverState *to_cow_parent;
+    BlockDriverState *to_detach;
      int ret;
  
-    if (detach_subchain) {
-        assert(bdrv_chain_contains(from, to));
-        assert(from != to);
-        for (to_cow_parent = from;
-             bdrv_filter_or_cow_bs(to_cow_parent) != to;
-             to_cow_parent = bdrv_filter_or_cow_bs(to_cow_parent))
-        {
-            ;
-        }
-    }
-
      /* Make sure that @from doesn't go away until we have successfully attached
       * all of its parents to @to. */
      bdrv_ref(from);
@@ -4883,8 +4871,8 @@ static int bdrv_replace_node_common(BlockDriverState *from,
          goto out;
      }
  
-    if (detach_subchain) {
-        bdrv_remove_filter_or_cow_child(to_cow_parent, tran);
+    if (to_detach) {
+        bdrv_remove_filter_or_cow_child(to_detach, tran);
      }
  
      found = g_hash_table_new(NULL, NULL);
@@ -4911,13 +4899,21 @@ out:
  int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
                        Error **errp)
  {
-    return bdrv_replace_node_common(from, to, true, false, errp);
+    return bdrv_replace_node_common(from, to, true, NULL, errp);
  }
  
  int bdrv_drop_filter(BlockDriverState *bs, Error **errp)
  {
-    return bdrv_replace_node_common(bs, bdrv_filter_or_cow_bs(bs), true, true,
-                                    errp);
+    BlockDriverState *to = bdrv_filter_or_cow_bs(bs);
+
+    assert(bdrv_chain_contains(bs, to));
+    assert(bs != to);
+    return bdrv_replace_node_common(bs, to, true, bs, errp);
  }
  
  /*
@@ -5262,7 +5262,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
       * test-bdrv-drain. test_drop_intermediate_poll() test-case will crash.
       * That's a FIXME.
       */
-    bdrv_replace_node_common(top, base, false, false, &local_err);
+    bdrv_replace_node_common(top, base, false, NULL, &local_err);
      if (local_err) {
          error_report_err(local_err);
          goto exit;

Even nicer would be to move the bdrv_remove_filter_or_cow_child() call to
bdrv_drop_filter, and pass in a Transaction to bdrv_replace_node_common, but
I'm not sure if bdrv_replace_node_noperm and bdrv_remove_filter_or_cow_child
can commute.

Paolo

Re: Prevent compiler warning on block.c

[Vladimir Sementsov-Ogievskiy]

05.05.2021 13:03, Paolo Bonzini wrote:
> On 05/05/21 10:05, Vladimir Sementsov-Ogievskiy wrote:
>>> diff --git a/block.c b/block.c
>>> index 874c22c43e..3ca27bd2d9 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -4851,7 +4851,7 @@ static int bdrv_replace_node_common(BlockDriverState *from,
>>>       Transaction *tran = tran_new();
>>>       g_autoptr(GHashTable) found = NULL;
>>>       g_autoptr(GSList) refresh_list = NULL;
>>> -    BlockDriverState *to_cow_parent;
>>> +    BlockDriverState *to_cow_parent = NULL;
>>
>> May be
>>
>> +    BlockDriverState *to_cow_parent = NULL; /* Silence compiler warning */
> 
> 
> We can also do something like this where the only caller with
> to_detach==true takes care of passing the right CoW-parent, and the
> for loop goes away completely if I'm not mistaken:
> 
> diff --git a/block.c b/block.c
> index ae1a7e25aa..3f6fa8475c 100644
> --- a/block.c
> +++ b/block.c
> @@ -4839,31 +4839,19 @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
>   * With auto_skip=false the error is returned if from has a parent which should
>   * not be updated.
>   *
> - * With @detach_subchain=true @to must be in a backing chain of @from. In this
> - * case backing link of the cow-parent of @to is removed.
> + * With @to_detach is not #NULL its link to @to is removed.
>   */
> static int bdrv_replace_node_common(BlockDriverState *from,
>                                      BlockDriverState *to,
> -                                    bool auto_skip, bool detach_subchain,
> +                                    bool auto_skip, BlockDriverState *to_detach,
>                                      Error **errp)
> {
>      Transaction *tran = tran_new();
>      g_autoptr(GHashTable) found = NULL;
>      g_autoptr(GSList) refresh_list = NULL;
> -    BlockDriverState *to_cow_parent;
> +    BlockDriverState *to_detach;
>      int ret;
> 
> -    if (detach_subchain) {
> -        assert(bdrv_chain_contains(from, to));
> -        assert(from != to);
> -        for (to_cow_parent = from;
> -             bdrv_filter_or_cow_bs(to_cow_parent) != to;
> -             to_cow_parent = bdrv_filter_or_cow_bs(to_cow_parent))
> -        {
> -            ;
> -        }
> -    }
> -
>      /* Make sure that @from doesn't go away until we have successfully attached
>       * all of its parents to @to. */
>      bdrv_ref(from);
> @@ -4883,8 +4871,8 @@ static int bdrv_replace_node_common(BlockDriverState *from,
>          goto out;
>      }
> 
> -    if (detach_subchain) {
> -        bdrv_remove_filter_or_cow_child(to_cow_parent, tran);
> +    if (to_detach) {
> +        bdrv_remove_filter_or_cow_child(to_detach, tran);
>      }
> 
>      found = g_hash_table_new(NULL, NULL);
> @@ -4911,13 +4899,21 @@ out:
> int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
>                        Error **errp)
> {
> -    return bdrv_replace_node_common(from, to, true, false, errp);
> +    return bdrv_replace_node_common(from, to, true, NULL, errp);
> }
> 
> int bdrv_drop_filter(BlockDriverState *bs, Error **errp)
> {
> -    return bdrv_replace_node_common(bs, bdrv_filter_or_cow_bs(bs), true, true,
> -                                    errp);
> +    BlockDriverState *to = bdrv_filter_or_cow_bs(bs);
> +
> +    assert(bdrv_chain_contains(bs, to));
> +    assert(bs != to);
> +    return bdrv_replace_node_common(bs, to, true, bs, errp);
> }
> 
> /*
> @@ -5262,7 +5262,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
>       * test-bdrv-drain. test_drop_intermediate_poll() test-case will crash.
>       * That's a FIXME.
>       */

I'd prefer first fix this FIXME. Then, one more caller with detach_subchain=true will appear, and we'll see, how to refactor the interface in the best way. Otherwise we'll just have to refactor it twice.


> -    bdrv_replace_node_common(top, base, false, false, &local_err);
> +    bdrv_replace_node_common(top, base, false, NULL, &local_err);
>      if (local_err) {
>          error_report_err(local_err);
>          goto exit;
> 
> Even nicer would be to move the bdrv_remove_filter_or_cow_child() call to
> bdrv_drop_filter, and pass in a Transaction to bdrv_replace_node_common, but
> I'm not sure if bdrv_replace_node_noperm and bdrv_remove_filter_or_cow_child
> can commute.
> 


-- 
Best regards,
Vladimir

Re: Prevent compiler warning on block.c

[Peter Maydell]

On Wed, 5 May 2021 at 09:06, Miroslav Rezanina <mrezanin@redhat.com> wrote:
>
> Commit 3108a15cf (block: introduce bdrv_drop_filter()) introduced uninitialized
> variable to_cow_parent in bdrv_replace_node_common function that is used only when
> detach_subchain is true. It is used in two places. First if block properly initialize
> the variable and second block use it.
>
> However, compiler treats this two blocks as two independent cases so it thinks first
> block can fail test and second one pass (although both use same condition). This cause
> warning that variable can be uninitialized in second block.
>
> To prevent this warning, initialize the variable with NULL.

If fixing compiler warnings, please quote the compiler name/version
in the commit message. (This helps with understanding whether the issue
is because of an older and not-smart-enough compiler or a new bleeding-edge
compiler with extra checking.)

thanks
-- PMM

Re: Prevent compiler warning on block.c

[Miroslav Rezanina]

----- Original Message -----
> From: "Peter Maydell" <peter.maydell@linaro.org>
> To: "Miroslav Rezanina" <mrezanin@redhat.com>
> Cc: "QEMU Developers" <qemu-devel@nongnu.org>, "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>,
> "Qemu-block" <qemu-block@nongnu.org>
> Sent: Wednesday, May 5, 2021 12:43:44 PM
> Subject: Re: Prevent compiler warning on block.c
> 
> On Wed, 5 May 2021 at 09:06, Miroslav Rezanina <mrezanin@redhat.com> wrote:
> >
> > Commit 3108a15cf (block: introduce bdrv_drop_filter()) introduced
> > uninitialized
> > variable to_cow_parent in bdrv_replace_node_common function that is used
> > only when
> > detach_subchain is true. It is used in two places. First if block properly
> > initialize
> > the variable and second block use it.
> >
> > However, compiler treats this two blocks as two independent cases so it
> > thinks first
> > block can fail test and second one pass (although both use same condition).
> > This cause
> > warning that variable can be uninitialized in second block.
> >
> > To prevent this warning, initialize the variable with NULL.
> 
> If fixing compiler warnings, please quote the compiler name/version
> in the commit message. (This helps with understanding whether the issue
> is because of an older and not-smart-enough compiler or a new bleeding-edge
> compiler with extra checking.)

Hi Peter,

sorry for missing version. I was going to put the version in but got distracted when checking versions.

This warning occurs using GCC 8.4.1 and 11.0.1.

Mirek

> 
> thanks
> -- PMM
> 
> 

-- 
Miroslav Rezanina
Software Engineer - Virtualization Team Maintainer

Re: Prevent compiler warning on block.c

[Thomas Huth]

On 05/05/2021 10.05, Vladimir Sementsov-Ogievskiy wrote:
> 05.05.2021 10:59, Miroslav Rezanina wrote:
>> Commit 3108a15cf (block: introduce bdrv_drop_filter()) introduced 
>> uninitialized
>> variable to_cow_parent in bdrv_replace_node_common function that is used 
>> only when
>> detach_subchain is true. It is used in two places. First if block properly 
>> initialize
>> the variable and second block use it.
>>
>> However, compiler treats this two blocks as two independent cases so it 
>> thinks first
>> block can fail test and second one pass (although both use same 
>> condition). This cause
>> warning that variable can be uninitialized in second block.
>>
>> To prevent this warning, initialize the variable with NULL.
>>
>> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
>> ---
>> diff --git a/block.c b/block.c
>> index 874c22c43e..3ca27bd2d9 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4851,7 +4851,7 @@ static int bdrv_replace_node_common(BlockDriverState 
>> *from,
>>       Transaction *tran = tran_new();
>>       g_autoptr(GHashTable) found = NULL;
>>       g_autoptr(GSList) refresh_list = NULL;
>> -    BlockDriverState *to_cow_parent;
>> +    BlockDriverState *to_cow_parent = NULL;
>>       int ret;
>>
>>       if (detach_subchain) {
>>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

This just popped up again here:

  https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg02067.html

Kevin, Max, could you please pick it up to get this problem fixed?

  Thomas

Re: Prevent compiler warning on block.c

[Kevin Wolf]

Am 09.06.2021 um 09:12 hat Thomas Huth geschrieben:
> On 05/05/2021 10.05, Vladimir Sementsov-Ogievskiy wrote:
> > 05.05.2021 10:59, Miroslav Rezanina wrote:
> > > Commit 3108a15cf (block: introduce bdrv_drop_filter()) introduced
> > > uninitialized
> > > variable to_cow_parent in bdrv_replace_node_common function that is
> > > used only when
> > > detach_subchain is true. It is used in two places. First if block
> > > properly initialize
> > > the variable and second block use it.
> > > 
> > > However, compiler treats this two blocks as two independent cases so
> > > it thinks first
> > > block can fail test and second one pass (although both use same
> > > condition). This cause
> > > warning that variable can be uninitialized in second block.
> > > 
> > > To prevent this warning, initialize the variable with NULL.
> > > 
> > > Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> > > ---
> > > diff --git a/block.c b/block.c
> > > index 874c22c43e..3ca27bd2d9 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -4851,7 +4851,7 @@ static int
> > > bdrv_replace_node_common(BlockDriverState *from,
> > >       Transaction *tran = tran_new();
> > >       g_autoptr(GHashTable) found = NULL;
> > >       g_autoptr(GSList) refresh_list = NULL;
> > > -    BlockDriverState *to_cow_parent;
> > > +    BlockDriverState *to_cow_parent = NULL;
> > >       int ret;
> > > 
> > >       if (detach_subchain) {
> > > 
> > 
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> This just popped up again here:
> 
>  https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg02067.html
> 
> Kevin, Max, could you please pick it up to get this problem fixed?

Thanks for pinging, seems the intended refactoring hasn't happened.

I've applied this one to my block branch now.

Kevin