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

[JENKINS-65911] Correct JNA method signature for fcntl(2) #9026

Merged
merged 1 commit into from
Mar 10, 2024

Conversation

basil
Copy link
Member

@basil basil commented Mar 8, 2024

While looking at JENKINS-72833 I remembered my old friend JENKINS-65911. While I could no longer reproduce the problem on my macOS system, I was able to reproduce incorrect fcntl results in a new unit test when running on macOS (but not Linux). The problem seems to be, as I wrote back in 2021, an incorrect JNA signature. fcntl is a variadic function. While I couldn't find much documentation about this, I tried using Java varargs and hoping JNA would deal with this correctly, and it did. My unit test started passing on macOS and there were no regressions running the test on Linux. Just to be sure, I stepped through the restart sequence outside of systemd in a debugger and verified in a separate terminal running strace that the system calls on Linux were unchanged. While I was here I also wrote some other tests to increase coverage for this file.

Testing done

Before this PR, new unit test failed on macOS and passed on Linux. After this PR, passes on both platforms.

Also stepped through the restart sequence outside of systemd on Linux and verified correct system call input and output in strace.

Proposed changelog entries

  • Fix a crash when restarting Jenkins on macOS.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

@basil basil added the bug For changelog: Minor bug. Will be listed after features label Mar 8, 2024
public class GNUCLibraryTest {

private static final int EBADF = 9;
private static final int O_CREAT = "Linux".equals(System.getProperty("os.name")) ? 64 : 512;
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Interesting finding and analysis 👀

@NotMyFault NotMyFault added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Mar 9, 2024
Copy link
Contributor

@StefanSpieker StefanSpieker left a comment

Choose a reason for hiding this comment

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

Thanks for the reasoning and detailed explanation

@MarkEWaite
Copy link
Contributor

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

@NotMyFault NotMyFault merged commit c9af352 into jenkinsci:master Mar 10, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Minor bug. Will be listed after features ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants