Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[Marc-André Lureau]

Hi

On Fri, Mar 29, 2024 at 12:35 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> On 28.03.24 13:20, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > ../block/stream.c:193:19: error: ‘unfiltered_bs’ may be used uninitialized 
> > [-Werror=maybe-uninitialized]
> > ../block/stream.c:176:5: error: ‘len’ may be used uninitialized 
> > [-Werror=maybe-uninitialized]
> > trace/trace-block.h:906:9: error: ‘ret’ may be used uninitialized 
> > [-Werror=maybe-uninitialized]
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Again, same false-positives, because of WITH_GRAPH_RDLOCK_GUARD()..
>
> Didn't you try to change WITH_ macros somehow, so that compiler believe in 
> our good intentions?
>


#define WITH_QEMU_LOCK_GUARD_(x, var) \
    for (g_autoptr(QemuLockable) var = \
                qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
         var; \
         qemu_lockable_auto_unlock(var), var = NULL)

I can't think of a clever way to rewrite this. The compiler probably
thinks the loop may not run, due to the "var" condition. But how to
convince it otherwise? it's hard to introduce another variable too..

> Actually, "unused variable initialization" is bad thing too.
>
> Anyway, if no better solution for now:
> Acked-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> > ---
> >   block/stream.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/stream.c b/block/stream.c
> > index 7031eef12b..9076203193 100644
> > --- a/block/stream.c
> > +++ b/block/stream.c
> > @@ -155,8 +155,8 @@ static void stream_clean(Job *job)
> >   static int coroutine_fn stream_run(Job *job, Error **errp)
> >   {
> >       StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> > -    BlockDriverState *unfiltered_bs;
> > -    int64_t len;
> > +    BlockDriverState *unfiltered_bs = NULL;
> > +    int64_t len = -1;
> >       int64_t offset = 0;
> >       int error = 0;
> >       int64_t n = 0; /* bytes */
> > @@ -177,7 +177,7 @@ static int coroutine_fn stream_run(Job *job, Error 
> > **errp)
> >
> >       for ( ; offset < len; offset += n) {
> >           bool copy;
> > -        int ret;
> > +        int ret = -1;
> >
> >           /* Note that even when no rate limit is applied we need to yield
> >            * with no pending I/O here so that bdrv_drain_all() returns.
>
> --
> Best regards,
> Vladimir
>
>


-- 
Marc-André Lureau