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?
Thanks,
Amir.
