Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/sys/unix: add name_to_handle_at and open_by_handle_at #30537

Closed
dominikh opened this issue Mar 3, 2019 · 8 comments
Closed

x/sys/unix: add name_to_handle_at and open_by_handle_at #30537

dominikh opened this issue Mar 3, 2019 · 8 comments
Labels
FeatureRequest FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Linux
Milestone

Comments

@dominikh
Copy link
Member

dominikh commented Mar 3, 2019

I would like to add name_to_handle_at and open_by_handle_at to x/sys/unix for Linux, but I am not sure how to structure the functions and types.

Their canonical definitions are

int name_to_handle_at(int dirfd, const char *pathname, struct file_handle *handle, int *mount_id, int flags);
int open_by_handle_at(int mount_fd, struct file_handle *handle, int flags);

struct file_handle {
  unsigned int  handle_bytes;   /* Size of f_handle [in, out] */
  int           handle_type;    /* Handle type [out] */
  unsigned char f_handle[0];    /* File identifier (sized by caller) [out] */
};

My issue is with the open-ended nature of file_handle. The name_to_handle_at syscall expects the user to allocate a file_handle of sufficient size (usually in a loop to determine the correct size) and to put its size in handle_bytes. And while the value of f_handle is opaque, the user may want to access it, for example to transmit it over the network.

I can imagine two possible ways of designing the API, and I am hoping for a third one.

The first option is to stick 1:1 to the C definitions. This would require use of unsafe.Pointer on the caller's side to convert the backing array into the appropriate type, as well as to access the value of f_handle. And due to #14440, the existence of the f_handle field is obscured. Furthermore, I am not sure how this interacts with the GC. If we back a 16 byte struct with an N bytes byte slice, can we be sure GC will never collect part of the memory?

The second option is to split the file_handle argument in two, one for the part of the struct that Go actually generates, and a byte slice for the f_handle, giving us the following:

func NameToHandleAt(..., handle *FileHandle, fHandle []byte, ...)

This, however, begs the question: who's responsibility is it to ensure that handle.Bytes and len(fHandle) match, and what do we do if they don't?

Is there a third option I'm missing?

/cc @ianlancetaylor @tklauser

@dominikh dominikh added this to the Unreleased milestone Mar 3, 2019
@ianlancetaylor
Copy link
Contributor

I don't see a reason to split up the C file_handle struct. I would be inclined to write it like this:

type FileHandle struct { unexported fields }

func NameToHandleAt(dirfd int, path string, flags int) (handle FileHandle, mountID int, err error)

func OpenByHandleAt(mountID int, handle FileHandle, flags int) (err error)

It's odd that in C the two functions use a different order for handle and mount_id. I repeated that above but in Go we could consider keeping them consistent.

@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 6, 2019
@dominikh
Copy link
Member Author

dominikh commented Mar 7, 2019

type FileHandle struct { unexported fields }

You left out the part I am actually struggling with. How can we structure FileHandle so that it a) allows Go users access to handle_type as well as f_handle b) has the same memory layout as in C, so that it can be passed to other functions accepting handles.

Are you suggesting that we do not maintain the memory layout and handle conversions between two representations transparently?

@ianlancetaylor
Copy link
Contributor

Why do Go users need access to handle_type? The man page says the whole handle is basically opaque. It doesn't provide any meaning for the handle_type field.

To preserve the same memory layout as C I suggest that the "unexported fields" be simply a []byte that is the C memory layout. Then we can add a Bytes accessor method for people who need to pass it to C.

For that matter we could have a Handle method that returns handle_type if that is useful.

Probably I am missing something.

@dominikh
Copy link
Member Author

dominikh commented Mar 7, 2019

[]byte and Bytes will do. Somehow I missed that option. Thanks. You're correct that users don't need access to the individual fields, only the data as a whole (for example to send it across the network in a user space NFS server)

@bradfitz
Copy link
Contributor

@ianlancetaylor, I need it for writing tests for a FUSE filesystem.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/172584 mentions this issue: unix: add unexported name_to_handle_at and open_by_handle_at types & wrappers

gopherbot pushed a commit to golang/sys that referenced this issue Apr 22, 2019
…wrappers

No usable change for users in this CL; just auto-generated syscall
wrapper funcs & types.

The next CL will have hand-written code adding the nice Go API around
these unexported symbols. (as outlined in the comment at
https://golang.org/issue/30537#issuecomment-470284573)

Updates golang/go#30537

Change-Id: I5e34df517efcf509fff97f670425500cc6608d59
Reviewed-on: https://go-review.googlesource.com/c/sys/+/172584
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Tobias Klauser <[email protected]>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/173357 mentions this issue: unix: add FileHandle, NewFileHandle, NameToHandleAt, OpenByHandleAt

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/174077 mentions this issue: unix: skip TestOpenByHandleAt if name_to_handle_at not supported

gopherbot pushed a commit to golang/sys that referenced this issue Apr 26, 2019
Updates golang/go#30537

Change-Id: Ica526a4bc689af594b0e91c988631bf9987a6119
Reviewed-on: https://go-review.googlesource.com/c/sys/+/174077
Run-TryBot: Ian Lance Taylor <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
@golang golang locked and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Linux
Projects
None yet
Development

No branches or pull requests

4 participants