Skip to content

Conversation

@ellert
Copy link
Contributor

@ellert ellert commented May 15, 2025

The access mode values for files are not the same for all platforms. The detection of a read only request here assumes that O_RDONLY is 0, which is not mandated by the POSIX standaed.

Linux:

  #define O_ACCMODE       00000003
  #define O_RDONLY        00000000
  #define O_WRONLY        00000001
  #define O_RDWR          00000002

But this is also allowed:

  #define O_RDONLY	0x0001	/* Open read-only.  */
  #define O_WRONLY	0x0002	/* Open write-only.  */
  #define O_RDWR	(O_RDONLY|O_WRONLY) /* Open for reading and writing. */
  #define O_ACCMODE	O_RDWR	/* Mask for file access modes.  */

And on systems using this option the detection of a read only request as implemented here is is broken and the cache test fails.

This commit fixes the detection so that it is done in the intended way that works everywhere.

Compare:

if ((flags&O_ACCMODE) == O_RDONLY) { // Access mode is READ

if ((dflags & O_ACCMODE) != O_RDONLY)

Log of failing test:
cachelog.txt

The access mode values for files are not the same for all platforms.
The detection of a read only request here assumes that O_RDONLY is 0,
which is not mandated by the POSIX standaed.

Linux:
  #define O_ACCMODE       00000003
  #define O_RDONLY        00000000
  #define O_WRONLY        00000001
  #define O_RDWR          00000002

But this is also allowed:
  #define O_RDONLY	0x0001	/* Open read-only.  */
  #define O_WRONLY	0x0002	/* Open write-only.  */
  #define O_RDWR	(O_RDONLY|O_WRONLY) /* Open for reading and writing. */
  #define O_ACCMODE	O_RDWR	/* Mask for file access modes.  */

And on systems using this option the detection of a read only request
as implemented here is is broken and the cache test fails.

This commit fixes the detection so that it is done in the intended way
that works everywhere.
@amadio
Copy link
Member

amadio commented May 15, 2025

Nice to see new tests at work. Thank you for the fix!

@amadio amadio requested a review from osschar May 15, 2025 16:28
Copy link
Contributor

@osschar osschar left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this! :)

@amadio amadio merged commit a080294 into xrootd:master May 15, 2025
11 checks passed
@amadio amadio added this to the 5.8.3 milestone May 15, 2025
@ellert ellert deleted the access-mode branch May 15, 2025 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants