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

[Vladimir Sementsov-Ogievskiy]

On 02.04.24 12:12, Marc-André Lureau wrote:
> 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..


hmm. maybe like this?

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


probably, it would be simpler for compiler to understand the logic this way. 
Could you check?


(actually, will also need to construct var2 name same way as for var)

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

--
Best regards,
Vladimir