-
-
Notifications
You must be signed in to change notification settings - Fork 135
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
FUSE (WIP) #891
base: master
Are you sure you want to change the base?
FUSE (WIP) #891
Conversation
SignalGuard sguard; | ||
|
||
managarm::posix::MountRequest<MemoryAllocator> req(getSysdepsAllocator()); | ||
req.set_path(frg::string<MemoryAllocator>(getSysdepsAllocator(), source)); | ||
req.set_target_path(frg::string<MemoryAllocator>(getSysdepsAllocator(), target)); | ||
req.set_fs_type(frg::string<MemoryAllocator>(getSysdepsAllocator(), fstype)); | ||
if(data){ | ||
req.set_arguments(frg::string<MemoryAllocator>(getSysdepsAllocator(), (char*)data)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the extra data always a string? (Is it just the mount options as specified by -o
to mount(8)
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of any cases in which this data isn't a string, but I don't think it's is required to be a string. This is mainly a temporary measure until I can think of a better way to approach this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mount(2)
on my system says:
The data argument is interpreted by the different filesystems. Typically it is a string of comma-separated options understood by this filesystem. See mount(8) for details of the options available for each filesystem type.
I think that, practically speaking, we would be fine by just interpreting this as a string in almost any occasion. However, I don't think it's technically correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems fine to me then. Since the man page directs you to mount(8)
then it'd be difficult to specify binary data (maybe check mount
source code?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might check the source later, but it's not very high-priority for me. I was thinking about implementing some sort of fs-specific argument decoding dispatch mechanism, but I'm not sure how I'd go about doing this and it'd probably require the sysdep to be aware of every possible filesystem - which would be impractical at best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just throwing around ideas here, but isn't it also possible to have a pushMemory
action similarly to how we have a pushDescriptor
action that pushes an arbitrary piece of memory across process boundaries? Or maybe a mechanism which turns a piece of memory into a descriptor? IIRC that would also help at other points in the managarm sysdeps.
1852fcc
to
20c55c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few items to fix
@@ -40,6 +40,11 @@ extern "C" { | |||
|
|||
#ifndef __MLIBC_ABI_ONLY | |||
|
|||
#define MNT_FORCE 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double define
@@ -40,6 +40,11 @@ extern "C" { | |||
|
|||
#ifndef __MLIBC_ABI_ONLY | |||
|
|||
#define MNT_FORCE 1 | |||
#define MNT_DETACH (1<<1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Styling is wrong (see the constants defined earlier in the file.
@@ -40,6 +40,11 @@ extern "C" { | |||
|
|||
#ifndef __MLIBC_ABI_ONLY | |||
|
|||
#define MNT_FORCE 1 | |||
#define MNT_DETACH (1<<1) | |||
#define MNT_EXPIRE (1<<2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move these defines outside the MLIBC_ABI_ONLY define. Also please make sure these new defines match Linux. Better yet, we should really move these to an abi-bit header, as I'm pretty sure that at least some of the constants in this file don't match Linux.
#define X(x) case x: s = #x; break; | ||
switch(e) { | ||
X(EAGAIN) | ||
X(EACCES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be indented one more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case statements I replaced were on the same indentation level - but I can indent it a level more of desired
sysdeps/managarm: implement sys_umount2()
This PR is the mlibc counterpart of managarm#533. None of the changes in this PR are necessary for FUSE functionality, however they aid in porting libfuse to managarm.