[PULL 01/24] Prevent compiler warning on block.c
From: 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 may treat these 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.
The warning was observed with GCC 8.4.1 and 11.0.1.
To prevent this warning, initialize the variable with NULL.
Signed-off-by: Miroslav Rezanina
Message-Id: <1162368493.17178530.1620201543649.javamail.zim...@redhat.com>
Signed-off-by: Kevin Wolf
---
block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 1d37f133a8..3e277855e7 100644
--- a/block.c
+++ b/block.c
@@ -4866,7 +4866,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) {
--
2.31.1
Re: Prevent compiler warning on block.c
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
> > > ---
> > > 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
>
> 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
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
---
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
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
- Original Message - > From: "Peter Maydell" > To: "Miroslav Rezanina" > Cc: "QEMU Developers" , "Vladimir Sementsov-Ogievskiy" > , > "Qemu-block" > 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 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
On Wed, 5 May 2021 at 09:06, 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. 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
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, _err);
+ bdrv_replace_node_common(top, base, false, NULL, _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
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, _err);
+bdrv_replace_node_common(top, base, false, NULL, _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
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
---
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
--
Best regards,
Vladimir
Prevent compiler warning on block.c
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
---
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) {
