On Wed, Oct 9, 2024 at 5:16 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Wed, Oct 9, 2024 at 11:40 AM Jan Kara <jack@xxxxxxx> wrote:
> >
> > 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.
> >mask
> > > +#define FILEID_USER_TYPE_IS_VALID(type) \
> > > + (FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS)
> >
> > The macro name is confusing
>
> Confusing enough to hide the fact that it was negated in v2...
>
> > because it speaks about type but actually
> > checks flags. Frankly, I'd just fold this in the single call site to make
> > things obvious.
>
> Agree. but see below...
>
> >
> > > 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...
>
> That seems like a good idea, but when I look at:
>
> /* we ask for a non connectable maybe decodeable file handle */
> retval = exportfs_encode_fh(path->dentry,
> (struct fid *)handle->f_handle,
> &handle_dwords, fh_flags);
> handle->handle_type = retval;
> /* convert handle size to bytes */
> handle_bytes = handle_dwords * sizeof(u32);
> handle->handle_bytes = handle_bytes;
> if ((handle->handle_bytes > f_handle.handle_bytes) ||
> (retval == FILEID_INVALID) || (retval < 0)) {
> /* As per old exportfs_encode_fh documentation
> * we could return ENOSPC to indicate overflow
> * But file system returned 255 always. So handle
> * both the values
> */
> if (retval == FILEID_INVALID || retval == -ENOSPC)
> retval = -EOVERFLOW;
> /*
>
> I realize that we actually return negative values in handle_type
> (-ESTALE would be quite common).
>
> So we probably don't want to make handle_type unsigned now,
> but maybe we do want a macro:
>
> #define FILEID_IS_USER_TYPE_VALID(type) \
> ((type) >= 0 && \
> !(FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS))
>
> that will be used for the assertions instead of assuming that
> FILEID_USER_FLAGS_MASK includes the sign bit.
>
> huh?
Scratch that, the assertions check for any user flags at all.
I will just open code type < 0 there as well.
Thanks,
Amir.
