On Tue 08-10-24 15:11:39, Amir Goldstein wrote:
> On Tue, Oct 8, 2024 at 1:07 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >
> > On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote:
> > > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> > > >
> > > > > open_by_handle_at(2) does not have AT_ flags argument, but also, I find
> > > > > it more useful API that encoding a connectable file handle can mandate
> > > > > the resolving of a connected fd, without having to opt-in for a
> > > > > connected fd independently.
> > > >
> > > > This seems the best option to me too if this api is to be added.
> > >
> > > Thanks.
> > >
> > > Jeff, Chuck,
> > >
> > > Any thoughts on this?
> > >
> >
> > Sorry for the delay. I think encoding the new flag into the fh itself
> > is a reasonable approach.
> >
>
> Adding Jan.
> Sorry I forgot to CC you on the patches, but struct file_handle is officially
> a part of fanotify ABI, so your ACK is also needed on this change.
Thanks. I've actually seen this series on list, went "eww bitfields, let's
sleep to this" and never got back to it.
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 96b62e502f71..3e60bac74fa3 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -159,8 +159,17 @@ struct fid {
> #define EXPORT_FH_CONNECTABLE 0x1 /* Encode file handle with parent */
> #define EXPORT_FH_FID 0x2 /* File handle may be non-decodeable */
> #define EXPORT_FH_DIR_ONLY 0x4 /* Only decode file handle for a
> directory */
> -/* Flags allowed in encoded handle_flags that is exported to user */
> -#define EXPORT_FH_USER_FLAGS (EXPORT_FH_CONNECTABLE | EXPORT_FH_DIR_ONLY)
> +
> +/* Flags supported in encoded handle_type that is exported to user */
> +#define FILEID_USER_FLAGS_MASK 0xffff0000
> +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK)
> +
> +#define FILEID_IS_CONNECTABLE 0x10000
> +#define FILEID_IS_DIR 0x40000
> +#define FILEID_VALID_USER_FLAGS (FILEID_IS_CONNECTABLE | FILEID_IS_DIR)
FWIW I prefer this variant much over bitfields as their binary format
depends on the compiler which leads to unpleasant surprises sooner rather
than later.
> +#define FILEID_USER_TYPE_IS_VALID(type) \
> + (FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS)
The macro name is confusing because it speaks about type but actually
checks flags. Frankly, I'd just fold this in the single call site to make
things obvious.
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index cca7e575d1f8..6329fec40872 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1071,8 +1071,7 @@ struct file {
>
> struct file_handle {
> __u32 handle_bytes;
> - int handle_type:16;
> - int handle_flags:16;
> + int handle_type;
Maybe you want to make handle_type unsigned when you treat it (partially)
as flags? Otherwise some constructs can lead to surprises with sign
extension etc...
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
