Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Apr 28, 2023 at 3:15 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Fri, 2023-04-28 at 13:40 +0200, Jan Kara wrote:
> > On Thu 27-04-23 22:11:46, Amir Goldstein wrote:
> > > On Thu, Apr 27, 2023 at 7:36 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > > > > There is also a way to extend the existing API with:
> > > > >
> > > > > Perhstruct file_handle {
> > > > >         unsigned int handle_bytes:8;
> > > > >         unsigned int handle_flags:24;
> > > > >         int handle_type;
> > > > >         unsigned char f_handle[];
> > > > > };
> > > > >
> > > > > AFAICT, this is guaranteed to be backward compat
> > > > > with old kernels and old applications.
> > > > >
> > > >
> > > > That could work. It would probably look cleaner as a union though.
> > > > Something like this maybe?
> > > >
> > > > union {
> > > >         unsigned int legacy_handle_bytes;
> > > >         struct {
> > > >                 u8      handle_bytes;
> > > >                 u8      __reserved;
> > > >                 u16     handle_flags;
> > > >         };
> > > > }
> > >
> > > I have no problem with the union, but does this struct
> > > guarantee that the lowest byte of legacy_handle_bytes
> > > is in handle_bytes for all architectures?
> > >
> > > That's the reason I went with
> > >
> > > struct {
> > >          unsigned int handle_bytes:8;
> > >          unsigned int handle_flags:24;
> > > }
> > >
> > > Is there a problem with this approach?
> >
> > As I'm thinking about it there are problems with both approaches in the
> > uAPI. The thing is: A lot of bitfield details (even whether they are packed
> > to a single int or not) are implementation defined (depends on the
> > architecture as well as the compiler) so they are not really usable in the
> > APIs.
> >
> > With the union, things are well-defined but they would not work for
> > big-endian architectures. We could make the structure layout depend on the
> > endianity but that's quite ugly...
> >
>
> Good point. Bitfields just have a bad code-smell anyway.
>
> Another idea would be to allow someone to set handle_bytes to a
> specified value that's larger than the current max of 128 (maybe ~0 or
> something), and use that as an indicator that this is a v2 struct.
>
> So the v2 struct would look something like:
>
> struct file_handle_v2 {
>         unsigned int    legacy_handle_bytes;    // always set to ~0 or whatever
>         unsigned int    flags;
>         int             handle_type;
>         unsigned int    handle_bytes;
>         unsigned char   f_handle[];
>
> };

The three of us are racing with proposals of crazy solutions ;)

I was going to propose:

struct file_handle_v2 {
        union {
                u32 legacy_handle_bytes;
                struct {
                        u16 handle_bytes;
                        u8 handle_flags;
                        u8 reserved;
                };
        };
        int handle_type;
        unsigned char f_handle[];
};

which is similar to your first proposal, but the way to use it would be:

static inline int file_handle_bytes(struct file_handle_v2 *handle)
{
        return (handle->legacy_handle_bytes < MAX_HANDLE_SZ) ?
                handle->legacy_handle_bytes : handle->handle_bytes;
}

I think this works for both LE and BE, because non zero handle_flags
would taint legacy_handle_bytes either way.

>
> ...but now I'm wondering...why do we return -EINVAL when
> f_handle.handle_bytes is > MAX_HANDLE_SZ? Is it really wrong for the
> caller to allocate more space for the resulting file_handle than will be
> needed? That seems wrong too. In fact, name_to_handle_at(2) says:
>
> "The constant MAX_HANDLE_SZ, defined in <fcntl.h>, specifies the maximum
> expected size for a file handle.  It  is  not guaranteed upper limit as
> future filesystems may require more space."
>
> So by returning -EINVAL when handle_bytes is too large, we're probably
> doing the wrong thing there.

Yeh it is very strange, but now it is very convenient too,
so no reason to fix it retroactively.

With the file_handle_v2 I proposed above, new handle_bytes is 16bit
so we won't need to error on large buffer size when using file_handle_v2.

Another odd thing about struct file_handle is that it is not actually
defined in include/uapi header files, although it is defined in the man page
of name_to_handle_at(2).
That is another thing that we can fix with file_handle_v2.

>
> Using an AT_* flag may be the best plan, actually.

It's true, but perhaps AT_HANDLE_V2 and then the
handle_flags could be easily extended later.

Thanks,
Amir.




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux