| Message ID | 1424841259-8470-1-git-send-email-zhangzl2013@126.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Feb 25, 2015 at 12:14 AM, Zhang Zhaolong <zhangzl2013@126.com> wrote: > > proc_create() should return true if CONFIG_PROC_FS is not configured. > Otherwise if-statement like this "if (!proc_create())" would go to the false path. Does that even compile? proc_create() and proc_create_data() both return "struct proc_dir_entry *". It doesn't make sense for those macros to "return" anything but NULL - certainly not 1. Besides, why shouldn't "if (!proc_create())" behave like proc_create() failed when CONFIG_PROC_FS is not enabled? You wouldn't want the caller to start trying to use that ((struct proc_dir_entry *)1) would you? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jonathon Reinhart <jonathon.reinhart@gmail.com> wrote: > Does that even compile? proc_create() and proc_create_data() both return > "struct proc_dir_entry *". It doesn't make sense for those macros to "return" > anything but NULL - certainly not 1. > > Besides, why shouldn't "if (!proc_create())" behave like proc_create() > failed when > CONFIG_PROC_FS is not enabled? You wouldn't want the caller to start trying > to use that ((struct proc_dir_entry *)1) would you? It's sort of arguable. If the proc interface is merely informational and doesn't impact the function of a module to not be present, then, yes, having proc_create() return some "true" value might make sense. It's possible to arrange things so that all the proc-related functions and data get compiled out in such a situation by not being referenced by anything. However, if the proc interface isn't merely functional, then the proc_create() failure *is* cause for module loading failure. But in that case, there should be a Kconfig dependency on procfs and you should never use the dummy functions. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 25, 2015 at 6:19 AM, David Howells <dhowells@redhat.com> wrote: >> Does that even compile? proc_create() and proc_create_data() both return >> "struct proc_dir_entry *". It doesn't make sense for those macros to "return" >> anything but NULL - certainly not 1. >> >> Besides, why shouldn't "if (!proc_create())" behave like proc_create() >> failed when >> CONFIG_PROC_FS is not enabled? You wouldn't want the caller to start trying >> to use that ((struct proc_dir_entry *)1) would you? > > It's sort of arguable. If the proc interface is merely informational and > doesn't impact the function of a module to not be present, then, yes, having > proc_create() return some "true" value might make sense. It's possible to > arrange things so that all the proc-related functions and data get compiled > out in such a situation by not being referenced by anything. > > However, if the proc interface isn't merely functional, then the proc_create() > failure *is* cause for module loading failure. But in that case, there should > be a Kconfig dependency on procfs and you should never use the dummy > functions. Of the 528 calls to proc_create(_data), only 70 are in an "if (!proc_create())" style conditional. Another 188 assign the result to something, presumably a "struct proc_dir_entry *, which are going to fail compilation with this patch. Is breaking the build when the result of these dummy functions are assigned to something what you're after? If that's the case, then why not remove them altogether? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/26/2015 05:22 AM, Jonathon Reinhart wrote: > On Wed, Feb 25, 2015 at 6:19 AM, David Howells<dhowells@redhat.com> wrote: >>> >>Does that even compile? proc_create() and proc_create_data() both return >>> >>"struct proc_dir_entry *". It doesn't make sense for those macros to "return" >>> >>anything but NULL - certainly not 1. >>> >> >>> >>Besides, why shouldn't "if (!proc_create())" behave like proc_create() >>> >>failed when >>> >>CONFIG_PROC_FS is not enabled? You wouldn't want the caller to start trying >>> >>to use that ((struct proc_dir_entry *)1) would you? >> > >> >It's sort of arguable. If the proc interface is merely informational and >> >doesn't impact the function of a module to not be present, then, yes, having >> >proc_create() return some "true" value might make sense. It's possible to >> >arrange things so that all the proc-related functions and data get compiled >> >out in such a situation by not being referenced by anything. >> > >> >However, if the proc interface isn't merely functional, then the proc_create() >> >failure*is* cause for module loading failure. But in that case, there should >> >be a Kconfig dependency on procfs and you should never use the dummy >> >functions. > Of the 528 calls to proc_create(_data), only 70 are in an "if (!proc_create())" > style conditional. Another 188 assign the result to something, presumably a > "struct proc_dir_entry *, which are going to fail compilation with this patch. Oh, the usage of the CONFIG_PROC_FS macro should be checked wherever proc_create(_data) being called, right? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h index b97bf2e..1fc07b9 100644 --- a/include/linux/proc_fs.h +++ b/include/linux/proc_fs.h @@ -60,8 +60,8 @@ static inline struct proc_dir_entry *proc_mkdir_data(const char *name, umode_t mode, struct proc_dir_entry *parent, void *data) { return NULL; } static inline struct proc_dir_entry *proc_mkdir_mode(const char *name, umode_t mode, struct proc_dir_entry *parent) { return NULL; } -#define proc_create(name, mode, parent, proc_fops) ({NULL;}) -#define proc_create_data(name, mode, parent, proc_fops, data) ({NULL;}) +#define proc_create(name, mode, parent, proc_fops) ({ 1; }) +#define proc_create_data(name, mode, parent, proc_fops, data) ({ 1; }) static inline void proc_set_size(struct proc_dir_entry *de, loff_t size) {} static inline void proc_set_user(struct proc_dir_entry *de, kuid_t uid, kgid_t gid) {}
proc_create() should return true if CONFIG_PROC_FS is not configured. Otherwise if-statement like this "if (!proc_create())" would go to the false path. Signed-off-by: Zhang Zhaolong <zhangzl2013@126.com> --- include/linux/proc_fs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)