-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
base: master
Are you sure you want to change the base?
Conversation
_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()
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) |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
buflen = sysconf(_SC_GETGR_R_SIZE_MAX); | ||
if (buflen < 1) { | ||
#if defined(__FreeBSD__) && defined(_SC_PAGESIZE) | ||
buflen = sysconf(_SC_PAGESIZE); | ||
#else | ||
RETURN_FALSE; | ||
#endif |
There was a problem hiding this comment.
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
Is there any update on this? It needs also rebase |
_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.