Skip to content

All FreeBSD do not implement the function sysconf with arguments #13187

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

5u623l20
Copy link
Contributor

_SC_GETGR_R_SIZE_MAX , _SC_TTY_NAME_MAX:_SC_GETPW_R_SIZE_MAX.

php use this sysconf() in:

main/main.c php_get_current_user()
main/fopen_wrappers.c php_fopen_primary_script()
ext/posix/posix.c posix_getpwnam(), posix_getpwuid(), posix_getgrnam(), posix_getgrgid()
ext/standard/filestat.c php_get_uid_by_name(), php_get_gid_by_name()

Somehow I mistakenly rebased on the previous PR branch. This is just a fresh continuation of:
#12995

@bukka Somehow I was not getting your comments until today I was rebasing the patches for php 8.3.2 and 8.2.15 in the FreeBSD ports tree. However I think this patch will answer that question.

_SC_GETGR_R_SIZE_MAX , _SC_TTY_NAME_MAX:_SC_GETPW_R_SIZE_MAX.

php use this sysconf() in:

main/main.c php_get_current_user()
main/fopen_wrappers.c php_fopen_primary_script()
ext/posix/posix.c posix_getpwnam(), posix_getpwuid(), posix_getgrnam(),
posix_getgrgid()
ext/standard/filestat.c php_get_uid_by_name(), php_get_gid_by_name()
@devnexen
Copy link
Member

Thanks for doing the (re)work, looks clearer now, I ll have a look soon-ish.

@@ -477,7 +477,11 @@ PHP_FUNCTION(posix_ttyname)
#if defined(ZTS) && defined(HAVE_TTYNAME_R) && defined(_SC_TTY_NAME_MAX)
buflen = sysconf(_SC_TTY_NAME_MAX);
if (buflen < 1) {
#if defined(__FreeBSD__) && defined(_SC_PAGESIZE)
Copy link
Member

Choose a reason for hiding this comment

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

what makes you think _SC_PAGESIZE might not be defined ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't recollect the exact reason at the moment but so far from my Goldfish memory I think it was that either Zend or zts redefines the page size with it's real size and checks in this similar fashion. Or it might have been too old when SC_PAGESIZE was not defined in FreeBSD at all. Because at one point there was only _SC_PAGE_SIZE rather then _SC_PAGESIZE

Copy link
Member

Choose a reason for hiding this comment

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

I looked even at freebsd 5.0, seems it was already there.

Comment on lines 789 to +795
buflen = sysconf(_SC_GETGR_R_SIZE_MAX);
if (buflen < 1) {
#if defined(__FreeBSD__) && defined(_SC_PAGESIZE)
buflen = sysconf(_SC_PAGESIZE);
#else
RETURN_FALSE;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

So I have already asked about this and probably not in a clear way. So my main question was don't you just do this:

#if defined(__FreeBSD__)
	buflen = sysconf(_SC_PAGESIZE);
#else
	buflen = sysconf(_SC_GETGR_R_SIZE_MAX);
#endif
	if (buflen < 1) {	
		RETURN_FALSE;

The only reason that I could see for not doing this is that you somehow expect that this is going to be supported on FreeBSD in the future. Is that the case? If so, I'm not sure we should be predicting what might happen (especially if it's more likely not going to happen). But if you really think that it's quite likely, then I'm fine with it.

In such case you should still check the return value of of the second sysconf call even though it is unlikely to fail. So something like this should be done:

	buflen = sysconf(_SC_GETPW_R_SIZE_MAX);
	if (buflen < 1) {
#if defined(__FreeBSD__)
		buflen = sysconf(_SC_PAGESIZE);
		if (buflen < 1) {
			RETURN_FALSE;
		}
#else
		RETURN_FALSE;
#endif

@bukka
Copy link
Member

bukka commented Nov 8, 2024

Is there any update on this? It needs also rebase

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