Skip to content

Use st_blksize and st_blocks members conditionally #18964

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

petk
Copy link
Member

@petk petk commented Jun 27, 2025

Instead of filtering Windows specifically, the preprocessor macros HAVE_STRUCT_STAT_ST_BLKSIZE and HAVE_STRUCT_STAT_ST_BLOCKS can be used.

These members are mostly present on all POSIX-based systems except on Windows these days but this syncs the usage style across the code base.

Additionally, in ext/standard/ftp_fopen_wrapper.c to set ssb->sb.st_blocks, also ssb->sb.st_blksize is added in the condition.

Instead of filtering Windows specifically, the preprocessor macros
HAVE_STRUCT_STAT_ST_BLKSIZE and HAVE_STRUCT_STAT_ST_BLOCKS can be used.

These members are mostly present on all POSIX-based systems except on
Windows these days but this syncs the usage style across the code base.

Additionally, in ext/standard/ftp_fopen_wrapper.c to set
ssb->sb.st_blocks, also ssb->sb.st_blksize is added in the condition.
ssb->sb.st_blksize = -1;
#endif
#ifdef HAVE_STRUCT_STAT_ST_BLOCKS
ssb->sb.st_blocks = -1;
Copy link
Member

Choose a reason for hiding this comment

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

it might be possible to simplify a tad as if there is st_blksize, there is st_blocks, wdyt ?

Copy link
Member Author

@petk petk Jun 28, 2025

Choose a reason for hiding this comment

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

I think we could do that, yes. Having st_blksize also most likely implies having st_blocks. And that would be good to get rid of this anomaly also in configure.ac then:

dnl AC_STRUCT_ST_BLOCKS will screw QNX because fileblocks.o does not exist.
if test "$(uname -s 2>/dev/null)" != "QNX"; then
  AC_STRUCT_ST_BLOCKS
fi

But I'm not sure. Should this be done all over php-src or only on this place?

Copy link
Member Author

@petk petk Jun 28, 2025

Choose a reason for hiding this comment

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

Ah, I see. Yes, I think this would be completely fine then to only check for st_blksize. If it's already in ext/phar and working out there when this condition is used #ifndef PHP_WIN32, then just checking for #ifdef HAVE_STRUCT_ST_BLKSIZE is the way to go in PHP. Because I don't see any build errors from ext/phar regarding these struct members...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll recheck this a bit. Yes, everyone uses both checks and check them separately but this can totally be simplified indeed. Today's platforms all should have either both (POSIX) or none (Windows). I've already checked some BSDs, macOS, Solaris 10, Haiku...

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.

2 participants