Fix #907

vk commented

2025-01-04 11:51:43 +00:00

Contributor

A naive fix for #907. Successfully tested on FreeBSD.

vk added 1 commit 2025-01-04 11:51:44 +00:00

Fix #907

ci/woodpecker/pr/debug Pipeline failed

Details

b568bb863d

vk added 1 commit 2025-01-04 13:46:48 +00:00

formatting

ci/woodpecker/pr/debug Pipeline failed

Details

6ca99fd02c

vk commented

2025-01-04 13:48:39 +00:00

Author

Contributor

Would you guys mind taking a look and fixing the formatting, I have no clue what I'm doing wrong?

Armael commented

2025-01-04 14:09:42 +00:00

Owner

Did you try running cargo fmt from the root of the project? This should reformat the code as appropriate.

Did you try running `cargo fmt` from the root of the project? This should reformat the code as appropriate.

👍 1

lx commented

2025-01-04 15:07:25 +00:00

Owner

With this implementation, on FreeBSD we will now make two calls, one to statfs and one to statvfs. I think we can avoid this and also make the code simpler, by just making the current mount_avail function conditionnal to #[cfg(not(target_os="freebsd"))], and rewrite a freebsd-specific version that uses statfs instead of statvfs. This would avoid adding lines 850-860 and we can keep 843-848 as is on all platforms.

With this implementation, on FreeBSD we will now make two calls, one to statfs and one to statvfs. I think we can avoid this and also make the code simpler, by just making the current mount_avail function conditionnal to `#[cfg(not(target_os="freebsd"))]`, and rewrite a freebsd-specific version that uses statfs instead of statvfs. This would avoid adding lines 850-860 and we can keep 843-848 as is on all platforms.

vk commented

2025-01-04 15:40:54 +00:00

Author

Contributor

With this implementation, on FreeBSD we will now make two calls, one to statfs and one to statvfs. I think we can avoid this and also make the code simpler, by just making the current mount_avail function conditionnal to #[cfg(not(target_os="freebsd"))], and rewrite a freebsd-specific version that uses statfs instead of statvfs. This would avoid adding lines 850-860 and we can keep 843-848 as is on all platforms.

Indeed, but that's intentional. statfs is deprecated on most platforms, by statvfs, which is furthermore part of the POSIX standard. Hence, statfs is being used barely as a workaround the filesystem_id issue. Moreover, IIRC (I'll check further), manipulating statfs results requires knowledge of the underlying filesystem (whether it's ufs, zfs, ...) in order to exploit the returned data, which is what statvfsdoes.

> With this implementation, on FreeBSD we will now make two calls, one to statfs and one to statvfs. I think we can avoid this and also make the code simpler, by just making the current mount_avail function conditionnal to `#[cfg(not(target_os="freebsd"))]`, and rewrite a freebsd-specific version that uses statfs instead of statvfs. This would avoid adding lines 850-860 and we can keep 843-848 as is on all platforms. Indeed, but that's intentional. `statfs` is deprecated on most platforms, by `statvfs`, which is furthermore part of the POSIX standard. Hence, `statfs` is being used barely as a workaround the `filesystem_id` issue. Moreover, IIRC (I'll check further), manipulating `statfs` results requires knowledge of the underlying filesystem (whether it's `ufs`, `zfs`, ...) in order to exploit the returned data, which is what `statvfs`does.

lx commented

2025-01-04 15:46:10 +00:00

Owner

Ok, that's fine then!

If you give me write access to your forked repo I can run the correct cargo fmt command for you

Ok, that's fine then! If you give me write access to your forked repo I can run the correct cargo fmt command for you

vk commented

2025-01-04 15:48:47 +00:00

Author

Contributor

It seems I was partly wrong, statvfs seems to barely copy the statfs results (it acts as a dumb wrapper).

static int
sfs2svfs(const struct statfs *from, struct statvfs *to)
{
        static const struct statvfs zvfs;

        *to = zvfs;

        if (from->f_flags & MNT_RDONLY)
                to->f_flag |= ST_RDONLY;
        if (from->f_flags & MNT_NOSUID)
                to->f_flag |= ST_NOSUID;

        /* XXX should we clamp negative values? */
#define COPY(field) \
        do { \
                to->field = from->field; \
                if (from->field != to->field) { \
                        errno = EOVERFLOW; \
                        return (-1); \
                } \
        } while(0)

        COPY(f_bavail);
        COPY(f_bfree);
        COPY(f_blocks);
        COPY(f_ffree);
        COPY(f_files);
        to->f_bsize = from->f_iosize;
        to->f_frsize = from->f_bsize;
        to->f_favail = to->f_ffree;
        return (0);
}

Yet, I'd still stick to using statfvs for its intended purpose, but I may refactor this as you decide.

It seems I was partly wrong, `statvfs` seems to barely copy the `statfs` results (it acts as a dumb wrapper). ``` static int sfs2svfs(const struct statfs *from, struct statvfs *to) { static const struct statvfs zvfs; *to = zvfs; if (from->f_flags & MNT_RDONLY) to->f_flag |= ST_RDONLY; if (from->f_flags & MNT_NOSUID) to->f_flag |= ST_NOSUID; /* XXX should we clamp negative values? */ #define COPY(field) \ do { \ to->field = from->field; \ if (from->field != to->field) { \ errno = EOVERFLOW; \ return (-1); \ } \ } while(0) COPY(f_bavail); COPY(f_bfree); COPY(f_blocks); COPY(f_ffree); COPY(f_files); to->f_bsize = from->f_iosize; to->f_frsize = from->f_bsize; to->f_favail = to->f_ffree; return (0); } ``` Yet, I'd still stick to using `statfvs` for its intended purpose, but I may refactor this as you decide.

vk commented

2025-01-04 15:49:49 +00:00

Author

Contributor

Ok, that's fine then!

If you give me write access to your forked repo I can run the correct cargo fmt command for you

I'll do, but I'll still try doing it by myself. I'm a total noob in rust and tooling but I believe I can do that :)

> Ok, that's fine then! > > If you give me write access to your forked repo I can run the correct cargo fmt command for you I'll do, but I'll still try doing it by myself. I'm a total noob in rust and tooling but I believe I can do that :)

vk commented

2025-01-04 15:51:48 +00:00

Author

Contributor

Ok, that's fine then!

If you give me write access to your forked repo I can run the correct cargo fmt command for you

I'll do, but I'll still try doing it by myself. I'm a total noob in rust and tooling but I believe I can do that :)

Ok, cargo fmt does reformat my patch but also other files I haven't touched, I'll push mine only, that okay?

> > Ok, that's fine then! > > > > If you give me write access to your forked repo I can run the correct cargo fmt command for you > > I'll do, but I'll still try doing it by myself. I'm a total noob in rust and tooling but I believe I can do that :) Ok, `cargo fmt` does reformat my patch but also other files I haven't touched, I'll push mine only, that okay?

vk added 1 commit 2025-01-04 15:52:28 +00:00

Formatting with

ci/woodpecker/pr/debug Pipeline was successful

Details

6689800986

lx commented

2025-01-04 16:07:34 +00:00

Owner

LGTM, thanks for your contribution :)

👍 1