Re: [PATCH 1/8] quota: Allow to pass mount path to quotactl

[Sascha Hauer <s.hauer@pengutronix.de>]

On Fri, Jan 22, 2021 at 05:16:58PM +0000, Christoph Hellwig wrote:
> On Fri, Jan 22, 2021 at 04:15:29PM +0100, Sascha Hauer wrote:
> > This patch introduces the Q_PATH flag to the quotactl cmd argument.
> > When given, the path given in the special argument to quotactl will
> > be the mount path where the filesystem is mounted, instead of a path
> > to the block device.
> > This is necessary for filesystems which do not have a block device as
> > backing store. Particularly this is done for upcoming UBIFS support.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> I hate overloading quotactl even more.  Why not add a new quotactl_path
> syscall instead?

We can probably do that. Honza, what do you think?

> 
> > +static struct super_block *quotactl_sb(dev_t dev, int cmd)
> >  {
> >  	struct super_block *sb;
> >  	bool excl = false, thawed = false;
> >  
> >  	if (quotactl_cmd_onoff(cmd)) {
> >  		excl = true;
> > @@ -901,12 +887,50 @@ static struct super_block *quotactl_block(const char __user *special, int cmd)
> >  		goto retry;
> >  	}
> >  	return sb;
> > +}
> > +
> > +/*
> > + * look up a superblock on which quota ops will be performed
> > + * - use the name of a block device to find the superblock thereon
> > + */
> > +static struct super_block *quotactl_block(const char __user *special, int cmd)
> > +{
> > +#ifdef CONFIG_BLOCK
> > +	struct filename *tmp = getname(special);
> > +	int error;
> > +	dev_t dev;
> >  
> > +	if (IS_ERR(tmp))
> > +		return ERR_CAST(tmp);
> > +	error = lookup_bdev(tmp->name, &dev);
> > +	putname(tmp);
> > +	if (error)
> > +		return ERR_PTR(error);
> > +
> > +	return quotactl_sb(dev, cmd);
> >  #else
> >  	return ERR_PTR(-ENODEV);
> >  #endif
> 
> Normal kernel style would be to keep the ifdef entirely outside the
> function.

It has been like this before, so changing this should be done in an
extra patch. Not sure if it's worth it though.

> 
> > +static struct super_block *quotactl_path(const char __user *special, int cmd)
> > +{
> > +	struct super_block *sb;
> > +	struct path path;
> > +	int error;
> > +
> > +	error = user_path_at(AT_FDCWD, special, LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
> > +			   &path);
> 
> This adds an overly long line.

ok, will change.

> 
> > +	if (error)
> > +		return ERR_PTR(error);
> > +
> > +	sb = quotactl_sb(path.mnt->mnt_sb->s_dev, cmd);
> 
> I think quotactl_sb should take the superblock directly.  This will
> need a little refactoring of user_get_super, but will lead to much
> better logic.

What do you mean by "take"? Take the superblock as an argument to
quotactl_sb() or take a reference to the superblock?
Sorry, I don't really get where you aiming at.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |