linux-kernel - Re: [RFC PATCH 12/20] famfs: Add inode_operations and file_system

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <y2ps2uhwp46x73hnlqz6hjik34kmw7qc2gkzyfcccfnp2f23em@254i2r3wm4eh>
Date: Mon, 26 Feb 2024 16:53:33 -0600
From: John Groves <John@...ves.net>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: John Groves <jgroves@...ron.com>, Jonathan Corbet <corbet@....net>, 
	Dan Williams <dan.j.williams@...el.com>, Vishal Verma <vishal.l.verma@...el.com>, 
	Dave Jiang <dave.jiang@...el.com>, Alexander Viro <viro@...iv.linux.org.uk>, 
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, Matthew Wilcox <willy@...radead.org>, 
	linux-cxl@...r.kernel.org, linux-fsdevel@...r.kernel.org, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, nvdimm@...ts.linux.dev, john@...alactic.com, 
	Dave Chinner <david@...morbit.com>, Christoph Hellwig <hch@...radead.org>, 
	dave.hansen@...ux.intel.com, gregory.price@...verge.com
Subject: Re: [RFC PATCH 12/20] famfs: Add inode_operations and
 file_system_type

On 24/02/26 01:25PM, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 11:41:56 -0600
> John Groves <John@...ves.net> wrote:
> 
> > This commit introduces the famfs inode_operations. There is nothing really
> > unique to famfs here in the inode_operations..
> > 
> > This commit also introduces the famfs_file_system_type struct and the
> > famfs_kill_sb() function.
> > 
> > Signed-off-by: John Groves <john@...ves.net>
> 
> Trivial comments only.
> 
> > +
> > +/*
> > + * File creation. Allocate an inode, and we're done..
> > + */
> > +/* SMP-safe */
> > +static int
> > +famfs_mknod(
> > +	struct mnt_idmap *idmap,
> > +	struct inode     *dir,
> > +	struct dentry    *dentry,
> > +	umode_t           mode,
> > +	dev_t             dev)
> > +{
> > +	struct inode *inode = famfs_get_inode(dir->i_sb, dir, mode, dev);
> > +	int error           = -ENOSPC;
> > +
> > +	if (inode) {
> 
> As below. I would flip it for cleaner code/ shorter indent etc.

Nice - done.

> 
> > +		struct timespec64       tv;
> > +
> > +		d_instantiate(dentry, inode);
> > +		dget(dentry);	/* Extra count - pin the dentry in core */
> > +		error = 0;
> > +		tv = inode_set_ctime_current(inode);
> > +		inode_set_mtime_to_ts(inode, tv);
> > +		inode_set_atime_to_ts(inode, tv);
> > +	}
> > +	return error;
> > +}
> > +
> > +static int famfs_mkdir(
> > +	struct mnt_idmap *idmap,
> > +	struct inode     *dir,
> > +	struct dentry    *dentry,
> > +	umode_t           mode)
> > +{
> > +	int retval = famfs_mknod(&nop_mnt_idmap, dir, dentry, mode | S_IFDIR, 0);
> > +
> > +	if (!retval)
> > +		inc_nlink(dir);
> 
> Copy local style, so fine if this is common pattern, otherwise I'd go for
> consistent error cases out of line as easier for us sleepy caffeine 
> deprived reviewers.
> 
> 
> 	if (retval)
> 		return retval;
> 
> 	inc_nlink(dir);
> 
> 	return 0;

Agree, done.

> > +
> > +	return retval;
> > +}
> > +
> > +static int famfs_create(
> > +	struct mnt_idmap *idmap,
> > +	struct inode     *dir,
> > +	struct dentry    *dentry,
> > +	umode_t           mode,
> > +	bool              excl)
> > +{
> > +	return famfs_mknod(&nop_mnt_idmap, dir, dentry, mode | S_IFREG, 0);
> > +}
> > +
> > +static int famfs_symlink(
> > +	struct mnt_idmap *idmap,
> > +	struct inode     *dir,
> > +	struct dentry    *dentry,
> > +	const char       *symname)
> > +{
> > +	struct inode *inode;
> > +	int error = -ENOSPC;
> > +
> > +	inode = famfs_get_inode(dir->i_sb, dir, S_IFLNK | 0777, 0);
> 	if (!inode)
> 		return -ENOSPC;
> 
> > +	if (inode) {
> > +		int l = strlen(symname)+1;
> > +
> > +		error = page_symlink(inode, symname, l);
> 	if (error) {
> 		iput(inode);
> 		return error;
> 	}
> 	
> 	...
> 

Right, I like it. This was some tortured conditioning, which came from
somewhere. I deny responsibility :D

Thanks,
John