On Wed, Nov 29, 2017 at 03:48:50PM -0700, Allison Henderson wrote:
>
>
> On 11/29/2017 02:37 PM, Dave Chinner wrote:
> >On Tue, Nov 28, 2017 at 12:35:37PM -0800, Darrick J. Wong wrote:
> >>On Fri, Nov 17, 2017 at 11:21:45AM -0700, Allison Henderson wrote:
> >>>This patch adds a new file ioctl to retrieve the parent
> >>>pointer of a given inode
> >>
> >>(Yes, it's time to start talking about actual use cases...)
> >>
> >>At a bare minimum, this is what I pictured for the "return parents of
> >>the open file" ioctl:
> >>
> >>#define XFS_PPTR_MAXNAMELEN 255
> >>
> >>struct xfs_pptr {
> >> u64 pp_ino;
> >> u32 pp_gen;
> >> u8 pp_namelen;
> >> u8 pp_name[XFS_PPTR_MAXNAMELEN];
> >>};
> >
> >That's going to be a different size on 32bit and 64 bit platforms
> >as the structure size is a multiple of 4 bytes, not 8 bytes.
> >That will cause problems and need complex comapt ioctl translation.
> >Better to make pp_namelen a u32 and that will make the structure
> >64 bit aligned and sized on all platforms.
> >
> >I'd allow more than u8 for the namelen. Yes, while we currently
> >allow on 255 bytes for a name, it would make more sense to
> >use a u32 here so that the structure size is a multiple of it's
> >alignment rather than having a 4 byte hole in the array we don't
> >fill out....
> >
> >>
> >>/* return parents of the handle, instead of the open fd */
> >>#define XFS_PPTR_FLAG_HANDLE (1u << 0)
> >>
> >>struct xfs_pptr_info {
> >> struct xfs_fsop_handlereq pi_handle;
> >> struct xfs_attrlist_cursor pi_cursor;
> >> u32 pi_flags;
> >> u32 pi_reserved;
> >> u32 pi_ptrs_size;
> >> u32 pi_ptrs_used;
> >> u64 pi_reserved2[6];
> >> struct xfs_pptr pi_ptrs[0];
> >>};
> >
> >I thought gcc had started doing weird things with variable size
> >array declarations like this (i.e. pi_ptrs[0]) because the exact
> >behaviour is not defined in the C standard. i.e. we need to avoid
> >adding new declarations that do this...
>
> Oh, I think there's a few places in the set where I have
> declarations like that.
Yup, there are quite a few, but IIRC we can't rely on them working
as they do right now in future compilers. So I'm pretty sure we need
to avoid these sorts of constructs if we can. Doing something like
this:
struct xfs_pptr_info {
struct xfs_fsop_handlereq pi_handle;
struct xfs_attrlist_cursor pi_cursor;
u32 pi_flags;
u32 pi_reserved;
u32 pi_ptrs_size;
u32 pi_ptrs_used;
u64 pi_reserved2[6];
/*
* An array of struct xfs_pptr follows the header
* information. Use XFS_PPINFO_TO_PP() to access the
* parent pointer array entries.
*/
};
And providing an accessor function:
#define XFS_PPINFO_TO_PP(info, idx) \
(&(((struct xfs_pptr *)((char *)(info) + sizeof(*(info))))[(idx)]))
Will solve the problem.
> Should they be some_array[1]; instead?
That has problems, too. See, for example, commit ffeecc521302 ("xfs:
Fix xfs_attr_leafblock definition"), where gcc completely mangled
the code because it thought it could optimise away bits of the
structure and code that "weren't used".
> >>#define XFS_PPTR_INFO_SIZEOF(ptrs) (sizeof(struct xfs_pptr_info) + \
> >> ((ptrs) * sizeof(struct xfs_pptr)));
> >>static inline struct xfs_pptr_info *
> >>xfs_pptr_alloc(
> >> size_t nr_ptrs)
> >>{
> >> struct xfs_pptr_info *ppi;
> >>
> >> ppi = malloc(XFS_PPTR_INFO_SIZEOF(nr_ptrs));
> >> if (!ppi)
> >> return NULL;
> >> memset(ppi, 0, XFS_PPTR_INFO_SIZEOF(nr_ptrs));
> >> ppi->pi_ptrs_size = nr_ptrs;
> >> return ppi;
> >>}
> >>
> >>With the following example userspace program (that does no checking
> >>whatsoever):
> >>
> >>int main(int argc, char *argv[])
> >>{
> >> struct xfs_pptr_info *ppi;
> >> struct xfs_pptr *pp;
> >> int fd;
> >>
> >> fd = open(argv[1], O_RDONLY);
> >> ppi = xfs_pptr_alloc(32);
> >>
> >> while (ioctl(fd, XFS_IOC_GETPPOINTER, ppi) == 0 && ppi->pi_ptrs_used) {
> >> for (i = 0; i < ppi->pi_ptrs_used; i++) {
> >> printf("%llu:%u -> %s\n",
> >> ppi->pi_ptrs[i].pp_ino,
> >> ppi->pi_ptrs[i].pp_gen,
> >> ppi->pi_ptrs[i].pp_name);
And this becomes:
for (i = 0; i < ppi->pi_ptrs_used; i++) {
pp = XFS_PPINFO_TO_PP(ppi, i);
printf("%llu:%u -> %s\n", pp->pp_ino, pp->pp_gen,
pp->pp_name);
}
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
