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

gh-99987: fcntl add the F_KINFO constant for FreeBSD 13.1 and onwards. #99988

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented Dec 4, 2022

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fcntl() does not contain code specific to other operations. The user is expected to provide a buffer object of the correct size and properly initialized.

devnexen and others added 3 commits December 28, 2023 00:39
- removing custom code out of `fcntl.fcntl`.
- F_GETPATH requires MAXPATHLEN or greater so we increase the buffer size
to be able to cater with F_KINFO greater size.
- `fcntl.kinfoalloc` preallocate a proper kinfo_file buffer.
- `fcntl.kinfodict` returns a handy representation of the data.
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why do you want to add kinfoalloc() and kinfodict(), but for now there are no precedents for other operations. See for example the use of F_SETLK in tests and in third-party examples. They use struct.pack() and struct.unpack(). The format string, unfortunately, is platform depending.

If we came with better solution, it should be universal, and cover all other fcntl() and ioctl() operations. It is a different and much larger issue. Let limit this issue for adding F_KINFO (and maybe other constants?).

@@ -57,6 +57,8 @@ descriptor.
``FICLONERANGE`` constants, which allow to share some data of one file with
another file by reflinking on some filesystems (e.g., btrfs, OCFS2, and
XFS). This behavior is commonly referred to as "copy-on-write".
On FreeBSD >= 13.1, the :mod:`fcntl` module exposes the ``F_KINFO``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be versionchanged:: 3.13 now.

@devnexen
Copy link
Contributor Author

I understand why do you want to add kinfoalloc() and kinfodict(), but for now there are no precedents for other operations. See for example the use of F_SETLK in tests and in third-party examples. They use struct.pack() and struct.unpack(). The format string, unfortunately, is platform depending.

If we came with better solution, it should be universal, and cover all other fcntl() and ioctl() operations. It is a different and much larger issue. Let limit this issue for adding F_KINFO (and maybe other constants?).

easier said than done tough.

@serhiy-storchaka
Copy link
Member

Oh, nested unions in the middle of structure. They hate us.

It means also that the complete unpacker that supports all fields should be super complex.

Does it support binary compatibility with FreeBSD < 12? If yes, then the first element in the union should have the largest size, and we can ignore alternatives.

@devnexen
Copy link
Contributor Author

devnexen commented Dec 28, 2023

That makes it more user unfriendly tough, the user needs to be aware of the struct layout (which might evolve over versions), pack() implies initialises fields which we do not want to do but one ... etc.

Here an example of the format
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants