FreeBSD support #532

arrowd · 2020-09-28T16:22:24Z

No description provided.

arrowd · 2020-10-21T10:03:06Z

Bump. Can this be merged in, please?

src/xdp-utils.c

freebsd-stubs.h

configure.ac

freebsd-stubs.h

document-portal/document-portal-fuse.c

valpackett · 2020-11-09T00:53:09Z

Heh, I did some changes on my own without looking at PRs, now found this :)

I managed to get the document provider to add a file to the FUSE fs, since we don't have flatpak, this involved hacks:

xdp_app_info_new_host returning non-empty string (to actually test the FS — originally for the host it just returns normal paths and doesn't use the document-provider)
commenting out parse_app_info_from_flatpak_info calls
in xdp_app_info_get_path_for_fd changing /proc/self/fd to /compat/linux/dev/fd where I mounted fdescfs -o linrdlnk

Also from fusermount_argv I needed to remove auto_unmount, it fails otherwise because that's not a recognized option.

But honestly the FUSE based document provider might not be the most useful thing for us…

if anyone cares about actual FreeBSD sandboxing read here

…unless someone wants to do something like bubblewrap based on jails, but why would you do that when we have some better ideas for sandboxing :) That is, I'd like to use the document portal with capsicumizer eventually, and for that I literally just need to pass the fd to the sandboxed application. Basically all I want is for org.freedesktop.portal.FileChooser to return fds in response to OpenFile etc. But I think I know how to fit this into the existing APIs, with a custom document provider and full control of what open() does in the sandboxed process, I can send the fds to the process from the provider, and return a special path that would be handled by the open() hook.

tl;dr maybe exclude all of document-portal from the build on FreeBSD at least for now, the patch would be really small then

arrowd · 2020-11-09T16:59:12Z

Thanks for your review. I hope I've addressed all your comments.

document-portal/document-portal-fuse.c

valpackett · 2020-11-09T20:30:13Z

Looks good!

jhenstridge · 2020-11-26T13:43:08Z

This looks like it substantially changes the behaviour of the file system. An O_PATH file descriptor is very different from a regular file descriptor, so doing #define O_PATH 0 is almost certainly incorrect.

For example, consider this call:

  o_path_fd = openat (dirfd, name, O_PATH|O_NOFOLLOW, 0);

If the file path being opened is a symbolic link, on Linux the call will succeed returning an O_PATH fd representing the symlink itself (not what the symlink points to). On FreeBSD the call will fail because O_NOFOLLOW on its own says to reject paths containing symbolic links.

valpackett · 2020-11-26T17:43:18Z

O_PATH support is mostly just disabled here https://github.com/flatpak/xdg-desktop-portal/pull/532/files#diff-73d3b0687424be897264ee39ca7f8d5f337fd226e4071f83d3e0b85877b9be37R945 — the define is used in some places for convenience, I guess ideally it would be ifdef'd out everywhere, sure

jhenstridge · 2020-11-27T00:44:48Z

The example I gave was from the document portal code, where this PR defines O_PATH to zero:

xdg-desktop-portal/document-portal/document-portal-fuse.c

Lines 1597 to 1599 in 2fe4f30

    
           o_path_fd = openat (dirfd, name, O_PATH|O_NOFOLLOW, 0); 
        
           if (o_path_fd == -1) 
        
               return -errno;

The code might compile, but it now means something different now.

arrowd · 2020-12-01T10:49:33Z

Would openat (dirfd, name, 0, 0); work, then?

valpackett · 2020-12-01T10:56:18Z

maybe just disable document-portal and don't port it

jhenstridge · 2020-12-01T11:18:34Z

My point was that it changes the meaning. For example, the /var/run path on my system is a symlink to /run. Here's how the two file descriptor types look (using Python for convenience):

>>> fd1 = os.open('/var/run', os.O_PATH|os.O_NOFOLLOW)
>>> stat.S_ISLNK(os.fstat(fd1).st_mode)
True
>>> fd2 = os.open('/var/run', os.O_RDONLY)
>>> stat.S_ISLNK(os.fstat(fd2).st_mode)
False
>>> stat.S_ISDIR(os.fstat(fd2).st_mode)
True

One file descriptor represents the symlink, and the other represents the symlink target: i.e. the difference between calling lstat and stat on a path. And the reason the code is doing all this with file descriptors rather than paths is to avoid time-of-check/time-of-use race conditions.

arrowd · 2020-12-20T14:05:38Z

I have removed O_PATH changes for now. Let's at least merge trivial build fixes.

arrowd · 2021-01-20T10:14:11Z

Bump. Can this be merged in?

arrowd · 2021-02-10T13:36:10Z

Another bump.

valpackett · 2021-03-18T16:17:10Z

An O_PATH implementation patch now exists https://reviews.freebsd.org/D29323

arrowd · 2021-04-15T11:07:26Z

I'd still prefer to get this PR merged as is first.

arrowd · 2021-04-21T14:52:26Z

It's been more than 6 months since this MR was submitted. Is there something wrong with it? Can we finally make it in?

arrowd · 2021-05-19T17:08:28Z

Monthly bump.

arrowd · 2022-10-11T13:10:09Z

Another bump.

meson.build

jadahl · 2022-10-11T13:29:45Z

These changes seems mostly harmless to me, but I can't tell whether the lack of flags added in the document portal has any unwanted consequences.

It's also surprising the commits that just ifdef some header inclusion doesn't break compilation. Are those includes needed to begin with?

arrowd · 2022-10-11T15:07:44Z

It's also surprising the commits that just ifdef some header inclusion doesn't break compilation. Are those includes needed to begin with?

FreeBSD may have functions defined in different headers, so the first thing I try when I stumble upon "header not found error" is just removing it to see if the stuff still compiles.

ngortheone · 2022-10-22T03:05:47Z

@arrowd thanks for your work. Will this allow us to upgrade to 1.15 version?

…and meson.build This is required to build the code on non-Linux OS like FreeBSD.

Check for <sys/statfs.h> and <sys/mount.h> presence before including them. Fixes build on FreeBSD.

Check for <sys/vfs.h> and <sys/mount.h> presence before including them. Fixes build on FreeBSD.

- Check for non-portable header presence before including them. - Define uncommon error code `ENODATA`. - Guard the usage of uncommon flags with appropriate #ifdef. - Use `posix_fallocate` on non-Linux platforms. Fixes build of document-portal/document-portal-fuse.c on FreeBSD.

arrowd · 2022-10-24T17:47:39Z

@arrowd thanks for your work. Will this allow us to upgrade to 1.15 version?

If this ever gets merged in. Speaking of which pinging again.

ngortheone · 2022-10-24T18:58:10Z

@GeorgesStavracas please merge

ngortheone · 2022-10-24T19:05:48Z

@smcv maybe you could help too? 🙏

GeorgesStavracas · 2022-10-24T19:11:55Z

It will be merged when it's considered ready. Please do not post comments asking for merging.

ngortheone · 2022-10-24T19:50:33Z

@GeorgesStavracas sorry for reaching out to you in this manner. Gleb has been working hard and diligently addressing all comments for 2 years. Please understand that all this time we have to maintain a private fork/patchset to make xdg-desktop-portal usable on FreeBSD. And following upstream becomes more difficult.

Maybe instead of gatekeeping the change and trying to make it perfect we you could grant @arrowd maintainer access to the project? It would help FreeBSD greatly to have an ambassador in freedesktop related projects. We are as interested in keeping this project working as you are.

Thanks

EDIT:
Maybe this is worth moving into a separate issue/thread:
@arrowd is a member of FreeBSD desktop team and he is helping us maintaining the tech stack. I couldn't find contributing guidelines for this repo (pls point me to the right location if they exit) but I would like raise a request for granting maintainer access to a member of Freebsd desktop team.

cc: @emaste

GeorgesStavracas · 2022-10-24T20:59:46Z

Asking to grant someone without previous contributions to a project full access to the repository is absolutely unreasonable. "Gatekeeping" changes and "trying to make them perfect" is standard procedure in software development, we call it "code review", and having maintainer access to the project does not imply permission to push without somebody else review it.

@ngortheone honestly, I appreciate you have the best of intentions here, but dealing with your misguided commentary has devoided me of more energy than another round of reviews. @arrowd has already done an outstanding job in applying review comments and got this pull request on the verge of being merged. Pinging random people you consider important won't make this go any faster, very likely will have the opposite effect. Please, disengage.

GeorgesStavracas

Only a minor nit now

meson.build

If it is not found, do not try to run icon validation in the sandbox.

GeorgesStavracas

We've briefly discussed this on the Flatpak Matrix channel today, and generally there are no objections to landing this given that the enablement work is mostly sprinkling ifdefs around. I'd like to make it clear that landing this does not imply any kind of "official" support, and the xdg-desktop-portal project still relies on BSD folks to actively monitor the project and send pull requests if the build breaks. We're not adding any BSD-based CI step due to that not being a core requirement of the project. These patches are appreciated, and I hope you continue to contribute to the project in the future.

arrowd · 2022-10-26T09:29:24Z

Of course, we do plan to upstream any local patches that we come up with.

Is there any ETA for the next release date that would contain this change?

GeorgesStavracas · 2022-10-26T14:58:52Z

The 1.16 milestone is set to December 1st