* [PATCH v2] seq_file: fix clang warning for NULL pointer arithmetic
@ 2020-10-27 14:52 Arnd Bergmann
2020-10-27 19:22 ` Christoph Hellwig
0 siblings, 1 reply; 3+ messages in thread
From: Arnd Bergmann @ 2020-10-27 14:52 UTC (permalink / raw)
To: Christoph Hellwig, Greg Kroah-Hartman, Tejun Heo, Alexander Viro
Cc: Arnd Bergmann, Nathan Chancellor, Nick Desaulniers, Jan Kara,
Amir Goldstein, linux-kernel, linux-fsdevel, clang-built-linux
From: Arnd Bergmann <arnd@arndb.de>
Clang points out that adding something to NULL is notallowed
in standard C:
fs/kernfs/file.c:127:15: warning: performing pointer arithmetic on a
null pointer has undefined behavior [-Wnull-pointer-arithmetic]
return NULL + !*ppos;
~~~~ ^
fs/seq_file.c:529:14: warning: performing pointer arithmetic on a
null pointer has undefined behavior [-Wnull-pointer-arithmetic]
return NULL + (*pos == 0);
Rephrase the code to be extra explicit about the valid, giving
them named SEQ_OPEN_EOF and SEQ_OPEN_SINGLE definitions.
The instance in kernfs was copied from single_start, so fix both
at once.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Fixes: c2b19daf6760 ("sysfs, kernfs: prepare read path for kernfs")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: add the named macros after Christoph Hellwig pointed out
that my original logic was too ugly.
Suggestions for better names welcome
---
fs/kernfs/file.c | 8 ++++----
fs/seq_file.c | 4 ++--
include/linux/seq_file.h | 3 +++
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index f277d023ebcd..eafeb8bf4fe4 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -121,10 +121,10 @@ static void *kernfs_seq_start(struct seq_file *sf, loff_t *ppos)
return next;
} else {
/*
- * The same behavior and code as single_open(). Returns
- * !NULL if pos is at the beginning; otherwise, NULL.
+ * The same behavior and code as single_open(). Continues
+ * if pos is at the beginning; otherwise, EOF.
*/
- return NULL + !*ppos;
+ return *ppos ? SEQ_OPEN_SINGLE : SEQ_OPEN_EOF;
}
}
@@ -145,7 +145,7 @@ static void *kernfs_seq_next(struct seq_file *sf, void *v, loff_t *ppos)
* terminate after the initial read.
*/
++*ppos;
- return NULL;
+ return SEQ_OPEN_EOF;
}
}
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 31219c1db17d..203cd86136ad 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -526,13 +526,13 @@ EXPORT_SYMBOL(seq_dentry);
static void *single_start(struct seq_file *p, loff_t *pos)
{
- return NULL + (*pos == 0);
+ return *pos == 0 ? SEQ_OPEN_SINGLE : SEQ_OPEN_EOF;
}
static void *single_next(struct seq_file *p, void *v, loff_t *pos)
{
++*pos;
- return NULL;
+ return SEQ_OPEN_EOF;
}
static void single_stop(struct seq_file *p, void *v)
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 813614d4b71f..26f0758b6551 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -37,6 +37,9 @@ struct seq_operations {
#define SEQ_SKIP 1
+#define SEQ_OPEN_EOF (void *)0
+#define SEQ_OPEN_SINGLE (void *)1
+
/**
* seq_has_overflowed - check if the buffer has overflowed
* @m: the seq_file handle
--
2.27.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] seq_file: fix clang warning for NULL pointer arithmetic
2020-10-27 14:52 [PATCH v2] seq_file: fix clang warning for NULL pointer arithmetic Arnd Bergmann
@ 2020-10-27 19:22 ` Christoph Hellwig
2020-10-27 20:00 ` Arnd Bergmann
0 siblings, 1 reply; 3+ messages in thread
From: Christoph Hellwig @ 2020-10-27 19:22 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Christoph Hellwig, Greg Kroah-Hartman, Tejun Heo, Alexander Viro,
Arnd Bergmann, Nathan Chancellor, Nick Desaulniers, Jan Kara,
Amir Goldstein, linux-kernel, linux-fsdevel, clang-built-linux
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index f277d023ebcd..eafeb8bf4fe4 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -121,10 +121,10 @@ static void *kernfs_seq_start(struct seq_file *sf, loff_t *ppos)
> return next;
> } else {
> /*
> - * The same behavior and code as single_open(). Returns
> - * !NULL if pos is at the beginning; otherwise, NULL.
> + * The same behavior and code as single_open(). Continues
> + * if pos is at the beginning; otherwise, EOF.
> */
> - return NULL + !*ppos;
> + return *ppos ? SEQ_OPEN_SINGLE : SEQ_OPEN_EOF;
Why the somewhat obsfucating unary expression instead of a good
old if?
e.g.
return next;
}
if (*ppos)
retun SEQ_OPEN_SINGLE;
return NULL;
> ++*ppos;
> - return NULL;
> + return SEQ_OPEN_EOF;
I don't think SEQ_OPEN_EOF is all that useful. NULL is the documented
end case already.
> diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
> index 813614d4b71f..26f0758b6551 100644
> --- a/include/linux/seq_file.h
> +++ b/include/linux/seq_file.h
> @@ -37,6 +37,9 @@ struct seq_operations {
>
> #define SEQ_SKIP 1
>
> +#define SEQ_OPEN_EOF (void *)0
> +#define SEQ_OPEN_SINGLE (void *)1
I think SEQ_OPEN_SINGLE also wants a comment documenting it.
AFAICS the reason for it is that ->start needs to return something
non-NULL for the seq_file code to make progress, and there is nothing
better for the single_open case.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] seq_file: fix clang warning for NULL pointer arithmetic
2020-10-27 19:22 ` Christoph Hellwig
@ 2020-10-27 20:00 ` Arnd Bergmann
0 siblings, 0 replies; 3+ messages in thread
From: Arnd Bergmann @ 2020-10-27 20:00 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Greg Kroah-Hartman, Tejun Heo, Alexander Viro, Nathan Chancellor,
Nick Desaulniers, Jan Kara, Amir Goldstein, linux-kernel,
Linux FS-devel Mailing List, clang-built-linux
On Tue, Oct 27, 2020 at 8:22 PM Christoph Hellwig <hch@infradead.org> wrote:
> > diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> > index f277d023ebcd..eafeb8bf4fe4 100644
> > --- a/fs/kernfs/file.c
> > +++ b/fs/kernfs/file.c
> > @@ -121,10 +121,10 @@ static void *kernfs_seq_start(struct seq_file *sf, loff_t *ppos)
> > return next;
> > } else {
> > /*
> > - * The same behavior and code as single_open(). Returns
> > - * !NULL if pos is at the beginning; otherwise, NULL.
> > + * The same behavior and code as single_open(). Continues
> > + * if pos is at the beginning; otherwise, EOF.
> > */
> > - return NULL + !*ppos;
> > + return *ppos ? SEQ_OPEN_SINGLE : SEQ_OPEN_EOF;
>
> Why the somewhat obsfucating unary expression instead of a good
> old if?
>
> e.g.
>
> return next;
> }
> if (*ppos)
> retun SEQ_OPEN_SINGLE;
> return NULL;
>
>
> > ++*ppos;
> > - return NULL;
> > + return SEQ_OPEN_EOF;
>
> I don't think SEQ_OPEN_EOF is all that useful. NULL is the documented
> end case already.
Right, Al already pointed out the same thing on IRC.
> > diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
> > index 813614d4b71f..26f0758b6551 100644
> > --- a/include/linux/seq_file.h
> > +++ b/include/linux/seq_file.h
> > @@ -37,6 +37,9 @@ struct seq_operations {
> >
> > #define SEQ_SKIP 1
> >
> > +#define SEQ_OPEN_EOF (void *)0
> > +#define SEQ_OPEN_SINGLE (void *)1
>
> I think SEQ_OPEN_SINGLE also wants a comment documenting it.
> AFAICS the reason for it is that ->start needs to return something
> non-NULL for the seq_file code to make progress, and there is nothing
> better for the single_open case.
Ok.
Arnd
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-27 20:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-27 14:52 [PATCH v2] seq_file: fix clang warning for NULL pointer arithmetic Arnd Bergmann
2020-10-27 19:22 ` Christoph Hellwig
2020-10-27 20:00 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).