1. Patchew
  2. QEMU
  3. View series

Prevent compiler warning on block.c

Miroslav Rezanina posted 1 patch 2 years, 11 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1162368493.17178530.1620201543649.JavaMail.zimbra@redhat.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>
Prevent compiler warning on block.c
Posted by Miroslav Rezanina 2 years, 11 months ago 
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
Posted by Vladimir Sementsov-Ogievskiy 2 years, 11 months ago 
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
Posted by Thomas Huth 2 years, 10 months ago 
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
Posted by Kevin Wolf 2 years, 10 months ago 
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
Re: Prevent compiler warning on block.c
Posted by Paolo Bonzini 2 years, 11 months ago 
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
Posted by Vladimir Sementsov-Ogievskiy 2 years, 11 months ago 
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
Posted by Peter Maydell 2 years, 11 months ago 
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
Posted by Miroslav Rezanina 2 years, 11 months ago 

----- 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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.