Added statfs for FreeBSD #1937

jercaianu · 2017-10-11T11:13:59Z

Although POSIX uses statvfs, statvfs does not give good results on FreeBSD.
However statfs works fine on FreeBSD as described here[1].

@edi33416 helped me test the functionality on FreeBSD. Statfs should be used for implementing "getAvailableDiskSpace" on FreeBSD as discussed here[2].

[1] - https://www.freebsd.org/cgi/man.cgi?query=statfs&sektion=2
[2] - dlang/phobos#5776

dlang-bot · 2017-10-11T11:14:00Z

Thanks for your pull request and interest in making D better, @jercaianu! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#1937"

PetarKirov · 2017-10-11T11:27:27Z

src/core/sys/posix/sys/statvfs.d

+else version (FreeBSD)
+{
+    enum  MFSNAMELEN = 16;
+    enum  MNAMELEN = 1024;


MNAMELEN is defined as 88 here: https://www.freebsd.org/cgi/man.cgi?query=statfs&sektion=2. Why are you changing it to 1024?

https://github.com/freebsd/freebsd/blob/aa552394dbf32a59a1ab8763f00c8fe774b22032/sys/sys/mount.h#L68

I'm not sure why man page doesn't respect that, though 🤔

Ah, I think I know why. It might be the part of the inode64 changes for the FreeBSD. Here's the latest man page: https://github.com/freebsd/freebsd/blob/565b5a2e9ec6e684421a7e6eb7b37d19e3c0f47e/lib/libc/sys/statfs.2#L69

Here's commit changing it: freebsd/freebsd-src@e75ba1d#diff-685f3d1e8b4f3709173b73973b379ac0

Extend the ino_t, dev_t, nlink_t types to 64-bit ints. Modify struct dirent layout to add d_off, increase the size of d_fileno to 64-bits, increase the size of d_namlen to 16-bits, and change the required alignment. Increase struct statfs f_mntfromname[] and f_mntonname[] array length MNAMELEN to 1024.

So, this is not still the value for the RELEASE, but it is for the CURRENT.

I'm not sure how we should proceed here. Use only the latest definition, or provide the two under version FreeBSDv1X.X (or something like that). I would go for the second option, but I don't know if we have a good interface for passing this version to druntime build.

The problem will arise as soon as this functionality is used in Phobos as the Autotester uses the (AFAIK) the older version of FreeBSD.

Here's what @CyberShadow proposed in the issue:

- We add getosreldate and INO64_FIRST to druntime - We add both the old and new struct definitions - We add a stat wrapper which,when getosreldate() < INO64_FIRST, translates the old struct to the new struct and returns it.

There's an issue about this, as right now dmd will not work on the CURRENT at all: https://issues.dlang.org/show_bug.cgi?id=17596)

Thanks. @jercaianu can you add both definitions (with different names, but defined unconditionally together) and add a D translation of the C wrapper function mentioned here: https://issues.dlang.org/show_bug.cgi?id=17596#c8 ?

Sure. I'll look into it :)

andralex

So what's the status of this?

andralex · 2017-10-16T07:13:56Z

src/core/sys/posix/sys/statvfs.d

+        MNT_AUTOMOUNTED	= 131072 /* mounted by automountd(8) */
+    }
+
+    int statfs (const char * file, statfs_t* buf);


we don't use space before open paren in function declarations and calls.

and also we don't put a space before pointers -> char*

andralex · 2017-10-16T07:14:27Z

src/core/sys/posix/sys/statvfs.d

+    enum FFlag
+    {
+        MNT_RDONLY = 1, /* read only filesystem */
+        MNT_SYNCHRONOUS	= 2, /* fs written synchronously */


no tabs please

PetarKirov · 2017-10-16T07:39:30Z

So what's the status of this?

@andralex FreeBSD changed their ABI and in order for us to be compatible with both versions we would need to add two versions of the same struct, along with a wrapper function that fills the right one depending on the OS version and returns the one defined by the newer ABI (superset of the older) to higher-level code. See the discussion in this bug report for more info: https://issues.dlang.org/show_bug.cgi?id=17596#c8

jercaianu · 2018-03-29T17:04:09Z

@ZombineDev @andralex @wilzbach
I can work on adding the struct but for pre FreeBSD 12 versions.
Is that acceptable, or is this still blocked by the FreeBSD 12 issue?

Thanks

jmdavis · 2018-03-29T17:49:55Z

I can work on adding the struct but for pre FreeBSD 12 versions.
Is that acceptable, or is this still blocked by the FreeBSD 12 issue?

Officially, we only support the latest version of FreeBSD, which is currently 11. We do need to sort out the 12 stuff, but that shouldn't be a blocker for adding missing druntime bindings. It just means that we're then going to need to do more work on the bindings to make them work for 12, and while how to fix that has been discussed, AFAIK, that hasn't actually been sorted out yet. So, I'd say that you should just add the bindings that match for 11 so that 11 works, and then if changes are necessary to make 12 work, then they can be made with the other changes that need to be made to make 12 work.

jercaianu · 2018-03-30T19:29:14Z

@jmdavis @ZombineDev @andralex
Tested on FreeBSD 11 and correctly returns the available disk space

PetarKirov reviewed Oct 11, 2017

View reviewed changes

andralex reviewed Oct 16, 2017

View reviewed changes

dlang-bot added Needs Rebase needs a `git rebase` performed Needs Work and removed Needs Rebase needs a `git rebase` performed labels Jan 1, 2018

dkgroot mentioned this pull request Jan 7, 2018

Add DragonFlyBSD support : Split PR : src/core/sys/dragonflybsd related changes #2002

Merged

dlang-bot added the stalled label Jan 14, 2018

jercaianu mentioned this pull request Mar 28, 2018

Fix Issue 16487 - Add function to obtain the available disk space dlang/phobos#5776

Merged

jercaianu force-pushed the statfs branch from 1fdd897 to b51b858 Compare March 30, 2018 19:11

dlang-bot removed Needs Work stalled labels Mar 30, 2018

jercaianu force-pushed the statfs branch from b51b858 to d8e8c29 Compare March 30, 2018 19:19

Added statfs for FreeBSD 11

bb0df19

jercaianu force-pushed the statfs branch from d8e8c29 to bb0df19 Compare March 30, 2018 19:31

andralex approved these changes Mar 30, 2018

View reviewed changes

andralex added the auto-merge label Mar 30, 2018

dlang-bot merged commit 3cb0da3 into dlang:master Mar 30, 2018

Uh oh!

Added statfs for FreeBSD #1937

Added statfs for FreeBSD #1937

Uh oh!

Conversation

jercaianu commented Oct 11, 2017

Uh oh!

dlang-bot commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jercaianu commented Mar 29, 2018

Uh oh!

jmdavis commented Mar 29, 2018

Uh oh!

jercaianu commented Mar 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dlang-bot commented Oct 11, 2017 •

edited

Loading

PetarKirov Oct 11, 2017 •

edited

Loading

PetarKirov commented Oct 16, 2017 •

edited

Loading