| Message ID | 20241002012125.405368-1-linux@treblig.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | printf: Remove unused 'bprintf' | expand |
On Wed, Oct 02, 2024 at 02:21:25AM +0100, linux@treblig.org wrote: > From: "Dr. David Alan Gilbert" <linux@treblig.org> > > bprintf is unused. Remove it. bprintf() > It was added in > commit 4370aa4aa753 ("vsprintf: add binary printf") > > but as far as I can see was never used, unlike the other > two functions in that patch. Please, rewrap these lines to use more room on each line. ... I am not familiar with tricks in BPF or ftrace code where this actually might be implicitly called via a macro, but brief grep gives nothing that might point to that. Hence, with the amended commit message Reviewed-by: Andy Shevchenko <andy@kernel.org> P.S. I hope we may rely in CIs to report issues soon, if any.
* Andy Shevchenko (andy@kernel.org) wrote: > On Wed, Oct 02, 2024 at 02:21:25AM +0100, linux@treblig.org wrote: > > From: "Dr. David Alan Gilbert" <linux@treblig.org> > > > > bprintf is unused. Remove it. > > bprintf() OK. > > It was added in > > commit 4370aa4aa753 ("vsprintf: add binary printf") > > > > but as far as I can see was never used, unlike the other > > two functions in that patch. > > Please, rewrap these lines to use more room on each line. Something like this? : bprintf() is unused. Remove it. It was added in commit 4370aa4aa753 ("vsprintf: add binary printf") but as far as I can see was never used, unlike the other two functions in that patch. > ... > > I am not familiar with tricks in BPF or ftrace code where this actually might > be implicitly called via a macro, but brief grep gives nothing that might point > to that. I've got an all-yes build (well, most after I took out broken stuff) booting with it, and it has CONFIG_BINARY_PRINTF=y and CONFIG_FTRACE=y . trace_seq.c uses seq_buf_bprintf which uses bstr_printf rather than the plain bprintf() that I've deleted. Not sure where to dig in BPF, but I've had a fairly good grep around. > Hence, with the amended commit message > > Reviewed-by: Andy Shevchenko <andy@kernel.org> Thanks! > P.S. I hope we may rely in CIs to report issues soon, if any. Sure Dave > -- > With Best Regards, > Andy Shevchenko > > >
On Wed, Oct 02, 2024 at 02:31:19PM +0000, Dr. David Alan Gilbert wrote: > * Andy Shevchenko (andy@kernel.org) wrote: > > On Wed, Oct 02, 2024 at 02:21:25AM +0100, linux@treblig.org wrote: > > > From: "Dr. David Alan Gilbert" <linux@treblig.org> > > > > > > bprintf is unused. Remove it. > > > > bprintf() > > OK. > > > > It was added in > > > commit 4370aa4aa753 ("vsprintf: add binary printf") > > > > > > but as far as I can see was never used, unlike the other > > > two functions in that patch. > > > > Please, rewrap these lines to use more room on each line. > > Something like this? : > bprintf() is unused. Remove it. It was added in > > commit 4370aa4aa753 ("vsprintf: add binary printf") > > but as far as I can see was never used, unlike the other two functions in > that patch. You have two blank lines in the middle, do you really need them? I would put it like this bprintf() is unused. Remove it. It was added in the commit 4370aa4aa753 ("vsprintf: add binary printf") but as far as I can see was never used, unlike the other two functions in that patch.
On Wed, 2 Oct 2024 14:31:19 +0000 "Dr. David Alan Gilbert" <linux@treblig.org> wrote: > > I am not familiar with tricks in BPF or ftrace code where this actually might > > be implicitly called via a macro, but brief grep gives nothing that might point > > to that. > > I've got an all-yes build (well, most after I took out broken stuff) booting > with it, and it has CONFIG_BINARY_PRINTF=y and CONFIG_FTRACE=y . > > trace_seq.c uses seq_buf_bprintf which uses bstr_printf rather than the plain > bprintf() that I've deleted. > Not sure where to dig in BPF, but I've had a fairly good grep around. I believe it is safe to delete. It looks like bprintf() was added for completeness, where as everything is just using the vbin_printf() directly. bprintf() is nothing more than a wrapper around it in case someone wanted to use binary prints directly. But I'm not sure there's a good use case for it, as all users would likely need to add some code around it for processing (like trace.c does). Send a v2 and I could take it for v6.13. -- Steve
* Andy Shevchenko (andy@kernel.org) wrote: > On Wed, Oct 02, 2024 at 02:31:19PM +0000, Dr. David Alan Gilbert wrote: > > * Andy Shevchenko (andy@kernel.org) wrote: > > > On Wed, Oct 02, 2024 at 02:21:25AM +0100, linux@treblig.org wrote: > > > > From: "Dr. David Alan Gilbert" <linux@treblig.org> > > > > > > > > bprintf is unused. Remove it. > > > > > > bprintf() > > > > OK. > > > > > > It was added in > > > > commit 4370aa4aa753 ("vsprintf: add binary printf") > > > > > > > > but as far as I can see was never used, unlike the other > > > > two functions in that patch. > > > > > > Please, rewrap these lines to use more room on each line. > > > > Something like this? : > > > bprintf() is unused. Remove it. It was added in > > > > commit 4370aa4aa753 ("vsprintf: add binary printf") > > > > but as far as I can see was never used, unlike the other two functions in > > that patch. > > You have two blank lines in the middle, do you really need them? No, I was just trying to keep the commit reference separate, I'm never quite sure what checkpatch gets upset by. > I would put it like this > > bprintf() is unused. Remove it. It was added in the commit 4370aa4aa753 > ("vsprintf: add binary printf") but as far as I can see was never used, > unlike the other two functions in that patch. Sure, I can send a v2 like that. Dave > -- > With Best Regards, > Andy Shevchenko > >
* Steven Rostedt (rostedt@goodmis.org) wrote: > On Wed, 2 Oct 2024 14:31:19 +0000 > "Dr. David Alan Gilbert" <linux@treblig.org> wrote: > > > > I am not familiar with tricks in BPF or ftrace code where this actually might > > > be implicitly called via a macro, but brief grep gives nothing that might point > > > to that. > > > > I've got an all-yes build (well, most after I took out broken stuff) booting > > with it, and it has CONFIG_BINARY_PRINTF=y and CONFIG_FTRACE=y . > > > > trace_seq.c uses seq_buf_bprintf which uses bstr_printf rather than the plain > > bprintf() that I've deleted. > > Not sure where to dig in BPF, but I've had a fairly good grep around. > > I believe it is safe to delete. It looks like bprintf() was added for > completeness, where as everything is just using the vbin_printf() directly. > bprintf() is nothing more than a wrapper around it in case someone wanted > to use binary prints directly. But I'm not sure there's a good use case for > it, as all users would likely need to add some code around it for > processing (like trace.c does). > > Send a v2 and I could take it for v6.13. Thanks, Just posted, message-id 20241002173147.210107-1-linux@treblig.org Dave > -- Steve >
diff --git a/include/linux/string.h b/include/linux/string.h index 0dd27afcfaf7..493ac4862c77 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -335,7 +335,6 @@ int __sysfs_match_string(const char * const *array, size_t n, const char *s); #ifdef CONFIG_BINARY_PRINTF int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args); int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf); -int bprintf(u32 *bin_buf, size_t size, const char *fmt, ...) __printf(3, 4); #endif extern ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos, diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 09f022ba1c05..d072cd59eb6a 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -3383,29 +3383,6 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf) } EXPORT_SYMBOL_GPL(bstr_printf); -/** - * bprintf - Parse a format string and place args' binary value in a buffer - * @bin_buf: The buffer to place args' binary value - * @size: The size of the buffer(by words(32bits), not characters) - * @fmt: The format string to use - * @...: Arguments for the format string - * - * The function returns the number of words(u32) written - * into @bin_buf. - */ -int bprintf(u32 *bin_buf, size_t size, const char *fmt, ...) -{ - va_list args; - int ret; - - va_start(args, fmt); - ret = vbin_printf(bin_buf, size, fmt, args); - va_end(args); - - return ret; -} -EXPORT_SYMBOL_GPL(bprintf); - #endif /* CONFIG_BINARY_PRINTF */ /**