9pfs: fix dead code in qemu_open_flags_tostr()

[PATCH] 9pfs: fix dead code in qemu_open_flags_tostr()

Posted by Christian Schoenebeck 1 month, 3 weeks ago

Coverity scan complained about expression "|LARGEFILE" to be non reachable
and the detailed Coverity report claims O_LARGEFILE was zero. I can't
reproduce this here, but I assume that means there are at least some
system(s) which define O_LARGEFILE as zero.

This is not really an issue, but to silence this Coverity warning, add a
preprocessor wrapper that checks for O_LARGEFILE being non-zero for this
overall expression. The 'defined(O_LARGEFILE)' check is not necessary,
but it makes it more clear that we really want to check for the value of
O_LARGEFILE, not just whether the macro was defined.

Fixes: 9a0dd4b3
Resolves: Coverity CID 1591178
Reported-by: Coverity Scan
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p-util-generic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/9pfs/9p-util-generic.c b/hw/9pfs/9p-util-generic.c
index 4c1e9c887d..02e359f17b 100644
--- a/hw/9pfs/9p-util-generic.c
+++ b/hw/9pfs/9p-util-generic.c
@@ -19,7 +19,9 @@ char *qemu_open_flags_tostr(int flags)
         #ifdef O_DIRECT
         (flags & O_DIRECT) ? "|DIRECT" : "",
         #endif
+        #if defined(O_LARGEFILE) && O_LARGEFILE != 0
         (flags & O_LARGEFILE) ? "|LARGEFILE" : "",
+        #endif
         (flags & O_DIRECTORY) ? "|DIRECTORY" : "",
         (flags & O_NOFOLLOW) ? "|NOFOLLOW" : "",
         #ifdef O_NOATIME
-- 
2.39.5

Re: [PATCH] 9pfs: fix dead code in qemu_open_flags_tostr()

Posted by Peter Maydell 1 month, 3 weeks ago

On Mon, 10 Feb 2025 at 14:40, Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
>
> Coverity scan complained about expression "|LARGEFILE" to be non reachable
> and the detailed Coverity report claims O_LARGEFILE was zero. I can't
> reproduce this here, but I assume that means there are at least some
> system(s) which define O_LARGEFILE as zero.
>
> This is not really an issue, but to silence this Coverity warning, add a
> preprocessor wrapper that checks for O_LARGEFILE being non-zero for this
> overall expression. The 'defined(O_LARGEFILE)' check is not necessary,
> but it makes it more clear that we really want to check for the value of
> O_LARGEFILE, not just whether the macro was defined.
>
> Fixes: 9a0dd4b3
> Resolves: Coverity CID 1591178
> Reported-by: Coverity Scan
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p-util-generic.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/9pfs/9p-util-generic.c b/hw/9pfs/9p-util-generic.c
> index 4c1e9c887d..02e359f17b 100644
> --- a/hw/9pfs/9p-util-generic.c
> +++ b/hw/9pfs/9p-util-generic.c
> @@ -19,7 +19,9 @@ char *qemu_open_flags_tostr(int flags)
>          #ifdef O_DIRECT
>          (flags & O_DIRECT) ? "|DIRECT" : "",
>          #endif
> +        #if defined(O_LARGEFILE) && O_LARGEFILE != 0
>          (flags & O_LARGEFILE) ? "|LARGEFILE" : "",
> +        #endif
>          (flags & O_DIRECTORY) ? "|DIRECTORY" : "",
>          (flags & O_NOFOLLOW) ? "|NOFOLLOW" : "",
>          #ifdef O_NOATIME

I don't think we need to make this change -- the code is
correct, and osdep.h defines O_LARGEFILE if the system doesn't,
exactly so that we don't need to put in extra ifdefs in the
code itself. Coverity often fails to understand that some
code is not dead in a different configuration or host OS
than the one that got scanned. I've marked the issue as
a false-positive in the Coverity UI.

thanks
-- PMM

Re: [PATCH] 9pfs: fix dead code in qemu_open_flags_tostr()

Posted by Christian Schoenebeck 1 month, 3 weeks ago

On Tuesday, February 11, 2025 3:47:33 PM CET Peter Maydell wrote:
> On Mon, 10 Feb 2025 at 14:40, Christian Schoenebeck
> <qemu_oss@crudebyte.com> wrote:
> >
> > Coverity scan complained about expression "|LARGEFILE" to be non reachable
> > and the detailed Coverity report claims O_LARGEFILE was zero. I can't
> > reproduce this here, but I assume that means there are at least some
> > system(s) which define O_LARGEFILE as zero.
> >
> > This is not really an issue, but to silence this Coverity warning, add a
> > preprocessor wrapper that checks for O_LARGEFILE being non-zero for this
> > overall expression. The 'defined(O_LARGEFILE)' check is not necessary,
> > but it makes it more clear that we really want to check for the value of
> > O_LARGEFILE, not just whether the macro was defined.
> >
> > Fixes: 9a0dd4b3
> > Resolves: Coverity CID 1591178
> > Reported-by: Coverity Scan
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> >  hw/9pfs/9p-util-generic.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/hw/9pfs/9p-util-generic.c b/hw/9pfs/9p-util-generic.c
> > index 4c1e9c887d..02e359f17b 100644
> > --- a/hw/9pfs/9p-util-generic.c
> > +++ b/hw/9pfs/9p-util-generic.c
> > @@ -19,7 +19,9 @@ char *qemu_open_flags_tostr(int flags)
> >          #ifdef O_DIRECT
> >          (flags & O_DIRECT) ? "|DIRECT" : "",
> >          #endif
> > +        #if defined(O_LARGEFILE) && O_LARGEFILE != 0
> >          (flags & O_LARGEFILE) ? "|LARGEFILE" : "",
> > +        #endif
> >          (flags & O_DIRECTORY) ? "|DIRECTORY" : "",
> >          (flags & O_NOFOLLOW) ? "|NOFOLLOW" : "",
> >          #ifdef O_NOATIME
> 
> I don't think we need to make this change -- the code is
> correct, and osdep.h defines O_LARGEFILE if the system doesn't,
> exactly so that we don't need to put in extra ifdefs in the
> code itself. Coverity often fails to understand that some
> code is not dead in a different configuration or host OS
> than the one that got scanned. I've marked the issue as
> a false-positive in the Coverity UI.

Fine with me, thanks!

/Christian

Re: [PATCH] 9pfs: fix dead code in qemu_open_flags_tostr()

Posted by Philippe Mathieu-Daudé 1 month, 3 weeks ago

On 10/2/25 15:33, Christian Schoenebeck wrote:
> Coverity scan complained about expression "|LARGEFILE" to be non reachable
> and the detailed Coverity report claims O_LARGEFILE was zero. I can't
> reproduce this here, but I assume that means there are at least some
> system(s) which define O_LARGEFILE as zero.

Is O_LARGEFILE a Linux-ism?

Commit 67b915a5dd5 ("win32 port (initial patch by kazu)") started to
define it to 0 on 32-bit Windows. It isn't defined on my 64-bit Darwin,
and apparently nor on other BSDs.

> This is not really an issue, but to silence this Coverity warning, add a
> preprocessor wrapper that checks for O_LARGEFILE being non-zero for this
> overall expression. The 'defined(O_LARGEFILE)' check is not necessary,
> but it makes it more clear that we really want to check for the value of
> O_LARGEFILE, not just whether the macro was defined.
> 
> Fixes: 9a0dd4b3
> Resolves: Coverity CID 1591178
> Reported-by: Coverity Scan
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>   hw/9pfs/9p-util-generic.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/9pfs/9p-util-generic.c b/hw/9pfs/9p-util-generic.c
> index 4c1e9c887d..02e359f17b 100644
> --- a/hw/9pfs/9p-util-generic.c
> +++ b/hw/9pfs/9p-util-generic.c
> @@ -19,7 +19,9 @@ char *qemu_open_flags_tostr(int flags)
>           #ifdef O_DIRECT
>           (flags & O_DIRECT) ? "|DIRECT" : "",
>           #endif
> +        #if defined(O_LARGEFILE) && O_LARGEFILE != 0
>           (flags & O_LARGEFILE) ? "|LARGEFILE" : "",
> +        #endif
>           (flags & O_DIRECTORY) ? "|DIRECTORY" : "",
>           (flags & O_NOFOLLOW) ? "|NOFOLLOW" : "",
>           #ifdef O_NOATIME

Re: [PATCH] 9pfs: fix dead code in qemu_open_flags_tostr()

Posted by Christian Schoenebeck 1 month, 3 weeks ago

On Monday, February 10, 2025 4:32:08 PM CET Philippe Mathieu-Daudé wrote:
> On 10/2/25 15:33, Christian Schoenebeck wrote:
> > Coverity scan complained about expression "|LARGEFILE" to be non reachable
> > and the detailed Coverity report claims O_LARGEFILE was zero. I can't
> > reproduce this here, but I assume that means there are at least some
> > system(s) which define O_LARGEFILE as zero.
> 
> Is O_LARGEFILE a Linux-ism?

Ah right, O_LARGEFILE is indeed Linux-specific, not POSIX.

> Commit 67b915a5dd5 ("win32 port (initial patch by kazu)") started to
> define it to 0 on 32-bit Windows. It isn't defined on my 64-bit Darwin,
> and apparently nor on other BSDs.

Yeah, that explains why O_LARGEFILE was defined as zero. I'll adjust the 
commit log message at least on my end. The code change itself is appropriate.

Thanks!

/Christian

> > This is not really an issue, but to silence this Coverity warning, add a
> > preprocessor wrapper that checks for O_LARGEFILE being non-zero for this
> > overall expression. The 'defined(O_LARGEFILE)' check is not necessary,
> > but it makes it more clear that we really want to check for the value of
> > O_LARGEFILE, not just whether the macro was defined.
> > 
> > Fixes: 9a0dd4b3
> > Resolves: Coverity CID 1591178
> > Reported-by: Coverity Scan
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> >   hw/9pfs/9p-util-generic.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/9pfs/9p-util-generic.c b/hw/9pfs/9p-util-generic.c
> > index 4c1e9c887d..02e359f17b 100644
> > --- a/hw/9pfs/9p-util-generic.c
> > +++ b/hw/9pfs/9p-util-generic.c
> > @@ -19,7 +19,9 @@ char *qemu_open_flags_tostr(int flags)
> >           #ifdef O_DIRECT
> >           (flags & O_DIRECT) ? "|DIRECT" : "",
> >           #endif
> > +        #if defined(O_LARGEFILE) && O_LARGEFILE != 0
> >           (flags & O_LARGEFILE) ? "|LARGEFILE" : "",
> > +        #endif
> >           (flags & O_DIRECTORY) ? "|DIRECTORY" : "",
> >           (flags & O_NOFOLLOW) ? "|NOFOLLOW" : "",
> >           #ifdef O_NOATIME
> 
> 
>