Yeah I think I did a lot of work on this a few years ago to support it, so glad that the remaining bit was small. Give me a bit to review this. Thanks for the PR!

I think this should be in an ifdef BSD vs linux, because linux statfs isn't 64bit capable I think.

How is O_PATH and O_RDONLY different between bsd/linux? Would be good to know more here.

Might be cleaner to move the comment above the line, rather than leaving it in-line.

At 2020-03-22 01:37:46 +0000, @firstyear commented on line 7 of ldap/servers/slapd/daemon.c:

I think this should be in an ifdef BSD vs linux, because linux statfs isn't 64bit capable I think.

Surprise --- it is already! The entirety of disk_mon_get_mount_point is in an #if LINUX and #elif __FreeBSD__. I suspect there's probably a better way to do it, but at the moment I'm going for "get it up on FreeBSD" over "be really cross-platform".

At 2020-03-22 01:38:26 +0000, @firstyear commented on line 10 of ldap/servers/slapd/dse.c:

How is O_PATH and O_RDONLY different between bsd/linux? Would be good to know more here.

This one I don't have a good answer to. Citing the open(2) manual entry, O_PATH is designed to "Obtain a file descriptor that can be used for two purposes: to indicate a location in the filesystem tree and to perform operations that act purely at the file descriptor level. The file itself is not opened, [...]". It is not semantically equivalent to O_RDONLY inasmuch as there are cases where O_PATH would succeed --- e.g., you can open a file O_PATH that you cannot read, but O_RDONLY would explicitly fail.

However: the only case where dse_write_file_nolock (where we are) is invoked is one where, as far as I can tell from twenty minutes of wild tag-walking, the path referenced by (struct dse *)->dse_configdir has already been opened elsewhere for other things, and therefore O_RDONLY should be good enough if O_PATH isn't an option.

At 2020-03-22 01:39:17 +0000, @firstyear commented on line 6 of lib/libaccess/lasip.cpp:

Might be cleaner to move the comment above the line, rather than leaving it in-line.

Having looked closer at what that condition means, I'm not even sure why (int)NULL was used...? I'll update that patch and drop the comment entirely.

At 2020-03-22 01:37:46 +0000, @firstyear commented on line 7 of ldap/servers/slapd/daemon.c:

I think this should be in an ifdef BSD vs linux, because linux statfs isn't 64bit capable I think.

Surprise --- it is already! The entirety of disk_mon_get_mount_point is in an #if LINUX and #elif FreeBSD. I suspect there's probably a better way to do it, but at the moment I'm going for "get it up on FreeBSD" over "be really cross-platform".

Having a read of daemon.c, this actually doesn't matter as that function is only getting f_mntonname anyway, so the 32/64 doesn't matter. Where it will matter is disk_mon_check_diskspace but that looks like it doesn't have a linux vs freebsd split and DOES use statvfs. So that's something for you to keep in mind for later ...

At 2020-03-22 01:38:26 +0000, @firstyear commented on line 10 of ldap/servers/slapd/dse.c:

How is O_PATH and O_RDONLY different between bsd/linux? Would be good to know more here.

This one I don't have a good answer to. Citing the open(2) manual entry, O_PATH is designed to "Obtain a file descriptor that can be used for two purposes: to indicate a location in the filesystem tree and to perform operations that act purely at the file descriptor level. The file itself is not opened, [...]". It is not semantically equivalent to O_RDONLY inasmuch as there are cases where O_PATH would succeed --- e.g., you can open a file O_PATH that you cannot read, but O_RDONLY would explicitly fail.
However: the only case where dse_write_file_nolock (where we are) is invoked is one where, as far as I can tell from twenty minutes of wild tag-walking, the path referenced by (struct dse *)->dse_configdir has already been opened elsewhere for other things, and therefore O_RDONLY should be good enough if O_PATH isn't an option.

Having a quick look into dse.c though, we then call fsync on the fd, is that legal to do to an FD that's RDONLY? It may be safer here to use O_WRONLY instead as the semantics of fsync on RDONLY seem unclear.

At 2020-03-22 01:39:17 +0000, @firstyear commented on line 6 of lib/libaccess/lasip.cpp:

Might be cleaner to move the comment above the line, rather than leaving it in-line.

Having looked closer at what that condition means, I'm not even sure why (int)NULL was used...? I'll update that patch and drop the comment entirely.

Thanks again! I think we are almost ready to merge this :)

At 2020-03-22 23:01:38 +0000, @firstyear commented:

At 2020-03-22 01:37:46 +0000, @firstyear commented on line 7 of ldap/servers/slapd/daemon.c:

I think this should be in an ifdef BSD vs linux, because linux statfs isn't 64bit capable I think.

Surprise --- it is already! The entirety of disk_mon_get_mount_point is in an #if LINUX and #elif FreeBSD. I suspect there's probably a better way to do it, but at the moment I'm going for "get it up on FreeBSD" over "be really cross-platform".

Having a read of daemon.c, this actually doesn't matter as that function is only getting f_mntonname anyway, so the 32/64 doesn't matter. Where it will matter is disk_mon_check_diskspace but that looks like it doesn't have a linux vs freebsd split and DOES use statvfs. So that's something for you to keep in mind for later ...

In fairness, the FreeBSD man page for statvfs(3) does begin: "The statvfs() and fstatvfs() functions fill the structure pointed to by buf with garbage. This garbage will occasionally bear resemblance to file system statistics, but portable applications must not depend on this."

I'm not immediately seeing a 32/64-unsafe operation in disk_mon_check_diskspace; the LL_UI2L's look okay to me?

At 2020-03-22 01:38:26 +0000, @firstyear commented on line 10 of ldap/servers/slapd/dse.c:

How is O_PATH and O_RDONLY different between bsd/linux? Would be good to know more here.

This one I don't have a good answer to. Citing the open(2) manual entry, O_PATH is designed to "Obtain a file descriptor that can be used for two purposes: to indicate a location in the filesystem tree and to perform operations that act purely at the file descriptor level. The file itself is not opened, [...]". It is not semantically equivalent to O_RDONLY inasmuch as there are cases where O_PATH would succeed --- e.g., you can open a file O_PATH that you cannot read, but O_RDONLY would explicitly fail.
However: the only case where dse_write_file_nolock (where we are) is invoked is one where, as far as I can tell from twenty minutes of wild tag-walking, the path referenced by (struct dse *)->dse_configdir has already been opened elsewhere for other things, and therefore O_RDONLY should be good enough if O_PATH isn't an option.

Having a quick look into dse.c though, we then call fsync on the fd, is that legal to do to an FD that's RDONLY? It may be safer here to use O_WRONLY instead as the semantics of fsync on RDONLY seem unclear.

I second "unclear". There's an interesting discussion on the Austin Group list about the semantics of fsync'ing a directory; it seems some implementations (apparently including FreeBSD) ignore the open mode anyway.

Empirical tests on Linux and FreeBSD suggest fsync'ing a O_RDONLY file descriptor does apparently the right thing.

Possibly more problematic is that O_WRONLY of a directory is an EISDIR.

Having a read of daemon.c, this actually doesn't matter as that function is only getting f_mntonname anyway, so the 32/64 doesn't matter. Where it will matter is disk_mon_check_diskspace but that looks like it doesn't have a linux vs freebsd split and DOES use statvfs. So that's something for you to keep in mind for later ...

In fairness, the FreeBSD man page for statvfs(3) does begin: "The statvfs() and fstatvfs() functions fill the structure pointed to by buf with garbage. This garbage will occasionally bear resemblance to file system statistics, but portable applications must not depend on this."
I'm not immediately seeing a 32/64-unsafe operation in disk_mon_check_diskspace; the LL_UI2L's look okay to me?

This absolutely fills me with confidence, and is yet another daily reminder that computers are cursed and we should all go back to our rooms and cry.

Anyway, in that case, it all looks fine to me.

Having a quick look into dse.c though, we then call fsync on the fd, is that legal to do to an FD that's RDONLY? It may be safer here to use O_WRONLY instead as the semantics of fsync on RDONLY seem unclear.

I second "unclear". There's an interesting discussion on the Austin Group list about the semantics of fsync'ing a directory; it seems some implementations (apparently including FreeBSD) ignore the open mode anyway.
Empirical tests on Linux and FreeBSD suggest fsync'ing a O_RDONLY file descriptor does apparently the right thing.
Possibly more problematic is that O_WRONLY of a directory is an EISDIR.

Okay, let's leave it as O_RDONLY then.

So last thing before we merge. Can you have a look at: http://www.port389.org/docs/389ds/contributing.html#getting-the-patch-ready for the git commit template that we use, and squash all your commits into a single change with git rebase -i HEAD~range?

You can use me as the reviewer. Once that's done, I'll merge this. Thanks so much for your time and contribution! If there is anything else I can do to help you, please let me know.

At 2020-03-23 00:39:11 +0000, @firstyear commented:

This absolutely fills me with confidence, and is yet another daily reminder that computers are cursed and we should all go back to our rooms and cry.

Hear hear.

So last thing before we merge. Can you have a look at: http://www.port389.org/docs/389ds/contributing.html#getting-the-patch-ready for the git commit template that we use, and squash all your commits into a single change with git rebase -i HEAD~range?

ACK, doing so now. Just to confirm --- do I need to also create an issue for this PR to resolve?

No, I'll create the issue on your behalf :)

https://pagure.io/389-ds-base/issue/50971

When we merge the Pr, I'll leave the issue open as an umbrella tracker for any follow up items you may find - it's likely there will be some in they python parts of the code around setup and instance creation :)

Excellent, thanks! Rebased, squashed, ready to go.

Let me know if you have any issues with the next step (dscreate and the python tooling). The 389-devel list might be the best place for you to join to discuss if you hit any issue with that! Thanks!

This commit introduced compiler warnings on Fedora/RHEL:

In file included from /usr/include/sys/types.h:25,
                 from ../389-ds-base/ldap/servers/plugins/pwdstorage/crypt_pwd.c:27:
/usr/include/features.h:185:3: warning: #warning "_BSD_SOURCE and _SVID_SOURCE are deprecated, use _DEFAULT_SOURCE" [-Wcpp]
  185 | # warning "_BSD_SOURCE and _SVID_SOURCE are deprecated, use _DEFAULT_SOURCE"
      |   ^~~~~~~

@mreynolds I'll have a look and submit a fix today :)

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4022

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.