| Message ID | 173608127627.1253657.12054758575695672674.stgit@devnote2 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tracing/kprobes: Cleanup with guard and __free | expand |
Masami, Sorry for abusing this thread. Your patches look fine to me, it is not that I suggest to change them. I will use your patch as an example for off-topic discussion. On 01/05, Masami Hiramatsu (Google) wrote: > > +DEFINE_FREE(argv, char **, if (!IS_ERR_OR_NULL(_T)) argv_free(_T)) (IS_ERR looks unneeded but this is cosmetic). OK, so it can be used as void func(void) { char **argv __free(argv) = argv_split(...); do_something(argv); return; } And I cry every time when I read the code like this ;) Because, to understand this code, I need to do the "nontrivial" grep to find "DEFINE_FREE(argv,". Perhaps we can establish a simple rule that every DEFINE_FREE() or DEFINE_CLASS() should add another #define? I mean something like DEFINE_FREE(argv, char **, if (!IS_ERR_OR_NULL(_T)) argv_free(_T)) #define __FREE_ARGV __free(argv) void func(void) { char **argv __FREE_ARGV = argv_split(...); do_something(argv); return; } This way I can press Ctrl-] and see what the cleanup code actually does. Can save a second or two. Important when you try to read the code you are not familiar with. Same for DEFINE_CLASS. For example, int ksys_fchown(unsigned int fd, uid_t user, gid_t group) { CLASS(fd, f)(fd); if (fd_empty(f)) return -EBADF; return vfs_fchown(fd_file(f), user, group); } If you are not familiar with this code, it looks mysterious until you find DEFINE_CLASS(fd, ...) in include/linux/file.h. Oleg.
On Sun, Jan 05, 2025 at 03:14:22PM +0100, Oleg Nesterov wrote: > OK, so it can be used as > > void func(void) > { > char **argv __free(argv) = argv_split(...); > do_something(argv); > return; > } > > And I cry every time when I read the code like this ;) > > Because, to understand this code, I need to do the "nontrivial" grep to find > "DEFINE_FREE(argv,". > > Perhaps we can establish a simple rule that every DEFINE_FREE() or DEFINE_CLASS() > should add another #define? I mean something like > > > DEFINE_FREE(argv, char **, if (!IS_ERR_OR_NULL(_T)) argv_free(_T)) > #define __FREE_ARGV __free(argv) > > void func(void) > { > char **argv __FREE_ARGV = argv_split(...); > do_something(argv); > return; > } > > This way I can press Ctrl-] and see what the cleanup code actually does. > Can save a second or two. Important when you try to read the code you are > not familiar with. Right, so I've been playing with neovim and clangd (lsp), and I'm very disappointed to have to tell you that that also doesn't get it :-( One thing we can and should do is something like: diff --git a/scripts/tags.sh b/scripts/tags.sh index b21236377998..f01d694abe65 100755 --- a/scripts/tags.sh +++ b/scripts/tags.sh @@ -212,6 +212,8 @@ regex_c=( '/^SEQCOUNT_LOCKTYPE(\([^,]*\),[[:space:]]*\([^,]*\),[^)]*)/seqcount_\2_init/' '/^\<DECLARE_IDTENTRY[[:alnum:]_]*([^,)]*,[[:space:]]*\([[:alnum:]_]\+\)/\1/' '/^\<DEFINE_IDTENTRY[[:alnum:]_]*([[:space:]]*\([[:alnum:]_]\+\)/\1/' + '/^\<DEFINE_FREE(\([[:alnum:]_]\+\)/cleanup_\1/' + '/^\<DEFINE_CLASS(\([[:alnum:]_]\+\)/class_\1/' ) regex_kconfig=( '/^[[:blank:]]*\(menu\|\)config[[:blank:]]\+\([[:alnum:]_]\+\)/\2/' That should be able to let you do: ':ts cleanup_argv'
On Mon, Jan 06, 2025 at 11:26:48AM +0100, Peter Zijlstra wrote: > On Sun, Jan 05, 2025 at 03:14:22PM +0100, Oleg Nesterov wrote: > > > OK, so it can be used as > > > > void func(void) > > { > > char **argv __free(argv) = argv_split(...); > > do_something(argv); > > return; > > } > > > > And I cry every time when I read the code like this ;) > > > > Because, to understand this code, I need to do the "nontrivial" grep to find > > "DEFINE_FREE(argv,". > > > > Perhaps we can establish a simple rule that every DEFINE_FREE() or DEFINE_CLASS() > > should add another #define? I mean something like > > > > > > DEFINE_FREE(argv, char **, if (!IS_ERR_OR_NULL(_T)) argv_free(_T)) > > #define __FREE_ARGV __free(argv) > > > > void func(void) > > { > > char **argv __FREE_ARGV = argv_split(...); > > do_something(argv); > > return; > > } > > > > This way I can press Ctrl-] and see what the cleanup code actually does. > > Can save a second or two. Important when you try to read the code you are > > not familiar with. > > Right, so I've been playing with neovim and clangd (lsp), and I'm very > disappointed to have to tell you that that also doesn't get it :-( > > One thing we can and should do is something like: > > diff --git a/scripts/tags.sh b/scripts/tags.sh > index b21236377998..f01d694abe65 100755 > --- a/scripts/tags.sh > +++ b/scripts/tags.sh > @@ -212,6 +212,8 @@ regex_c=( > '/^SEQCOUNT_LOCKTYPE(\([^,]*\),[[:space:]]*\([^,]*\),[^)]*)/seqcount_\2_init/' > '/^\<DECLARE_IDTENTRY[[:alnum:]_]*([^,)]*,[[:space:]]*\([[:alnum:]_]\+\)/\1/' > '/^\<DEFINE_IDTENTRY[[:alnum:]_]*([[:space:]]*\([[:alnum:]_]\+\)/\1/' > + '/^\<DEFINE_FREE(\([[:alnum:]_]\+\)/cleanup_\1/' > + '/^\<DEFINE_CLASS(\([[:alnum:]_]\+\)/class_\1/' /^\<EXTEND_CLASS(\([[:alnum:]_]\+\),[[:space:]]*\([[:alnum:]]\+\)/class_\1\2/' /^\<DEFINE_GUARD(\([[:alnum:]_]\+\)/class_\1/' /^\<DEFINE_GUARD_COND(\([[:alnum:]_]\+\),[[:space:]]*\([[:alnum:]]\+\)/class_\1\2/' /^\<DEFINE_LOCK_GUARD_[[:digit:]](\([[:alnum:]_]\+\)/class_\1/' /^\<DEFINE_LOCK_GUARD_[[:digit:]]_COND(\([[:alnum:]_]\+\),[[:space:]]*\([[:alnum:]]\+\)/class_\1\2/' I suppose... not tested these
On Sun, 5 Jan 2025 15:14:22 +0100 Oleg Nesterov <oleg@redhat.com> wrote: > Masami, > > Sorry for abusing this thread. Your patches look fine to me, it is not > that I suggest to change them. I will use your patch as an example for > off-topic discussion. > > On 01/05, Masami Hiramatsu (Google) wrote: > > > > +DEFINE_FREE(argv, char **, if (!IS_ERR_OR_NULL(_T)) argv_free(_T)) > > (IS_ERR looks unneeded but this is cosmetic). > > OK, so it can be used as > > void func(void) > { > char **argv __free(argv) = argv_split(...); > do_something(argv); > return; > } > > And I cry every time when I read the code like this ;) > > Because, to understand this code, I need to do the "nontrivial" grep to find > "DEFINE_FREE(argv,". > > Perhaps we can establish a simple rule that every DEFINE_FREE() or DEFINE_CLASS() > should add another #define? I mean something like > > > DEFINE_FREE(argv, char **, if (!IS_ERR_OR_NULL(_T)) argv_free(_T)) > #define __FREE_ARGV __free(argv) > > void func(void) > { > char **argv __FREE_ARGV = argv_split(...); > do_something(argv); > return; > } > > This way I can press Ctrl-] and see what the cleanup code actually does. > Can save a second or two. Important when you try to read the code you are > not familiar with. That sounds lile a problem of your tool. Do you really need to find the DEFINE_FREE() or do you think "__free(argv)" is too generic name? If it is latter, we can make it "__free(argv_free) so that it is more obvious to call argv_free()? > > Same for DEFINE_CLASS. For example, > > int ksys_fchown(unsigned int fd, uid_t user, gid_t group) > { > CLASS(fd, f)(fd); > > if (fd_empty(f)) > return -EBADF; > > return vfs_fchown(fd_file(f), user, group); > } > > If you are not familiar with this code, it looks mysterious until you find > DEFINE_CLASS(fd, ...) in include/linux/file.h. DEFINE_CLASS() is somewhat mysterious to me too :) But if I understand correctly, it is for intermediate macro for implementing guard(). Whether we like it or not, cleanup.h has been introduced, and it will be more popular. What we need is a document about cleanup.h which includes better naming conventions for its label. BTW, I agree that 'argv' was too simple. Basically the label name of DEFINE_FREE() is better to be a function name for free. Let me fix that. Thank you,
On 01/06, Masami Hiramatsu wrote: > > On Sun, 5 Jan 2025 15:14:22 +0100 > Oleg Nesterov <oleg@redhat.com> wrote: > > > Perhaps we can establish a simple rule that every DEFINE_FREE() or DEFINE_CLASS() > > should add another #define? I mean something like > > > > > > DEFINE_FREE(argv, char **, if (!IS_ERR_OR_NULL(_T)) argv_free(_T)) > > #define __FREE_ARGV __free(argv) > > > > void func(void) > > { > > char **argv __FREE_ARGV = argv_split(...); > > do_something(argv); > > return; > > } > > > > This way I can press Ctrl-] and see what the cleanup code actually does. > > Can save a second or two. Important when you try to read the code you are > > not familiar with. > > That sounds lile a problem of your tool. Do you really need to find the > DEFINE_FREE() Yes, ":tag __free_argv" wont work. it is defined by DEFINE_FREE(argv) above. I need to find this DEFINE_FREE(argv) to see what __free_argv() actually does. > > Same for DEFINE_CLASS. For example, > > > > int ksys_fchown(unsigned int fd, uid_t user, gid_t group) > > { > > CLASS(fd, f)(fd); > > > > if (fd_empty(f)) > > return -EBADF; > > > > return vfs_fchown(fd_file(f), user, group); > > } > > > > If you are not familiar with this code, it looks mysterious until you find > > DEFINE_CLASS(fd, ...) in include/linux/file.h. > > DEFINE_CLASS() is somewhat mysterious to me too :) But if I understand > correctly, it is for intermediate macro for implementing guard(). Well, in this case you just need to find DEFINE_CLASS(fd, struct fd, fdput(_T), fdget(fd), int fd) in include/linux/file.h, after that the code is clear. > BTW, I agree that 'argv' was too simple. Basically the label name of > DEFINE_FREE() is better to be a function name for free. > Let me fix that. Up to you, but __free(argv) looks good to me. Oleg.
On 01/06, Peter Zijlstra wrote: > > On Sun, Jan 05, 2025 at 03:14:22PM +0100, Oleg Nesterov wrote: > > > Perhaps we can establish a simple rule that every DEFINE_FREE() or DEFINE_CLASS() > > should add another #define? I mean something like > > > > DEFINE_FREE(argv, char **, if (!IS_ERR_OR_NULL(_T)) argv_free(_T)) > > #define __FREE_ARGV __free(argv) > > > > void func(void) > > { > > char **argv __FREE_ARGV = argv_split(...); > > do_something(argv); > > return; > > } > > > > This way I can press Ctrl-] and see what the cleanup code actually does. > > Can save a second or two. Important when you try to read the code you are > > not familiar with. > > Right, so I've been playing with neovim and clangd (lsp), and I'm very > disappointed to have to tell you that that also doesn't get it :-( > > One thing we can and should do is something like: > > diff --git a/scripts/tags.sh b/scripts/tags.sh > index b21236377998..f01d694abe65 100755 > --- a/scripts/tags.sh > +++ b/scripts/tags.sh > @@ -212,6 +212,8 @@ regex_c=( > '/^SEQCOUNT_LOCKTYPE(\([^,]*\),[[:space:]]*\([^,]*\),[^)]*)/seqcount_\2_init/' > '/^\<DECLARE_IDTENTRY[[:alnum:]_]*([^,)]*,[[:space:]]*\([[:alnum:]_]\+\)/\1/' > '/^\<DEFINE_IDTENTRY[[:alnum:]_]*([[:space:]]*\([[:alnum:]_]\+\)/\1/' > + '/^\<DEFINE_FREE(\([[:alnum:]_]\+\)/cleanup_\1/' > + '/^\<DEFINE_CLASS(\([[:alnum:]_]\+\)/class_\1/' Yes, thanks ;) Ctrl-] still won't work, but at least ":ta cleanup_argv" will do. Thanks, Oleg.
diff --git a/include/linux/string.h b/include/linux/string.h index 493ac4862c77..07f2a90d5d9c 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -4,6 +4,7 @@ #include <linux/args.h> #include <linux/array_size.h> +#include <linux/cleanup.h> /* for DEFINE_FREE() */ #include <linux/compiler.h> /* for inline */ #include <linux/types.h> /* for size_t */ #include <linux/stddef.h> /* for NULL */ @@ -312,6 +313,8 @@ extern void *kmemdup_array(const void *src, size_t count, size_t element_size, g extern char **argv_split(gfp_t gfp, const char *str, int *argcp); extern void argv_free(char **argv); +DEFINE_FREE(argv, char **, if (!IS_ERR_OR_NULL(_T)) argv_free(_T)) + /* lib/cmdline.c */ extern int get_option(char **str, int *pint); extern char *get_options(const char *str, int nints, int *ints);