On Fri 28-04-23 08:15:50, Jeff Layton wrote:

On Fri, 2023-04-28 at 13:40 +0200, Jan Kara wrote:

quoted

On Thu 27-04-23 22:11:46, Amir Goldstein wrote:

quoted

On Thu, Apr 27, 2023 at 7:36 PM Jeff Layton [off-list ref] wrote:

quoted

quoted

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[];
	
};

Well, there's also always the option of having:

struct file_handle {
	unsigned int handle_bytes_flags;
	int handle_type;
	unsigned char f_handle[];
};

And then helper functions like:

unsigned int file_handle_bytes(struct file_handle *handle)
{
	return handle->handle_bytes_flags & 0xffff;
}

unsigned int file_handle_flags(struct file_handle *handle)
{
	return handle->handle_bytes_flags >> 16;
}

(and similar for forming the handle_bytes_flags value).

That is well defined and compatible across architectures and compilers and
bearable (although a bit clumsy).

...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.

Yeah, you're right. But at this point it can serve us well by making sure
there's no userspace passing absurdly high handle_bytes ;).

								Honza

-- 
Jan Kara [off-list ref]
SUSE Labs, CR