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[];
};
...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.
Using an AT_* flag may be the best plan, actually.
--
Jeff Layton <jlayton@xxxxxxxxxx>
