| Message ID | 20250131083201.GX1977892@ZenIV (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] add a string-to-qstr constructor | expand |
On Fri, 31 Jan 2025 at 00:32, Al Viro <viro@zeniv.linux.org.uk> wrote: > > This commit lifts definition(s) of QSTR() into linux/dcache.h, > converts it to compound literal (all bcachefs users are fine > with that) and converts assorted open-coded instances to using > that. Looks fine to me. I do wonder if the cast to 'struct qstr' should be part of QSTR_INIT, so that you can use that the same way when you already know the length. We have code like this in fs/overlayfs/namei.c: *name = (struct qstr) QSTR_INIT(n, s - n); in bcachefs has return (struct qstr) QSTR_INIT(d.v->d_name, bch2_dirent_name_bytes(d)); So both of them would seem to want to have that cast as part of the QSTR_INIT() thing. Or maybe we could just make QSTR() itself more powerful, and do something like this: #define QSTR_INIT(n, l, ...) { { { .len = l } }, .name = n } #define QSTR(a, ...) (struct qstr) QSTR_INIT(a , ## __VA_ARGS__, strlen(a)) which allows you to write either "QSTR(str)" or "QSTR(str, len)", and defaults to using 'strlen()' when no length is given. Because if we have heper macros, let's make them _helpful_. Side note: you missed a few places in the core VFS code that could also use this new cleanup: struct qstr this = QSTR_INIT("pts", 3); ... child = d_hash_and_lookup(parent, &this); can now be just child = d_hash_and_lookup(parent, &QSTR("pts")); but sadly a few more are static initializers and can't use 'strlen()'. Linus
On Fri, Jan 31, 2025 at 11:29:05AM -0800, Linus Torvalds wrote: > On Fri, 31 Jan 2025 at 00:32, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > This commit lifts definition(s) of QSTR() into linux/dcache.h, > > converts it to compound literal (all bcachefs users are fine > > with that) and converts assorted open-coded instances to using > > that. > > Looks fine to me. I do wonder if the cast to 'struct qstr' should be > part of QSTR_INIT, so that you can use that the same way when you > already know the length. > > We have code like this in fs/overlayfs/namei.c: > > *name = (struct qstr) QSTR_INIT(n, s - n); > > in bcachefs has > > return (struct qstr) QSTR_INIT(d.v->d_name, bch2_dirent_name_bytes(d)); > > So both of them would seem to want to have that cast as part of the > QSTR_INIT() thing. > > Or maybe we could just make QSTR() itself more powerful, and do > something like this: > > #define QSTR_INIT(n, l, ...) { { { .len = l } }, .name = n } > #define QSTR(a, ...) (struct qstr) QSTR_INIT(a , ## __VA_ARGS__, strlen(a)) > > which allows you to write either "QSTR(str)" or "QSTR(str, len)", and > defaults to using 'strlen()' when no length is given. > > Because if we have heper macros, let's make them _helpful_. > > Side note: you missed a few places in the core VFS code that could > also use this new cleanup: > > struct qstr this = QSTR_INIT("pts", 3); > ... > child = d_hash_and_lookup(parent, &this); > > can now be just > > child = d_hash_and_lookup(parent, &QSTR("pts")); > > but sadly a few more are static initializers and can't use 'strlen()'. There's also this in fs/fuse/inode.c const struct qstr name = QSTR_INIT(".", 1); and struct qstr null_name = QSTR_INIT(NULL_FILE_NAME, sizeof(NULL_FILE_NAME)-1); in selinuxfs - I wasn't trying to get them all, just the infrastructure and enough examples to demonstrate the usefulness. Once the definition is merged, those could be switched at leisure.
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c index 42bd1cb7c9cd..583ac81669c2 100644 --- a/fs/anon_inodes.c +++ b/fs/anon_inodes.c @@ -60,14 +60,14 @@ static struct inode *anon_inode_make_secure_inode( const struct inode *context_inode) { struct inode *inode; - const struct qstr qname = QSTR_INIT(name, strlen(name)); int error; inode = alloc_anon_inode(anon_inode_mnt->mnt_sb); if (IS_ERR(inode)) return inode; inode->i_flags &= ~S_PRIVATE; - error = security_inode_init_security_anon(inode, &qname, context_inode); + error = security_inode_init_security_anon(inode, &QSTR(name), + context_inode); if (error) { iput(inode); return ERR_PTR(error); diff --git a/fs/bcachefs/fsck.c b/fs/bcachefs/fsck.c index 75c8a97a6954..7b3b63ed747c 100644 --- a/fs/bcachefs/fsck.c +++ b/fs/bcachefs/fsck.c @@ -405,7 +405,7 @@ static int reattach_inode(struct btree_trans *trans, struct bch_inode_unpacked * return ret; struct bch_hash_info dir_hash = bch2_hash_info_init(c, &lostfound); - struct qstr name = (struct qstr) QSTR(name_buf); + struct qstr name = QSTR(name_buf); inode->bi_dir = lostfound.bi_inum; diff --git a/fs/bcachefs/recovery.c b/fs/bcachefs/recovery.c index 3c7f941dde39..ebabba296882 100644 --- a/fs/bcachefs/recovery.c +++ b/fs/bcachefs/recovery.c @@ -32,8 +32,6 @@ #include <linux/sort.h> #include <linux/stat.h> -#define QSTR(n) { { { .len = strlen(n) } }, .name = n } - void bch2_btree_lost_data(struct bch_fs *c, enum btree_id btree) { if (btree >= BTREE_ID_NR_MAX) diff --git a/fs/bcachefs/util.h b/fs/bcachefs/util.h index fb02c1c36004..a27f4b84fe77 100644 --- a/fs/bcachefs/util.h +++ b/fs/bcachefs/util.h @@ -647,8 +647,6 @@ static inline int cmp_le32(__le32 l, __le32 r) #include <linux/uuid.h> -#define QSTR(n) { { { .len = strlen(n) } }, .name = n } - static inline bool qstr_eq(const struct qstr l, const struct qstr r) { return l.len == r.len && !memcmp(l.name, r.name, l.len); diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c index a90d7d649739..60d2cf26e837 100644 --- a/fs/erofs/xattr.c +++ b/fs/erofs/xattr.c @@ -407,7 +407,7 @@ int erofs_getxattr(struct inode *inode, int index, const char *name, } it.index = index; - it.name = (struct qstr)QSTR_INIT(name, strlen(name)); + it.name = QSTR(name); if (it.name.len > EROFS_NAME_LEN) return -ERANGE; diff --git a/fs/file_table.c b/fs/file_table.c index 976736be47cb..a329623d0b42 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -351,9 +351,7 @@ static struct file *alloc_file(const struct path *path, int flags, static inline int alloc_path_pseudo(const char *name, struct inode *inode, struct vfsmount *mnt, struct path *path) { - struct qstr this = QSTR_INIT(name, strlen(name)); - - path->dentry = d_alloc_pseudo(mnt->mnt_sb, &this); + path->dentry = d_alloc_pseudo(mnt->mnt_sb, &QSTR(name)); if (!path->dentry) return -ENOMEM; path->mnt = mntget(mnt); diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c index 8502ef68459b..0eb320617d7b 100644 --- a/fs/kernfs/file.c +++ b/fs/kernfs/file.c @@ -927,7 +927,7 @@ static void kernfs_notify_workfn(struct work_struct *work) if (!inode) continue; - name = (struct qstr)QSTR_INIT(kn->name, strlen(kn->name)); + name = QSTR(kn->name); parent = kernfs_get_parent(kn); if (parent) { p_inode = ilookup(info->sb, kernfs_ino(parent)); diff --git a/include/linux/dcache.h b/include/linux/dcache.h index bff956f7b2b9..3d53a6014591 100644 --- a/include/linux/dcache.h +++ b/include/linux/dcache.h @@ -57,6 +57,7 @@ struct qstr { }; #define QSTR_INIT(n,l) { { { .len = l } }, .name = n } +#define QSTR(n) (struct qstr)QSTR_INIT(n, strlen(n)) extern const struct qstr empty_name; extern const struct qstr slash_name; diff --git a/mm/secretmem.c b/mm/secretmem.c index 399552814fd0..1b0a214ee558 100644 --- a/mm/secretmem.c +++ b/mm/secretmem.c @@ -195,14 +195,13 @@ static struct file *secretmem_file_create(unsigned long flags) struct file *file; struct inode *inode; const char *anon_name = "[secretmem]"; - const struct qstr qname = QSTR_INIT(anon_name, strlen(anon_name)); int err; inode = alloc_anon_inode(secretmem_mnt->mnt_sb); if (IS_ERR(inode)) return ERR_CAST(inode); - err = security_inode_init_security_anon(inode, &qname, NULL); + err = security_inode_init_security_anon(inode, &QSTR(anon_name), NULL); if (err) { file = ERR_PTR(err); goto err_free_inode; diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c index 7ce3721c06ca..eadc00410ebc 100644 --- a/net/sunrpc/rpc_pipe.c +++ b/net/sunrpc/rpc_pipe.c @@ -630,7 +630,7 @@ static int __rpc_rmpipe(struct inode *dir, struct dentry *dentry) static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent, const char *name) { - struct qstr q = QSTR_INIT(name, strlen(name)); + struct qstr q = QSTR(name); struct dentry *dentry = d_hash_and_lookup(parent, &q); if (!dentry) { dentry = d_alloc(parent, &q); @@ -1190,8 +1190,7 @@ static const struct rpc_filelist files[] = { struct dentry *rpc_d_lookup_sb(const struct super_block *sb, const unsigned char *dir_name) { - struct qstr dir = QSTR_INIT(dir_name, strlen(dir_name)); - return d_hash_and_lookup(sb->s_root, &dir); + return d_hash_and_lookup(sb->s_root, &QSTR(dir_name)); } EXPORT_SYMBOL_GPL(rpc_d_lookup_sb); @@ -1300,11 +1299,9 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data) struct dentry *gssd_dentry; struct dentry *clnt_dentry = NULL; struct dentry *pipe_dentry = NULL; - struct qstr q = QSTR_INIT(files[RPCAUTH_gssd].name, - strlen(files[RPCAUTH_gssd].name)); /* We should never get this far if "gssd" doesn't exist */ - gssd_dentry = d_hash_and_lookup(root, &q); + gssd_dentry = d_hash_and_lookup(root, &QSTR(files[RPCAUTH_gssd].name)); if (!gssd_dentry) return ERR_PTR(-ENOENT); @@ -1314,9 +1311,8 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data) goto out; } - q.name = gssd_dummy_clnt_dir[0].name; - q.len = strlen(gssd_dummy_clnt_dir[0].name); - clnt_dentry = d_hash_and_lookup(gssd_dentry, &q); + clnt_dentry = d_hash_and_lookup(gssd_dentry, + &QSTR(gssd_dummy_clnt_dir[0].name)); if (!clnt_dentry) { __rpc_depopulate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1); pipe_dentry = ERR_PTR(-ENOENT);
[that thing sat in -next for a while, but hadn't been posted yet; mea culpa. Some followups for the next cycle are using it, and it would be more convenient to get it before the next merge window; if it gets any objections, I'll drop it from pull request for #work.next, but it would be nice to have it in -rc1] Quite a few places want to build a struct qstr by given string; it would be convenient to have a primitive doing that, rather than open-coding it via QSTR_INIT(). The closest approximation was in bcachefs, but that expands to initializer list - {.len = strlen(string), .name = string}. It would be more useful to have it as compound literal - (struct qstr){.len = strlen(string), .name = string}. Unlike initializer list it's a valid expression. What's more, it's a valid lvalue - it's an equivalent of anonymous local variable with such initializer, so the things like path->dentry = d_alloc_pseudo(mnt->mnt_sb, &QSTR(name)); are valid. It can also be used as initializer, with identical effect - struct qstr x = (struct qstr){.name = s, .len = strlen(s)}; is equivalent to struct qstr anon_variable = {.name = s, .len = strlen(s)}; struct qstr x = anon_variable; // anon_variable is never used after that point and any even remotely sane compiler will manage to collapse that into struct qstr x = {.name = s, .len = strlen(s)}; What compound literals can't be used for is initialization of global variables, but those are covered by QSTR_INIT(). This commit lifts definition(s) of QSTR() into linux/dcache.h, converts it to compound literal (all bcachefs users are fine with that) and converts assorted open-coded instances to using that. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> ---