Skip to content

Conversation

@semarie
Copy link
Contributor

@semarie semarie commented Oct 27, 2025

The following PR fixes few build errors while running make and make test on alpine linux.

It mostly adds compatibility declaration on C stuff (unixpwd and forkexecd compoments).

The more important changes is regarding getpwent_r and getspent_r usage in unixpwd.
As it, these functions doesn't exists in musl-libc. So I switched back to more standard variants (getpwent and getspent). There are explicitly not reentrants, but getpwent_r and getspent_r weren't fully reentrants too (due to FILE sharing between threads), so I assume it doesn't introduce regression from this point of vue.

With the PR, I am able to run ./configure && make && make test without errors (tested on alpine edge).

@freddy77
Copy link
Collaborator

If you don't add some locking amongst getpwent calls you could end up with one thread giving result from a function called in another thread. This can be a security concern.

@last-genius
Copy link
Contributor

last-genius commented Oct 27, 2025

Some of these changes are small and reasonable (portability is always good!), but I'm not sure how much these are going to help you work with xapi on alpine. xs-opam will require quite a few packages that might not be available on alpine, and I'm sure there's more work to get some of our dependencies to build with musl libc too. Approximating the build environment might be the easiest option - most of us use Fedora or similar (I'm personally using a Fedora distrobox on Arch Linux).

UPD: I'd love to be proven wrong - if these are the only blockers for you currently, that'd be great

@andyhhp
Copy link
Collaborator

andyhhp commented Oct 27, 2025

Commits 1, 4 and 5 are trivial and fine. The use of non-recursive functions needs more careful review

@semarie
Copy link
Contributor Author

semarie commented Oct 27, 2025

@freddy77 I agree that concurrent threads could lead to problems. but the current code isn't different: concurrent calls to getpwent_r will result in similar problems.

The getpwent_rman page documents it as:

The function getpwent_r() is not really reentrant since it shares
the reading position in the stream with all other threads.

getpwent_r only avoids using static memory for the returned result, but the source (the passwd file opened) is still shared between threads.

so if reentrance is mandated the current code is already wrong.

@last-genius the sole problem I had with dependencies is dlm. I had to patch xs-opam to avoid the explicit reject of alpine. But the library built fine from source (but only the library, tools like dlm_controld needs additional stuff)

@freddy77
Copy link
Collaborator

Why not using fopen/fclose and fgetpwent_r/fgetspent_r where available instead? There are also lckpwdf/ulckpwdf although musl implementation for these is empty...

Comment on lines 122 to 125
if (rc != ENOENT) {
unlink(tmp_name);
return rc;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be dangerous. Potentially for an error we are truncating /etc/passwd file instead of returning error.

@semarie
Copy link
Contributor Author

semarie commented Oct 27, 2025

yes, fgetpwent_r and fgetspent_r seems to be the way for reentrance (but not for musl).

For now, I will drop the changes for getpwent and getspent usage, so only uncontroversal changes will be in the PR. Next I will look for doing another PR for fgetpwent_r and fgetspent_r usage. And last go back to musl compat case to see what could be done.

Copy link
Contributor

@last-genius last-genius left a comment

Choose a reason for hiding this comment

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

I believe Andy used to have the maintainer status and his approvals were green, did anything change?

@andyhhp
Copy link
Collaborator

andyhhp commented Oct 27, 2025

I've never been a maintainer in Xapi.

@robhoes robhoes added this pull request to the merge queue Oct 27, 2025
Merged via the queue into xapi-project:master with commit 7a3cf08 Oct 27, 2025
16 checks passed
@semarie semarie deleted the alpine-compat branch October 27, 2025 16:01
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.

5 participants