| Message ID | E1thUwq-0020ux-5f@kylie.crudebyte.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | 9pfs: fix dead code in qemu_open_flags_tostr() | expand |
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
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 > > >
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
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
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
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(+)