Re: [PATCH v10 10/11] quota: Add proper versioning support to fs_quota_stat

[Dave Chinner <david@fromorbit.com>]

On Wed, Jul 10, 2013 at 03:49:50PM -0500, Chandra Seetharaman wrote:
> On Wed, 2013-07-10 at 18:26 +0200, Jan Kara wrote:
> > On Wed 10-07-13 10:55:38, Ben Myers wrote:
> > > Hey Chandra,
> 
> <snip>
> 
> > > > diff --git a/fs/quota/quota.c b/fs/quota/quota.c
> > > > index c7314f1..ae6526e 100644
> > > > --- a/fs/quota/quota.c
> > > > +++ b/fs/quota/quota.c
> > > > @@ -207,12 +207,57 @@ static int quota_setxstate(struct super_block *sb, int cmd, void __user *addr)
> > > >  static int quota_getxstate(struct super_block *sb, void __user *addr)
> > > >  {
> > > >  	struct fs_quota_stat fqs;
> > > > -	int ret;
> > > > +	struct fs_quota_stat_v1 fqs_v1;
> > > > +	int ret, size;
> > > > +	void *fqsp;
> > > >  
> > > >  	if (!sb->s_qcop->get_xstate)
> > > >  		return -ENOSYS;
> > > > +
> > > > +	memset(&fqs, 0, sizeof(struct fs_quota_stat));
> > > > +	if (copy_from_user(&fqs, addr, 1)) /* just get the version */
> > > > +		return -EFAULT;
> > > > +
> > > > +	switch (fqs.qs_version) {
> > > > +	case FS_QSTAT_VERSION_2:
> > > > +		size = FS_QSTAT_V2_SIZE;
> > > > +		break;
> > > > +	default:
> > > > +		fqs.qs_version = FS_QSTAT_VERSION;
> > > > +		/* fallthrough */
> > > > +	case FS_QSTAT_VERSION:
> > > > +		size = FS_QSTAT_V1_SIZE;
> > > > +		break;
> > > > +	}
> >   Guys, you cannot really do this. If anyone is using getxstate() with
> > uninitialized buffer (which is fine so far), you cannot suddently start to
> > rely on the contents of qs_version field. That's ABI (and even API)
> > breakage.
> 
> Agree, it is a API breakage.
> 
> Will fix it by adding a new quotactl value as you suggest. 

Please enforce the version number as being valid so we don't
need to add another new interface if we need to change this
structure again. :)

FWIW, we're now going to need a new xfs_quota changes to go along
with this - it will need to use the new interface by default, and
fall back to the existing interface if it gets an error saying the
command does not exist.  We can't actually test the new interface
until we have xfs_quota patches that use it.

And to play Devil's advocate: it is way too late in the merge cycle
to make these sorts of ABI changes to a patch and test/review them
adequately.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs