Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

document-portal: Make possible to use on systems without splice support #1318

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arrowd
Copy link
Contributor

@arrowd arrowd commented Mar 31, 2024

No description provided.

Copy link
Contributor

@swick swick left a comment

Choose a reason for hiding this comment

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

What problem is this trying to solve?

@@ -2010,7 +2012,7 @@ xdp_fuse_read (fuse_req_t req,
buf.buf[0].fd = file->fd;
buf.buf[0].pos = off;

fuse_reply_data (req, &buf, FUSE_BUF_SPLICE_MOVE);
Copy link
Contributor

Choose a reason for hiding this comment

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

From the docs it sounds like FUSE_BUF_SPLICE_MOVE should be able to fall back to not using splice. Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I understand, the docs mean that a splice move may decay to copying in some circumstances, but it still requires that the splice feature is supported by the FUSE server.

document-portal/document-portal-fuse.c Outdated Show resolved Hide resolved
@arrowd
Copy link
Contributor Author

arrowd commented Apr 4, 2024

What problem is this trying to solve?

A FUSE client should not assume that the server supports some features. The FreeBSD implementation of FUSE doesn't implement that splice stuff which makes the code fail at the early initialization step.

document-portal/document-portal-fuse.c Outdated Show resolved Hide resolved
document-portal/document-portal-fuse.c Outdated Show resolved Hide resolved
document-portal/document-portal-fuse.c Outdated Show resolved Hide resolved
document-portal/document-portal-fuse.c Outdated Show resolved Hide resolved
document-portal/document-portal-fuse.c Show resolved Hide resolved
document-portal/document-portal-fuse.c Outdated Show resolved Hide resolved
document-portal/document-portal-fuse.c Outdated Show resolved Hide resolved
document-portal/document-portal-fuse.c Outdated Show resolved Hide resolved
document-portal/document-portal-fuse.c Outdated Show resolved Hide resolved
document-portal/document-portal-fuse.c Outdated Show resolved Hide resolved
@smcv
Copy link
Collaborator

smcv commented Apr 8, 2024

The FreeBSD implementation of FUSE doesn't implement that splice stuff which makes the code fail at the early initialization step.

That seems like information that should be in the commit message and PR description, and/or in an issue report of the form "document-portal: Doesn't start on FreeBSD" that's closed by this PR.

@swick
Copy link
Contributor

swick commented Apr 10, 2024

Code LGTM but I can't judge the details of the FUSE changes.

@arrowd arrowd marked this pull request as draft April 23, 2024 07:07
@arrowd
Copy link
Contributor Author

arrowd commented Apr 28, 2024

Turns out that xdp_fuse_init_cb is called not during fuse_session_new but later so this approach doesn't really work.

I'm thinking of a meson option that will set this compile time. Would that be acceptable?

…pport

The FreeBSD implementation of FUSE doesn't have splice features, which makes
the code fail at the early initialization step.
@arrowd arrowd marked this pull request as ready for review April 30, 2024 13:37
@arrowd
Copy link
Contributor Author

arrowd commented Apr 30, 2024

I made Meson check for splice function to enable WITH_SPLICE CPP define. This is the same as what libfuse does: https://github.com/libfuse/libfuse/blob/8a9e2dc20c82259c9ded075f39383432c90d9786/meson.build#L67

@arrowd
Copy link
Contributor Author

arrowd commented Jun 10, 2024

Bump. Can we get this in?

Copy link
Member

@GeorgesStavracas GeorgesStavracas left a comment

Choose a reason for hiding this comment

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

Two small nitpicks, but generally this looks fine to me

if (se == NULL)
{
g_set_error (&thread_data->error, XDG_DESKTOP_PORTAL_ERROR,
XDG_DESKTOP_PORTAL_ERROR_FAILED,
"Can't create fuse session");
g_free (fuse_opts);
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: g_clear_pointer (&fuse_opts, g_free);

g_debug ("DESTROY");

/* Ensure we call this on the main thread */
g_idle_add ((GSourceFunc) on_fuse_unmount, NULL);

g_free (fuse_opts);
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: g_clear_pointer (&fuse_opts, g_free);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Triage
Development

Successfully merging this pull request may close these issues.

None yet

4 participants