Andreas Dilger <[email protected]> wrote:

> > + case S_IFCHR: printf(" character special file\n"); ft = 'c'; break;
>
> This will overflow 80 columns. Could use just "character special"?
>
> > + case S_IFDIR: printf(" directory\n"); ft = 'd'; break;
> > + case S_IFBLK: printf(" block special file\n"); ft = 'b'; break;
> > + case S_IFREG: printf(" regular file\n"); ft = '-'; break;
> > + case S_IFLNK: printf(" symbolic link\n"); ft = 'l'; break;
>
> Having these all as single-word items would make it easier to parse for
> test programs. Same with "IO Block:" -> "Blocksize:". Not a huge deal.

Try doing "stat /dev/null".

David

2016-11-24 08:14:20

by Michael Kerrisk (man-pages)

[permalink] [raw]

Subject: Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available [ver #3]

Hi Andreas,

On 11/23/2016 11:57 PM, Andreas Dilger wrote:
> On Nov 23, 2016, at 1:37 AM, Michael Kerrisk <[email protected]> wrote:
>>

[...]

>>> ===============
>>> NEW SYSTEM CALL
>>> ===============
>>>
>>> The new system call is:
>>>
>>> int ret = statx(int dfd,
>>> const char *filename,
>>> unsigned int flags,
>>
>> In the 0/4 of this patch series, this argument is called 'atflags'.
>> These should be consistent. 'flags' seems correct to me.
>
> Given that there are a number of different flags and masks in use for
> this syscall, naming this field "atflags" makes it more clear what it
> is used for.

Well, yes, but in all of the "at" calls the argument is just flags.
So, better to be consistent in that dimension, I'd say, and then
make sure we perhaps have meaningfil names for the fiellds, as you suggest next:

>>> unsigned int mask,
>
> Similarly, naming this field "request_mask" would also be more clear,
> and matches what is used elsewhere in the code.
>
> That said, I don't care enough about this detail to request a patch refresh,
> but it would be useful for the man pages.

--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

2016-12-04 04:01:29

by Al Viro

[permalink] [raw]

Subject: Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available [ver #3]

On Wed, Nov 23, 2016 at 12:55:51AM +0000, David Howells wrote:

> -int vfs_getattr_nosec(struct path *path, struct kstat *stat)
> +int vfs_xgetattr_nosec(struct path *path, struct kstat *stat)

const struct path *, please. Especially since we feed &file->f_path to
it. And yes, I realize that the same applies to vfs_getattr() in the
current tree.

> +int vfs_xgetattr(struct path *path, struct kstat *stat)

Ditto.

> +int vfs_getattr(struct path *path, struct kstat *stat)

Again.

> +#define __put_timestamp(kts, uts) ( \
> + __put_user(kts.tv_sec, uts.tv_sec ) || \
> + __put_user(kts.tv_nsec, uts.tv_nsec ) || \
> + __put_user(0, uts.__reserved ))

static inline.

> +#define KSTAT_ATTR_FS_IOC_FLAGS 0x00000874 /* Attrs corresponding to FS_*_FL flags */

Err... An explicit hex constant?

2016-12-04 04:38:16

by Al Viro

[permalink] [raw]

Subject: Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available [ver #3]

On Wed, Nov 23, 2016 at 12:55:51AM +0000, David Howells wrote:

> + * vfs_xgetattr - Get the enhanced basic attributes of a file
> + * @path: The file of interest
> + * @stat: Where to return the statistics
> + *
> + * Ask the filesystem for a file's attributes. The caller must have preset
> + * stat->request_mask and stat->query_flags to indicate what they want.
> + *
> + * If the file is remote, the filesystem can be forced to update the attributes
> + * from the backing store by passing AT_FORCE_ATTR_SYNC in query_flags or can
> + * suppress the update by passing AT_NO_ATTR_SYNC.
> + *
> + * Bits must have been set in stat->request_mask to indicate which attributes
> + * the caller wants retrieving. Any such attribute not requested may be
> + * returned anyway, but the value may be approximate, and, if remote, may not
> + * have been synchronised with the server.

I understand wanting to avoid extra arguments, but you are asking for trouble
with that sort of calling conventions. Verifying that all call chains have
these fields initialized is bloody unpleasant and it *is* going to break,
especially since the rules are "you need to initialize it for vfs_xgetattr(),
but not for vfs_getattr()" - the names are similar enough for confusion,
and that's not the only such pair.

2016-12-04 16:05:08

by Al Viro

[permalink] [raw]

Al Viro <[email protected]> wrote:

> > I understand wanting to avoid extra arguments, but you are asking for
> > trouble with that sort of calling conventions. Verifying that all call
> > chains have these fields initialized is bloody unpleasant and it *is*
> > going to break, especially since the rules are "you need to initialize it
> > for vfs_xgetattr(), but not for vfs_getattr()" - the names are similar
> > enough for confusion, and that's not the only such pair.
>
> BTW, after some grepping: you seem to leave ecryptfs_getattr() as-is, which
> means that flags it had been passed by caller in *stat are not seen by
> underlying fs. Looks fishy...

I have patches for some other filesystems as well, but I'm trying to nail the
core down before whipping those back into shape.

David