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

test: skip the pwd_shell test on IBMi #2592

Closed
wants to merge 1 commit into from

Conversation

dmabupt
Copy link
Contributor

@dmabupt dmabupt commented Jan 3, 2020

On IBMi PASE, the value of pw_shell is always an empty string.
Seems the API getpwuid_r() is not fully implemented on PASE.

On IBMi PASE, the value of pw_shell is always an empty string.
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

I'm a little undecided on whether it'd be better to change uv_os_get_passwd() to set pwd->shell to NULL or e.g. "/bin/sh" when pw_shell is an empty string. Thoughts?

The Windows implementation of uv_os_get_passwd() always sets pwd->shell = NULL but UNIX users may have the (reasonable?) expectation that it's always set to something. From man 5 passwd:

This is the program to run at login (if empty, use /bin/sh).

@dmabupt
Copy link
Contributor Author

dmabupt commented Jan 7, 2020

I will discuss with the API owner to see if the returned empty string is working as design. If so, we may assert that len == 0 or set it to /bin/sh.

@dmabupt
Copy link
Contributor Author

dmabupt commented Jan 10, 2020

Based on the document , the default shell of a user is empty. We have to set it manually.

When I called the procedure

CALL QSYS2.SET_PASE_SHELL_INFO('*CURRENT', '/QOpenSys/pkgs/bin/bash'); 

the test of get_passwd passed.

Seems we should not overwrite the value if it exist. It could be an empty string or a meaningful value. So shall we keep current patch to let IBMi PASE pass the test?

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM. It doesn't make sense to assert that a size_t is >= 0.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 12, 2020

@cjihrig
Copy link
Contributor

cjihrig commented Jan 12, 2020

CI seems fine. This can land, but I'll wait in case @bnoordhuis wants to weigh in, based on #2592 (review). No rush since this just changes a test for a platform that isn't in the CI 😄

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but #2592 (comment) still seems like a good idea.

cjihrig pushed a commit that referenced this pull request Feb 28, 2020
On IBMi PASE, the value of pw_shell is always an empty string.

PR-URL: #2592
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@cjihrig
Copy link
Contributor

cjihrig commented Feb 28, 2020

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1766/. Seems fine - the issues are unrelated to this PR, and have been fixed since this PR branch was created.

Landed in 174a6ed. Thanks!

@cjihrig cjihrig closed this Feb 28, 2020
@dmabupt dmabupt deleted the ibmi_pwdshell branch February 29, 2020 00:57
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