Change https://go.dev/cl/484995 mentions this issue: unix: add Fsconfig syscall on linux

@ianlancetaylor , we talked to @AlexeyPerevalov and decided to create a new review for this (https://go-review.googlesource.com/c/sys/+/484995), this fixed issues you mentioned on the previous one, PTAL

This is a very complicated system call. Please document the suggested Go API in this issue so that people can discuss it. In particular I wonder whether value should be string or []byte. Thanks.

I wonder whether value should be string or []byte

I wrote a version few weeks ago with []byte (IlyaHanov/sys@aeaec22), but I didn't test it so much, it worked for my own cases. As I can see from the previous review rounds there might be other cases that this version doesn't handle, of course, it will differ with string and going to be more complicated.

I can suggest to use interface{} for value:

• In case of FSCONFIG_BINARY do v, ok := value.([]byte)
• In case of FSCONFIG_SET_PATH AND FSCONFIG_SET_PATH_EMPTY AND FSCONFIG_SET_STRING do v, ok := value.(string)
• In other cases value is ignored and nil passed to the kernel

Fsconfig(fd int, cmd uint, key string, value interface{}, aux int) (err error)

Hi, thanks for moving this forward, I too would like to use the syscall. What is the next step for the review, and can I help?

This is a very complicated system call. Please document the suggested Go API in this issue so that people can discuss it. In particular I wonder whether value should be string or []byte. Thanks.

@stagnation AFAIR, the next step is to define the interface, I've suggested few of them:

• Fsconfig(fd int, cmd uint, key string, value string, aux int) (err error)
• Fsconfig(fd int, cmd uint, key string, value []byte, aux int) (err error)
• Fsconfig(fd int, cmd uint, key string, value interface{}, aux int) (err error)

I think it's possible to move it forward if people and maintainers will be agreed on the interface. It's not a problem to implement any of them. WDYT?

I do not have a strong opinion, I can see either option working fine. My main use case is mountat that just uses the SET_STRING command, which is convenient to call with a string argument, but interface{} works too, and []byte is not too much of a hassle.

In short:

 func mountat(dfd int, fstype, source, mountname string) (int, error) {
     // Mounts the `source` filesystem on `mountname` inside a directory
     // given as `dfd` file descriptor.
     // Using the `fstype` filesystem type.
     fd, err := unix.Fsopen(fstype, unix.FSOPEN_CLOEXEC)
     err = fsconfig(fd, FSCONFIG_SET_STRING, "source", source, 0)
     err = fsconfig(fd, FSCONFIG_CMD_CREATE, "", "", 0)
     mfd, err := unix.Fsmount(fd, unix.FSMOUNT_CLOEXEC, unix.MS_NOEXEC)
     err = unix.MoveMount(mfd, "", dfd, mountname, unix.MOVE_MOUNT_F_EMPTY_PATH)

     return mfd, nil

@ianlancetaylor would you mind to merge version that uses interface{} (I can prepare that on days) it looks the most attractive for users, but as @stagnation mentioned it seems that any of solutions can be adopted by the user if needed?

Go is, of course, a strongly typed language. I don't think we should drop back to empty interface. Besides the loss of type checking, it's harder to avoid an allocation when using an interface value.

It seems to me that we should have multiple FsConfig functions. Is there a reason that that is a bad idea?

It seems to me that we should have multiple FsConfig functions. Is there a reason that that is a bad idea?

@ianlancetaylor I don't think that this is a bad idea, semantically this is the same, so we can have one version for string (we can emulate NULL using empty "" string) and another one for []byte. There's only one thing I do have doubts about -- fsconfig_set_{fd,flag} have both versions and this may look counterintuitive (the same for the rest, string version may potentially use fsconfig_set_binary).

Maybe it's better to have versions for all of {fsconfig_set_flag, fsconfig_set_string, fsconfig_set_binary, fsconfig_set_path, fsconfig_set_path_empty, fsconfig_set_fd} and name them literally the same way as cmd (ofc, in Go-style)?

Sounds plausible. I'd like to see the API.

Sounds plausible. I'd like to see the API.

I guess API may look like this (description i.e. comments are just little explanation -- not the API docs now):

// fsconfig_set_flag
FsconfigSetFlag(fd int, key string) (err error)

// fsconfig_set_string
FsconfigSetString(fd int, key string, value string) (err error)

// fsconfig_set_binary (aux is the size of value buffer and we can calculate that inside the call)
FsconfigSetBinary(fd int, key string, value []byte) (err error)

// fsconfig_set_path
FsconfigSetPath(fd int, key string, path string, atfd int) (err error)

// fsconfig_set_path_empty
FsconfigSetPathEmpty(fd int, key string, path string, atfd int) (err error)

// fsconfig_set_fd
FsconfigSetFd(fd int, key string, value int) (err error)

// fsconfig_cmd_create
FsconfigCreate(fd int) (err error)

// fsconfig_cmd_reconfigure
FsconfigReconfigure(fd int) (err error)

@ianlancetaylor WDYT?

Looks good to me.
But, I think the argument names to FsconfigSetFd are a little weird, I think it is better if fd is the same for all the variants.
But it is hard to find a good name for the argument file descriptor.

https://github.com/torvalds/linux/blob/master/fs/fsopen.c#L346 . Here it is just "aux", the polymorphic argument

(*) fsconfig_set_fd: An open file descriptor is specified. @_value must be
NULL and @Aux indicates the file descriptor.

I think it is better if fd is the same for all the variants.
But it is hard to find a good name for the argument file descriptor.

@stagnation we can call it e.g. value (since it is FsconfigSetFd it should be understandable):

FsconfigSetFd(fd int, key string, value int) (err error)

For sure, I think we should explain that in docs for this function.

Thanks, turning this into a proposal.

Thanks, turning this into a proposal.

Great, I'll prepare a patch-set this week. Thanks!

Patch-set is ready for review at https://go-review.googlesource.com/c/sys/+/484995

ping @ianlancetaylor for review

This is waiting in the proposal process. Thanks.

Adding to proposal process. Proposal is #59537 (comment)

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

Proposal details in #59537 (comment).

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Proposal details in #59537 (comment).

ping @ianlancetaylor for review