| Message ID | 20170405194741.18956-11-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 05.04.2017 um 21:47 hat Eric Blake geschrieben: > Noticed while checking Coccinelle results. Naming a label 'out:' > when it is only used on error paths is weird; meanwhile we know > that snapshot_options is NULL on success and that QDECREF(NULL) > is safe. So merge the two exit paths into one. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > block.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/block.c b/block.c > index 9b87bf6..a13625f 100644 > --- a/block.c > +++ b/block.c > @@ -2209,13 +2209,10 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, > goto out; > } > > +out: > + QDECREF(snapshot_options); > g_free(tmp_filename); > return bs_snapshot; bs_snapshot is uninitialised or at least the wrong return value in error cases. (Hm... Shouldn't the compiler catch the uninitialised part?) > - > -out: > - QDECREF(snapshot_options); > - g_free(tmp_filename); > - return NULL; > } Kevin
On 04/06/2017 04:00 AM, Kevin Wolf wrote: > Am 05.04.2017 um 21:47 hat Eric Blake geschrieben: >> Noticed while checking Coccinelle results. Naming a label 'out:' >> when it is only used on error paths is weird; meanwhile we know >> that snapshot_options is NULL on success and that QDECREF(NULL) >> is safe. So merge the two exit paths into one. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> block.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/block.c b/block.c >> index 9b87bf6..a13625f 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2209,13 +2209,10 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, >> goto out; >> } >> >> +out: >> + QDECREF(snapshot_options); >> g_free(tmp_filename); >> return bs_snapshot; > > bs_snapshot is uninitialised or at least the wrong return value in error > cases. (Hm... Shouldn't the compiler catch the uninitialised part?) Odd, and I agree that I recall the compiler generally able to catch that (maybe it's a matter of compiling with -g vs. -O2). I'm surprised the autobuilder didn't flag it. (I think I missed it due to rebase churn on my end). The obvious fix is to: - BlockDriverState *bs_snapshot; + BlockDriverState *bs_snapshot = NULL;
On 04/06/2017 07:52 AM, Eric Blake wrote: >>> +out: >>> + QDECREF(snapshot_options); >>> g_free(tmp_filename); >>> return bs_snapshot; >> >> bs_snapshot is uninitialised or at least the wrong return value in error >> cases. (Hm... Shouldn't the compiler catch the uninitialised part?) > > Odd, and I agree that I recall the compiler generally able to catch that > (maybe it's a matter of compiling with -g vs. -O2). I'm surprised the > autobuilder didn't flag it. And now the autobuilder finally replies. It is indeed a matter of difference in CFLAGS on whether the compiler will catch it.
Am 06.04.2017 um 14:52 hat Eric Blake geschrieben: > On 04/06/2017 04:00 AM, Kevin Wolf wrote: > > Am 05.04.2017 um 21:47 hat Eric Blake geschrieben: > >> Noticed while checking Coccinelle results. Naming a label 'out:' > >> when it is only used on error paths is weird; meanwhile we know > >> that snapshot_options is NULL on success and that QDECREF(NULL) > >> is safe. So merge the two exit paths into one. > >> > >> Signed-off-by: Eric Blake <eblake@redhat.com> > >> --- > >> block.c | 7 ++----- > >> 1 file changed, 2 insertions(+), 5 deletions(-) > >> > >> diff --git a/block.c b/block.c > >> index 9b87bf6..a13625f 100644 > >> --- a/block.c > >> +++ b/block.c > >> @@ -2209,13 +2209,10 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, > >> goto out; > >> } > >> > >> +out: > >> + QDECREF(snapshot_options); > >> g_free(tmp_filename); > >> return bs_snapshot; > > > > bs_snapshot is uninitialised or at least the wrong return value in error > > cases. (Hm... Shouldn't the compiler catch the uninitialised part?) > > Odd, and I agree that I recall the compiler generally able to catch that > (maybe it's a matter of compiling with -g vs. -O2). I'm surprised the > autobuilder didn't flag it. (I think I missed it due to rebase churn on > my end). The obvious fix is to: > > - BlockDriverState *bs_snapshot; > + BlockDriverState *bs_snapshot = NULL; That's just half of the fix. It doesn't fix the cases where bs_snapshot is already set. We still want to return NULL on errors there. Kevin
On 04/06/2017 08:27 AM, Kevin Wolf wrote: >>>> @@ -2209,13 +2209,10 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, More context: > bs_snapshot = bdrv_open(NULL, NULL, snapshot_options, flags, errp); > snapshot_options = NULL; > if (!bs_snapshot) { > ret = -EINVAL; > goto out; > } Up to here, bs_snapshot is NULL if we return. What is the deal with ret? Setting it to -EINVAL has no purpose, as we don't reference ret in the out label. > > /* bdrv_append() consumes a strong reference to bs_snapshot (i.e. it will > * call bdrv_unref() on it), so in order to be able to return one, we have > * to increase bs_snapshot's refcount here */ > bdrv_ref(bs_snapshot); > bdrv_append(bs_snapshot, bs, &local_err); > if (local_err) { > error_propagate(errp, local_err); > ret = -EINVAL; > goto out; > } Again, what's with setting ret? But you are right that this is a place where bs_snapshot would be non-null when we want to return an error. The goto out looks a bit funny with no further code (although it's okay to leave it as a defensive measure for code added later, as that's what bdrv_append() does). >>>> >>>> +out: >>>> + QDECREF(snapshot_options); >>>> g_free(tmp_filename); >>>> return bs_snapshot; >>> >>> bs_snapshot is uninitialised or at least the wrong return value in error >>> cases. (Hm... Shouldn't the compiler catch the uninitialised part?) >> >> Odd, and I agree that I recall the compiler generally able to catch that >> (maybe it's a matter of compiling with -g vs. -O2). I'm surprised the >> autobuilder didn't flag it. (I think I missed it due to rebase churn on >> my end). The obvious fix is to: >> >> - BlockDriverState *bs_snapshot; >> + BlockDriverState *bs_snapshot = NULL; > > That's just half of the fix. It doesn't fix the cases where bs_snapshot > is already set. We still want to return NULL on errors there. Thanks for calling it to my attention.
diff --git a/block.c b/block.c index 9b87bf6..a13625f 100644 --- a/block.c +++ b/block.c @@ -2209,13 +2209,10 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, goto out; } +out: + QDECREF(snapshot_options); g_free(tmp_filename); return bs_snapshot; - -out: - QDECREF(snapshot_options); - g_free(tmp_filename); - return NULL; } /*
Noticed while checking Coccinelle results. Naming a label 'out:' when it is only used on error paths is weird; meanwhile we know that snapshot_options is NULL on success and that QDECREF(NULL) is safe. So merge the two exit paths into one. Signed-off-by: Eric Blake <eblake@redhat.com> --- block.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)