NotificationsYou must be signed in to change notification settings
Fork32k
Star67.2k

bpo-46021: fcntl module update supporting FreeBSD's F_KINFO proposal.#30000

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

devnexen wants to merge1 commit intopython:main

base:main

Choose a base branch

fromdevnexen:fcntl_fbsd_upd2

Open

bpo-46021: fcntl module update supporting FreeBSD's F_KINFO proposal.#30000

devnexen wants to merge1 commit intopython:mainfromdevnexen:fcntl_fbsd_upd2

Conversation

Copy link

Contributor

devnexen commentedDec 9, 2021•
edited by bedevere-bot
Loading

https://bugs.python.org/issue46021

the-knights-who-say-ni added the CLA signed label

Dec 9, 2021

bedevere-bot added the awaiting review label

Dec 9, 2021

devnexen changed the title~~fcntl module update supporting FreeBSD's F_KINFO proposal.~~bpo-46021: fcntl module update supporting FreeBSD's F_KINFO proposal.

Dec 9, 2021

fcntl module update supporting FreeBSD's F_KINFO proposal.

62ea0ae

devnexen force-pushed thefcntl_fbsd_upd2 branch from7e2e98a to62ea0aeCompare

December 9, 2021 09:15

tiran requested changes

Dec 9, 2021

View reviewed changes

Copy link

Member

tiran left a comment

Choose a reason for hiding this comment

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

Please include

blurb (news entry)
documentation update
tests
configure.ac header check

Modules/fcntlmodule.c

Comment on lines +18 to +20

		#ifdef __FreeBSD__
		#include <sys/user.h>
		#endif

Copy link

Member

Choose a reason for hiding this comment

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

This needs anAC_CHECK_HEADERS check in configure.ac

Modules/fcntlmodule.c

Comment on lines +116 to +121

		if (ret) {
		PyDict_SetItemString(ret, "status", PyLong_FromLong(f.kf_status));
		PyDict_SetItemString(ret, "type", PyLong_FromLong(f.kf_type));
		PyDict_SetItemString(ret, "offset", PyLong_FromLongLong(f.kf_offset));
		PyDict_SetItemString(ret, "path", PyBytes_FromString(f.kf_path));
		return ret;

Copy link

Member

Choose a reason for hiding this comment

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

fcntl.fcntl() returns either an integer or bytes. IfF_KINFO needs a different return type, then it would be better to implement the feature as a new platform-specific function in the fcntl module, e.g.fcntl.kinfo(fd: int) -> dict.

bedevere-bot added awaiting changes and removed awaiting review labels

Dec 9, 2021

Copy link

bedevere-bot commentedDec 9, 2021

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!