Skip to content
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

Add support for zos/s390x #582

Merged
merged 4 commits into from
Apr 6, 2024
Merged

Add support for zos/s390x #582

merged 4 commits into from
Apr 6, 2024

Conversation

dustin-ward
Copy link
Contributor

Closes #581 #579 #578

  • Created stat_zos.go to alter the syscall constants that were not consistent with other platforms

  • toPflags also needed a minor change to support platforms that have different values for os.O_WRONLY, os.O_RDONLY & os.O_RDWR (0x1, 0x2, 0x3 on zos respectively)

  • Some other small modifications were made to tests so that they pass on zos/s390x

switch f & os.O_WRONLY {
case os.O_WRONLY:
out |= sshFxfWrite
switch f & (os.O_RDONLY | os.O_WRONLY | os.O_RDWR) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider that POSIX standard requires the assertion: O_RDONLY | O_WRONLY == O_RDWR[1]

However, due to constant folding, I think we’re still fine here regardless.

1: https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html

stat_zos.go Outdated Show resolved Hide resolved
stat_zos.go Outdated Show resolved Hide resolved
@dustin-ward
Copy link
Contributor Author

dustin-ward commented Apr 5, 2024

Removed stat_zos and changed up stat_posix. Im not really happy with all of the type casting... But I still want to make the changes as small as possible. I also dont like renaming the "io/fs" import... but "fs" is already taken. Would you prefer we rename kr/fs? Any ideas?

Copy link
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

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

On the lines where you’re adding io/fs for something, there are available os aliases. I’m not saying to use the os version, but it is a possible way to skirt the name clash between io/fs and github.com/kr/fs.

Alternatively, I would recommend renaming kr/fs rather than the standard library. (Even though this probably is a larger change surface.)

client.go Outdated Show resolved Hide resolved
client_integration_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

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

Looks great. So much thanks.

stat.go Outdated
s_ISVTX = uint32(sshfx.ModeSticky)
)

// Legacy export:
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] godoc should start with S_IFMT

(I know the comments on the pre-existing code wasn’t ideal, and you’re just moving it around.)

@dustin-ward
Copy link
Contributor Author

Thanks for your feedback! ✌️

// Go defines S_IFMT on windows, plan9 and js/wasm as 0x1f000 instead of
// 0xf000. None of the the other S_IFxyz values include the "1" (in 0x1f000)
// which prevents them from matching the bitmask.
// S_IFMT is a legacy export. `sshfx.ModeType` should be used instead
Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem is that the sshfx package is internal and so no one else can import it. So, the given alternative isn’t useful.

I would say that the local OS might use a different value from what we need to use internally, and there is no reason anyone should need this specific internal value.

We can also merge, and then I can adjust it after.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see. That makes more sense. Better to have you leave the comment anyways. Thanks again

@drakkan drakkan merged commit 3c39a36 into pkg:master Apr 6, 2024
4 checks passed
@dustin-ward
Copy link
Contributor Author

Would it also be possible to get a new tag that includes the recent changes?

@puellanivis
Copy link
Collaborator

I want to get the WithWindowsRootEnumeratesDrives also checked in before I cut another release.

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.

Proposal: Standardized FileInfo Structure. RE: z/OS performance investigation
3 participants