I'm building the latest kernel with GCC 15 and got this build failure
fs/qnx6/inode.c: In function ‘qnx6_checkroot’:
fs/qnx6/inode.c:182:41: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
182 | static char match_root[2][3] = {".\0\0", "..\0"};
| ^~~~~~~
fs/qnx6/inode.c:182:50: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
182 | static char match_root[2][3] = {".\0\0", "..\0"};
| ^~~~~~
This is due to GCC 15 now enables
-Werror=unterminated-string-initialization by default.
This is not the only error, there are many such build failures in
various subsystems, some of theme are easy to fix, while some are not.
In this case I was thinking of something like:
--- a/fs/qnx6/inode.c
+++ b/fs/qnx6/inode.c
@@ -179,7 +179,7 @@ static int qnx6_statfs(struct dentry *dentry, struct kstatfs *buf)
*/
static const char *qnx6_checkroot(struct super_block *s)
{
- static char match_root[2][3] = {".\0\0", "..\0"};
+ static char *match_root[][3] = {".\0\0", "..\0"};
int i, error = 0;
struct qnx6_dir_entry *dir_entry;
struct inode *root = d_inode(s->s_root);
I'm not sure if this is the right implementation or not, but I'm always
open to better ideas (or just increasing the dimension of match_root to
[3][4]) and would love to send in a patch.
--
Regards,
Brahmajit
On Thu, Oct 03, 2024 at 02:46:48AM +0530, Brahmajit wrote:
> I'm building the latest kernel with GCC 15 and got this build failure
>
> fs/qnx6/inode.c: In function ‘qnx6_checkroot’:
> fs/qnx6/inode.c:182:41: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
> 182 | static char match_root[2][3] = {".\0\0", "..\0"};
> | ^~~~~~~
> fs/qnx6/inode.c:182:50: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
> 182 | static char match_root[2][3] = {".\0\0", "..\0"};
> | ^~~~~~
>
> This is due to GCC 15 now enables
> -Werror=unterminated-string-initialization by default.
> This is not the only error, there are many such build failures in
> various subsystems, some of theme are easy to fix, while some are not.
> In this case I was thinking of something like:
>
> --- a/fs/qnx6/inode.c
> +++ b/fs/qnx6/inode.c
> @@ -179,7 +179,7 @@ static int qnx6_statfs(struct dentry *dentry, struct kstatfs *buf)
> */
> static const char *qnx6_checkroot(struct super_block *s)
> {
> - static char match_root[2][3] = {".\0\0", "..\0"};
> + static char *match_root[][3] = {".\0\0", "..\0"};
Huh? That makes no sense whatsoever - you get a single-element
array of 3-element arrays of pointers to char.
What you have written is equivalent to
static char *s1 = ".\0\0";
static char *s2 = "..\0";
static char *match_root[1][3] = {[0][0] = s1, [0][1] = s2, [0][2] = NULL};
and match_root[0] is *NOT* a pointer to char anymore.
Just lose the last \0 in each of those string literals...
On Wed, Oct 02, 2024 at 10:46:56PM +0100, Al Viro wrote:
> Huh? That makes no sense whatsoever - you get a single-element
> array of 3-element arrays of pointers to char.
>
> What you have written is equivalent to
> static char *s1 = ".\0\0";
> static char *s2 = "..\0";
> static char *match_root[1][3] = {[0][0] = s1, [0][1] = s2, [0][2] = NULL};
>
> and match_root[0] is *NOT* a pointer to char anymore.
>
> Just lose the last \0 in each of those string literals...
... and looking at the actual code using that, just lose the entire
array -
if (memcmp(dir_entry[0].de_fname, ".", 2) ||
memcmp(dir_entry[1].de_fname, "..", 3))
error = 1;
and be done with that.
On 02.10.2024 22:54, Al Viro wrote:
> ... and looking at the actual code using that, just lose the entire
> array -
> if (memcmp(dir_entry[0].de_fname, ".", 2) ||
> memcmp(dir_entry[1].de_fname, "..", 3))
> error = 1;
> and be done with that.
Hey sorry for the initial bad code, I'm just starting out. With your
recommendation I wrote this, does this look good?
--- a/fs/qnx6/inode.c
+++ b/fs/qnx6/inode.c
@@ -179,8 +179,7 @@ static int qnx6_statfs(struct dentry *dentry, struct kstatfs *buf)
*/
static const char *qnx6_checkroot(struct super_block *s)
{
- static char match_root[2][3] = {".\0\0", "..\0"};
- int i, error = 0;
+ int error = 0;
struct qnx6_dir_entry *dir_entry;
struct inode *root = d_inode(s->s_root);
struct address_space *mapping = root->i_mapping;
@@ -189,11 +188,9 @@ static const char *qnx6_checkroot(struct super_block *s)
if (IS_ERR(folio))
return "error reading root directory";
dir_entry = kmap_local_folio(folio, 0);
- for (i = 0; i < 2; i++) {
- /* maximum 3 bytes - due to match_root limitation */
- if (strncmp(dir_entry[i].de_fname, match_root[i], 3))
- error = 1;
- }
+ if (memcmp(dir_entry[0].de_fname, ".", 2) ||
+ memcmp(dir_entry[1].de_fname, "..", 3))
+ error = 1;
folio_release_kmap(folio, dir_entry);
if (error)
return "error reading root directory.";
--
Regards,
listout
GCC 15 enables -Werror=unterminated-string-initialization by default.
This results in the following build error
fs/qnx6/inode.c: In function ‘qnx6_checkroot’:
fs/qnx6/inode.c:182:41: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
182 | static char match_root[2][3] = {".\0\0", "..\0"};
| ^~~~~~~
fs/qnx6/inode.c:182:50: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
182 | static char match_root[2][3] = {".\0\0", "..\0"};
| ^~~~~~
Dropping to match_root array and drictly comparing dir_entry entries via
memcmp provides a work aroud for the build error.
Signed-off-by: Brahmajit Das <brahmajit.xyz@gmail.com>
---
fs/qnx6/inode.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/fs/qnx6/inode.c b/fs/qnx6/inode.c
index 85925ec0051a..3310d1ad4d0e 100644
--- a/fs/qnx6/inode.c
+++ b/fs/qnx6/inode.c
@@ -179,8 +179,7 @@ static int qnx6_statfs(struct dentry *dentry, struct kstatfs *buf)
*/
static const char *qnx6_checkroot(struct super_block *s)
{
- static char match_root[2][3] = {".\0\0", "..\0"};
- int i, error = 0;
+ int error = 0;
struct qnx6_dir_entry *dir_entry;
struct inode *root = d_inode(s->s_root);
struct address_space *mapping = root->i_mapping;
@@ -189,11 +188,9 @@ static const char *qnx6_checkroot(struct super_block *s)
if (IS_ERR(folio))
return "error reading root directory";
dir_entry = kmap_local_folio(folio, 0);
- for (i = 0; i < 2; i++) {
- /* maximum 3 bytes - due to match_root limitation */
- if (strncmp(dir_entry[i].de_fname, match_root[i], 3))
- error = 1;
- }
+ if (memcmp(dir_entry[0].de_fname, ".", 2) ||
+ memcmp(dir_entry[1].de_fname, "..", 3))
+ error = 1;
folio_release_kmap(folio, dir_entry);
if (error)
return "error reading root directory.";
--
2.46.2
On Fri, Oct 04, 2024 at 03:19:21PM +0530, Brahmajit Das wrote:
> GCC 15 enables -Werror=unterminated-string-initialization by default.
> This results in the following build error
>
> fs/qnx6/inode.c: In function ‘qnx6_checkroot’:
> fs/qnx6/inode.c:182:41: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
> 182 | static char match_root[2][3] = {".\0\0", "..\0"};
> | ^~~~~~~
> fs/qnx6/inode.c:182:50: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
> 182 | static char match_root[2][3] = {".\0\0", "..\0"};
> | ^~~~~~
>
> Dropping to match_root array and drictly comparing dir_entry entries via
> memcmp provides a work aroud for the build error.
LGTM, except that I'd probably make the commit message less warning-centric -
something like
qnx6_checkroot() had been using weirdly spelled initializer - it needed
to initialize 3-element arrays of char and it used NUL-padded 3-character
string literals (i.e. 4-element initializers, with completely pointless
zeroes at the end).
That had been spotted by gcc-15[*]; prior to that gcc quietly dropped
the 4th element of initializers.
However, none of that had been needed in the first place - all this array
is used for is checking that the first directory entry in root directory
is "." and the second - "..". The check had been expressed as a loop,
using that match_root[] array. Since there is no chance that we ever
want to extend that list of entries, the entire thing is much too fancy
for its own good; what we need is just a couple of explicit memcmp()
and that's it.
[*] <quoted warnings>
would explain what was really going on - the point is not to make gcc STFU, it's
to make the code more straightforward. The warning is basically "it smells
somewhat fishy around >here<, might be worth taking a look". And yes, it turned
out to be fishy; minimal "make it STFU" would be to strip those NULs from
the initializers (i.e. just go for static char match_root[2][3] = {".", ".."}; -
an array initializer is zero-padded if it's shorter than the array), but that
wasn't the only, er, oddity in that code.
...
> would explain what was really going on - the point is not to make gcc STFU, it's
> to make the code more straightforward. The warning is basically "it smells
> somewhat fishy around >here<, might be worth taking a look". And yes, it turned
> out to be fishy; minimal "make it STFU" would be to strip those NULs from
> the initializers (i.e. just go for static char match_root[2][3] = {".", ".."}; -
> an array initializer is zero-padded if it's shorter than the array), but that
> wasn't the only, er, oddity in that code.
Indeed - looks like it is checking that the first two directory entries
are "." and ".." in about the most complex way possible.
I have vague recollections on some code that ignored the first two entries
because they 'must be "." and ".."' - and then failed because some filesystem
(and I can't even remember the O/S) didn't meet its expectations!
A simple:
if (strcmp(dir_entry[0].de_fname, ".") || strcmp(dir_entry[1].de_fname, ".."))
error = 1;
would suffice.
The compiler ought to completely inline them.
On x86 to:
error |= *(u16 *)dir_entry[0].de_fname ^ '.';
error |= (*(u32 *)dir_entry[1].de_fname & 0xffffff) ^ ('.' * 0x101);
but I bet it doesn't!
(and it isn't quite the same)
David.
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Sun, Oct 06, 2024 at 07:38:07PM +0000, David Laight wrote:
> ...
> > would explain what was really going on - the point is not to make gcc STFU, it's
> > to make the code more straightforward. The warning is basically "it smells
> > somewhat fishy around >here<, might be worth taking a look". And yes, it turned
> > out to be fishy; minimal "make it STFU" would be to strip those NULs from
> > the initializers (i.e. just go for static char match_root[2][3] = {".", ".."}; -
> > an array initializer is zero-padded if it's shorter than the array), but that
> > wasn't the only, er, oddity in that code.
>
> Indeed - looks like it is checking that the first two directory entries
> are "." and ".." in about the most complex way possible.
>
> I have vague recollections on some code that ignored the first two entries
> because they 'must be "." and ".."' - and then failed because some filesystem
> (and I can't even remember the O/S) didn't meet its expectations!
>
> A simple:
> if (strcmp(dir_entry[0].de_fname, ".") || strcmp(dir_entry[1].de_fname, ".."))
> error = 1;
> would suffice.
memcmp(), please. strcmp() is _not_ guaranteed to be safe without both being
NUL-terminated; yes, compiler will almost simplify that in case when one of
the arguments is a string literal, but it's better to do straight memcmp() in
this case. It's not worth trying to be fancy there...
On 06.10.2024 21:00, Al Viro wrote: > > Indeed - looks like it is checking that the first two directory entries > > are "." and ".." in about the most complex way possible. > > > > I have vague recollections on some code that ignored the first two entries > > because they 'must be "." and ".."' - and then failed because some filesystem > > (and I can't even remember the O/S) didn't meet its expectations! > > > > A simple: > > if (strcmp(dir_entry[0].de_fname, ".") || strcmp(dir_entry[1].de_fname, "..")) > > error = 1; > > would suffice. > > memcmp(), please. strcmp() is _not_ guaranteed to be safe without both being > NUL-terminated; yes, compiler will almost simplify that in case when one of > the arguments is a string literal, but it's better to do straight memcmp() in > this case. It's not worth trying to be fancy there... Hi Al, Hi David, Is my version 2 of the patch correct, I updated with Al's initial recommendation (both in terms of the patch and commit). Is there anything else that needs to be updated/added. -- Regards, listout
On 04.10.2024 19:44, Al Viro wrote:
> LGTM, except that I'd probably make the commit message less warning-centric -
> something like
> ...
> would explain what was really going on - the point is not to make gcc STFU, it's
> to make the code more straightforward. The warning is basically "it smells
> somewhat fishy around >here<, might be worth taking a look". And yes, it turned
> out to be fishy; minimal "make it STFU" would be to strip those NULs from
> the initializers (i.e. just go for static char match_root[2][3] = {".", ".."}; -
> an array initializer is zero-padded if it's shorter than the array), but that
> wasn't the only, er, oddity in that code.
Thank you very much for the feedback, sending the patch with updates
commit shortly.
--
Regards,
listout
qnx6_checkroot() had been using weirdly spelled initializer - it needed
to initialize 3-element arrays of char and it used NUL-padded
3-character string literals (i.e. 4-element initializers, with
completely pointless zeroes at the end).
That had been spotted by gcc-15[*]; prior to that gcc quietly dropped
the 4th element of initializers.
However, none of that had been needed in the first place - all this
array is used for is checking that the first directory entry in root
directory is "." and the second - "..". The check had been expressed as
a loop, using that match_root[] array. Since there is no chance that we
ever want to extend that list of entries, the entire thing is much too
fancy for its own good; what we need is just a couple of explicit
memcmp() and that's it.
[*]: fs/qnx6/inode.c: In function ‘qnx6_checkroot’:
fs/qnx6/inode.c:182:41: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
182 | static char match_root[2][3] = {".\0\0", "..\0"};
| ^~~~~~~
fs/qnx6/inode.c:182:50: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
182 | static char match_root[2][3] = {".\0\0", "..\0"};
| ^~~~~~
Signed-off-by: Brahmajit Das <brahmajit.xyz@gmail.com>
---
fs/qnx6/inode.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/fs/qnx6/inode.c b/fs/qnx6/inode.c
index 85925ec0051a..3310d1ad4d0e 100644
--- a/fs/qnx6/inode.c
+++ b/fs/qnx6/inode.c
@@ -179,8 +179,7 @@ static int qnx6_statfs(struct dentry *dentry, struct kstatfs *buf)
*/
static const char *qnx6_checkroot(struct super_block *s)
{
- static char match_root[2][3] = {".\0\0", "..\0"};
- int i, error = 0;
+ int error = 0;
struct qnx6_dir_entry *dir_entry;
struct inode *root = d_inode(s->s_root);
struct address_space *mapping = root->i_mapping;
@@ -189,11 +188,9 @@ static const char *qnx6_checkroot(struct super_block *s)
if (IS_ERR(folio))
return "error reading root directory";
dir_entry = kmap_local_folio(folio, 0);
- for (i = 0; i < 2; i++) {
- /* maximum 3 bytes - due to match_root limitation */
- if (strncmp(dir_entry[i].de_fname, match_root[i], 3))
- error = 1;
- }
+ if (memcmp(dir_entry[0].de_fname, ".", 2) ||
+ memcmp(dir_entry[1].de_fname, "..", 3))
+ error = 1;
folio_release_kmap(folio, dir_entry);
if (error)
return "error reading root directory.";
--
2.46.2
On Sat, Oct 05, 2024 at 01:21:32AM +0530, Brahmajit Das wrote:
> qnx6_checkroot() had been using weirdly spelled initializer - it needed
> to initialize 3-element arrays of char and it used NUL-padded
> 3-character string literals (i.e. 4-element initializers, with
> completely pointless zeroes at the end).
>
> That had been spotted by gcc-15[*]; prior to that gcc quietly dropped
> the 4th element of initializers.
>
> However, none of that had been needed in the first place - all this
> array is used for is checking that the first directory entry in root
> directory is "." and the second - "..". The check had been expressed as
> a loop, using that match_root[] array. Since there is no chance that we
> ever want to extend that list of entries, the entire thing is much too
> fancy for its own good; what we need is just a couple of explicit
> memcmp() and that's it.
>
> [*]: fs/qnx6/inode.c: In function ‘qnx6_checkroot’:
> fs/qnx6/inode.c:182:41: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
> 182 | static char match_root[2][3] = {".\0\0", "..\0"};
> | ^~~~~~~
> fs/qnx6/inode.c:182:50: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
> 182 | static char match_root[2][3] = {".\0\0", "..\0"};
> | ^~~~~~
>
> Signed-off-by: Brahmajit Das <brahmajit.xyz@gmail.com>
Acked-by: Al Viro <viro@zeniv.linux.org.uk>
Sorry, hadn't realized it had fallen through the cracks.
Christian, usually you are handling vfs/vfs.git#vfs.misc; I can put
it there myself, if you prefer it that way...
© 2016 - 2024 Red Hat, Inc.