x/sys/unix: request for Ioctl*Int32 functions

[ericonr]

One very relevant usage of ioctl functions (at least on Linux) is for querying extended attributes in files. As seen in https://man7.org/linux/man-pages/man2/ioctl_iflags.2.html ,

The type of the argument given to the FS_IOC_GETFLAGS and
FS_IOC_SETFLAGS operations is int *, notwithstanding the
implication in the kernel source file include/uapi/linux/fs.h
that the argument is long *.

What this means is that the attr field in ioctl(fd, FS_IOC_GETFLAGS, &attr); and ioctl(fd, FS_IOC_SETFLAGS, &attr); is always a 32-bit signed integer. This means that using the IoctlGetInt and IoctlSetPointerInt functions is wrong for these values, because Go's int will be 32-bit or 64-bit, depending on the platform's pointer width. It happens to work on little endian platforms, but it will definitely be wrong on 64-bit big endian platforms (so at least ppc64 and some mips, maybe aarch64be? not sure go supports that).

x/sys/unix does provide IoctlGetUint32, but that has the wrong sign, unfortunately.

There is a workaround, which is using IoctlGetUint32 and converting to an int32, and using IoctlSetInt(fd, req, int(uintptr(unsafe.Pointer(attr))) instead of IoctlSetPointerInt, but these are hardly ergonomic.

Therefore, I would suggest providing IoctlGetInt32, IoctlSetPointerInt32 and IoctlSetInt32 functions. If homogenous coverage is desirable, a *Uint32 set could be added as well.

[mknyszek]

CC @ianlancetaylor @tklauser @bradfitz via https://dev.golang.org/owners

[tklauser]

Adding IoctlGetInt32, IoctlSetPointerInt32 and IoctlSetInt32 sounds reasonable to me. Want to send a patch?

If I remember correctly, IoctlGetUint32 was added for IOCTL_VM_SOCKETS_GET_LOCAL_CID about which vsock(7) says: "Get the CID of the local machine. The argument is a pointer to an unsigned int." Do you know of any relevant ioctl variants using unsigned int which would benefit from IoctlSetPointerUint32 and IoctlSetUint32 being added?

[ericonr]

Adding IoctlGetInt32, IoctlSetPointerInt32 and IoctlSetInt32 sounds reasonable to me. Want to send a patch?

Sure, will try to whip one up!

Do you know of any relevant ioctl variants using unsigned int which would benefit from IoctlSetPointerUint32 and IoctlSetUint32 being added?

I didn't, which is why I suggested them more for symmetry than anything else. Some investigation gives me at least:

int ioctl(int fd, FAT_IOCTL_GET_ATTRIBUTES, uint32_t *attr);
int ioctl(int fd, FAT_IOCTL_SET_ATTRIBUTES, uint32_t *attr);
int ioctl(int fd, FAT_IOCTL_GET_VOLUME_ID, uint32_t *id);

which justify IoctlSetPointerUint32.

I have now realized that I'm not sure IoctlSetInt32 and IoctlSetUint32 make sense by themselves, since they will necessarily have to be simple casts from arg to uintptr(arg). Maybe it makes more sense to only add a IoctlSetUint function.

[lhl2617]

Relevant discussion: #14873 (https://go-review.googlesource.com/c/sys/+/26650/)

@robpike mentioned:

My point is that I don't want to see ioctl get helpers to make it look nice. It really doesn't need int and uint and *Termio and *Whatever helpers. The caller can do the conversion. It may be uglier but it will actually be faster, and anyway ioctl is hideous and I prefer not to hide that fact.

[ericonr]

I started looking into implementing these, and it turns out that since 0cf1ed9 the IoctlSetPointerInt function silently truncates its argument to an int32, which would, funnily enough, make it adequate for my usage. @ianlancetaylor pointed out that it should have been named IoctlSetPointerInt32 (though the final function signature was a better fit), but I don't understand why it was dropped in https://go-review.googlesource.com/c/sys/+/150321

Should I open a separate issue to improve docs for this function and/or change its signature? IMO its value argument should already be int32 or it should check if value fits in the max range of int32 and error out otherwise (or it should be deprecated and IoctlSetPointerInt32 should become the correct function?).

[lhl2617]

Hmmm... in #46060 a proposal for IoctlSetIntPtr was made, but this int is just a bare Go int, without any indication whether it's int32 or int64. Should the proposal be for IoctlSetInt32Ptr and IoctlSetInt64Ptr then?

For my use case, I'm compiling against a 32 bit system, so it's a non-issue and I can safely assure that ints (and pointers) are 32-bits, but I think problems may arise with it just being an int. I've also found some kernel ioctls with __kernel_loff_t as an arg, which is long long.

Maybe we should admit that not exposing the raw ioctl call is a mistake now? I've occasionally fell back to writing a syscall ioctl helper function because this package doesn't expose it && I couldn't find a compatible ioctl function signature.

[ianlancetaylor]

The names of the ioctl functions refer to C, so I think it's OK if IoctlSetIntPtr refers to a 32-bit integer type.

Ah, I see the problem: the Go type is int, and people won't realize that this doesn't correspond to the C type. That is a problem. I think we can change the Go type to int32, but happy to hear other opinions.

[lhl2617]

Rather annoyingly we might need four functions: IoctlSet(Ui|I)nt(32|64), which I think is also the case here?

I can see this bloat just continue increasing however. A quick search of
SYS_IOCTL for the Go language on Github gives almost 30k results: https://github.com/search?l=Go&q=SYS_IOCTL&type=code
A lot of these are from vendored/library code, but I'm sure there's a sizeable amount of code written that directly uses the ioctl syscall due to the limitations of the types offered in this package.

github.com/vtolstov/go-ioctl appears to be a "de-facto" ioctl package for when the unix/syscall packages are not enough (it exposes an ioctl function taking in uintptr, i.e. basically anything), it's not as popular, but I've personally used it for large-scale go projects.

ioctls are a hot mess, and if the goal in this package is to try to support every single type, we'd end up playing catch-up with the ever increasing ioctl mess in the kernel.

Adding helpers for the (Ui|I)nt(32|64) types are sensible as most ioctls use this, but I don't know if it's a good idea to let this package continue playing catch up.

[lhl2617]

Addendum:

I dug deeper into kernel code, and I think we'd actually need them for all of

int8  int16  int32  int64
uint8 uint16 uint32 uint64

thus basically every int type with a specified width.

Some examples:

PPRSTATUS: uint8
RIO_CM_CHAN_CREATE: uint16
TIOCGPTN: uint32
ACPI_THERMAL_GET_TRT_LEN: uint64

TIOCSTI: int8
<NOT FOUND BUT NOT SURE IF PRESENT>: int16
TIOCSPTLCK: int32
FIOQSIZE: int64

[tmthrgd]

You don’t actually need both signed and unsigned types as converting between them explicitly keeps the same bit pattern. All the kernel knows is the size of the argument. They might be useful to have for clarity though.