Re: [PATCH 2/4] kill struct xfs_mount_args

[Dave Chinner <david@fromorbit.com>]

On Sun, May 25, 2008 at 09:07:41PM +0200, Christoph Hellwig wrote:
> No need to parse the mount option into a structure before applying it
> to struct xfs_mount.
> 
> The content of xfs_start_flags gets merged into xfs_parseargs.  Calls
> inbetween don't care and can use mount members instead of the args
> struct.
> 
> This patch uncovered that the mount option for shared filesystems wasn't
> ever exposed on Linux.  The code to handle it is #if 0'ed in this patch
> pending a decision on this feature.  I'll send a writeup about it to
> the list soon.

Overall is good - cleans up a lot of mess, but a couple of things
need fixing:

> @@ -228,7 +231,9 @@ xfs_parseargs(
>  					this_char);
>  				return EINVAL;
>  			}
> -			strncpy(args->mtpt, value, MAXNAMELEN);
> +			*mtpt = kstrndup(value, MAXNAMELEN, GFP_KERNEL);
> +			if (!*mtpt)
> +				return ENOMEM;

It's a double ptr that dup'd the mtpt= value to successfully.
we check we have a string, but it still count be a null string.

> -	if ((args->flags & XFSMNT_DMAPI) && *args->mtpt == '\0') {
> +	if ((mp->m_flags & XFS_MOUNT_DMAPI) && !mtpt) {

Which means that check is not doing the same thing. Did you mean
to check it was not a null string like the original code
(i.e !**mtpt)?

Also, a comment needs to be made in the function header that mtpt
needs to be freed by the caller.

> @@ -131,9 +130,11 @@ static struct xfs_qmops xfs_qmcore_stub 
>  };
>  
>  int
> -xfs_qmops_get(struct xfs_mount *mp, struct xfs_mount_args *args)
> +xfs_qmops_get(struct xfs_mount *mp)
>  {
> -	if (args->flags & (XFSMNT_UQUOTA | XFSMNT_PQUOTA | XFSMNT_GQUOTA)) {
> +	if (mp->m_qflags & (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE |
> +			    XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE |
> +			    XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE)) {
>  		struct xfs_qmops *ops;
>  
>  		ops = symbol_get(xfs_qmcore_xfs);

I think *QUOTA_ACTIVE implies *QUOTA_ACCT. i.e. the quota can't
be active if we are not accounting it. Hence I think this can
be simplified to :

	if (XFS_IS_QUOTA_RUNNING(mp)) {

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com