xfs
[Top] [All Lists]

Re: [patch 3/3] Fix XFS_IOC_FSBULKSTAT{,_SINGLE} and XFS_IOC_FSINUMBERS

To: mmarek@xxxxxxx
Subject: Re: [patch 3/3] Fix XFS_IOC_FSBULKSTAT{,_SINGLE} and XFS_IOC_FSINUMBERS in compat mode
From: David Chinner <dgc@xxxxxxx>
Date: Thu, 28 Jun 2007 13:49:57 +1000
Cc: xfs@xxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx
In-reply-to: <20070619132726.893544847@suse.cz>
References: <20070619132549.266927601@suse.cz> <20070619132726.893544847@suse.cz>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.4.2.1i
On Tue, Jun 19, 2007 at 03:25:52PM +0200, mmarek@xxxxxxx wrote:
> * 32bit struct xfs_fsop_bulkreq has different size and layout of
>   members, no matter the alignment. Move the code out of the #else
>   branch (why was it there in the first place?). Define _32 variants of
>   the ioctl constants.
> * 32bit struct xfs_bstat is different because of time_t and on
>   i386 becaus of different padding. Create a new formatter
>   xfs_bulkstat_one_compat() that takes care of this. To do this, we need
>   to make xfs_bulkstat_one_iget() and xfs_bulkstat_one_dinode()
>   non-static.
> * i386 struct xfs_inogrp has different padding. Introduce a similar
>   "formatter" mechanism for xfs_inumbers: the native formatter is just a
>   copy_to_user, the compat formatter takes care of the different layout

Oh, wow, that is so much nicer than the first version, Michal. ;)

Still, I think there's possibly one further revision:

> +static int xfs_bulkstat_one_compat(
> +     xfs_mount_t     *mp,            /* mount point for filesystem */
> +     xfs_ino_t       ino,            /* inode number to get data for */
> +     void            __user *buffer, /* buffer to place output in */
> +     int             ubsize,         /* size of buffer */
> +     void            *private_data,  /* my private data */
> +     xfs_daddr_t     bno,            /* starting bno of inode cluster */
> +     int             *ubused,        /* bytes used by me */
> +     void            *dibuff,        /* on-disk inode buffer */
> +     int             *stat)          /* BULKSTAT_RV_... */

Hmmm - this is almost all duplicated code. It's pretty much what I
described, but maybe not /quite/ what I had in mind here. It's a
*big* improvement on the first version, but it seems now that the
only real difference xfs_bulkstat_one() and
xfs_bulkstat_one_compat() is copy_to_user() vs the discrete put_user
calls.

I think we can remove xfs_bulkstat_one_compat() completely by using
the same method you used with the xfs_inumber_fmt functions. That
would mean the only duplicated code is the initial ioctl handling
(which we can't really avoid). It would also mean that there is no
need to make xfs_bulkstat_one_iget() and xfs_bulkstat_one_dinode()
non-static. Your thoughts?

Other than that possible improvement, this looks really good.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group


<Prev in Thread] Current Thread [Next in Thread>