Added FUSE support #55
No reviewers
Labels
No Label
Bug
Cantfix
Duplicate
Enhancement
External
Help Wanted
In Progress
Invalid
Question
Waiting
Waiting For Info
Waiting for Upstream
Wontfix
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: Elara6331/itd#55
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "yannickulrich/itd:fuse"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This exposes the watches' file system over FUSE. This way, we can access files on the watch without having to go through
itctl
or developing 3rd party tools.Features
1a5970a
4d3bc1ed401a5970a0
) d2dbcd8713Thanks for your PR. There are some changes I'd like, mostly readability improvements.
@ -0,0 +9,4 @@
"go.arsenm.dev/infinitime"
)
func startFuse(ctx context.Context, dev *infinitime.Device) error {
This function should be called
startFUSE
instead to adhere to Go naming conventionsThanks, I'm relatively new to Go, sorry for these issues
Done in
673383f
@ -0,0 +38,4 @@
func BuildRootNode(dev *infinitime.Device) *ITNode {
inodemap = make(map[string]uint64)
myfs, _ = dev.FS()
This error needs to be handled, you can just return it in this case and update the code that calls this function to do the actual handling
Done in
a54ca7a
@ -0,0 +81,4 @@
var myfs *blefs.FS = nil;
These variables should go above
BuildRootNode
because it's using them and it would be more readable that way. Also, Go doesn't require semicolons, you can remove those.Done in
673383f
@ -0,0 +119,4 @@
case 2:
// on device
files, _ := myfs.ReadDir(n.path)
You should definitely handle this error
Done in
a54ca7a
@ -0,0 +161,4 @@
return fs.NewListDirStream(r), 0
}
var _ = (fs.NodeLookuper)((*ITNode)(nil))
Same for all the other interface checks
Done in
2396623
@ -0,0 +298,4 @@
go func() {
// For every progress event
for sent := range fp.Progress() {
log.Info("Progress").Int("bytes", int(sent)).Int("of", len(fh.content)).Send();
Done in
b5328ec
@ -0,0 +330,4 @@
var _ = (fs.NodeGetattrer)((*ITNode)(nil))
func (bn *ITNode) Getattr(ctx context.Context, f fs.FileHandle, out *fuse.AttrOut) syscall.Errno {
log.Info("getattr").Str("path", bn.path).Send();
These should be debug logs rather than info logs. Also, the message should be a bit more specific, something like "FUSE getattr". Same for all the similar logs.
Done in
b5328ec
@ -0,0 +483,4 @@
}
node := f.NewInode(ctx, operations, stable)
log.Info("Mkdir sucess").
Minor typo, it's "success"
Done in
b5328ec
@ -187,2 +187,4 @@
log.Error("Error starting socket").Err(err).Send()
}
// Start fuse socket
if k.Bool("fuse.enabled") {
FUSE should be started before the socket (socket should always be last)
Done in
3b96901
I've just realised that the error codes aren't correct. I'll fix this next
@ -0,0 +126,4 @@
files, err := myfs.ReadDir(n.path)
if err != nil {
log.Error("FUSE ReadDir failed").Str("path", n.path).Err(err).Send()
// TODO we probably should figure out why it failed
How would you recommend doing this? I suppose it could fail for all sorts of reasons such as
ENOENT
EIO
EINVAL
ECONNABORTED
ECONNREFUSED
ECONNRESET
EISNAM
In other places such as when we open a file, we could have
EISDIR
EEXIST
Again, I'm sorry but I'm not a Go expert and don't really now how to do this properly.. especially when dealing with
FSError
The
err
can be several different kinds of errors, andFSError
is just one of them. It's actually a type I made. You can see it here: https://gitea.arsenm.dev/Arsen6331/infinitime/src/commit/512d48bc2469/blefs/error.go#L20. It contains an error code you can check to see what went wrong, and you can scroll down to see the meaning of each code.In this case, I'd use a Go type switch to check which error type actually occurred and then check the code or do whatever else needs to be done. Maybe there could be a function like
syscallErr()
that takes anerror
and returns the proper syscall error?If you don't feel comfortable doing that, I can merge this and then implement it myself and send you the commit so you can see how I did it, or you can just try it yourself, whatever you feel would be better.
Something like in 4c59561a99? There are a few
TODO
where I'm not sure what the correct POSIX error would be and improvised. If you have a better idea, feel free to change them thoughThat looks good, thanks
Sorry it's taken me a while to respond. I wanted to test this PR but my PineTime wasn't booting. Now that I've gotten it to boot, I'll test this and get back to you.
@ -0,0 +11,4 @@
func startFUSE(ctx context.Context, dev *infinitime.Device) error {
// This is where we'll mount the FS
os.Mkdir(k.String("fuse.mountpoint"), 0755)
Use
os.MkdirAll
here instead so that it creates parent directories, and handle the error (just return it).This is actually a bit more complicated than that. If the mountpoint already exists, fuse will crash. We also can't delete the mountpoint beforehand (
rm: cannot remove '/tmp/itd/mnt': Transport endpoint is not connected
). The best way to solve this should be calling the unmount function. How would you suggest going about doing this?Yeah, this one is going to be a bit more complicated. The FUSE library does have a different unmount function that you could call before trying to mount the fs, but it's not exported, so we'll need to do a small hack to get access to it anyway. In the
fusefs
package, add a file calledunmount.go
with the following contents:unmount.go
Then, you can simply call that function at the top of
startFUSE
like so:I'll file an issue with the fuse library to export that function once this is merged.
I agree that this should be done. However, this doesn't solve the problem completely because if ITD panics for whatever reason, it won't get run and then the user will get a confusing message. Let me see what the best way would be to call this function on shutdown.
Cool, thank you! I have implemented this 🙂
@ -0,0 +18,4 @@
Err(err).
Send()
return err
return err
err
is returned twice hereOh, sorry, fixed now
@ -0,0 +97,4 @@
// root folder
r := make([]fuse.DirEntry, 2)
r[0] = fuse.DirEntry{
Name: "device",
In my opinion, "device" is unclear. I think it should be called "info" instead, that seems clearer.
Another thing is that you seem to be providing continuous sensor data by never sending an EOF, which means files will be read forever. Files generally don't work like that, so it leads to consequences. Since the syscall for reading from the file will never return until new information is received, a program that reads from this file will not be able to be killed, even by
SIGKILL
. You can see this if you try tocat
the file. It can also break your system's shell depending on how it's configured. Instead, I think you should just provide single data points, and people will just have to useitctl
if they want continuous data, because files aren't meant to work this way.Happy to change that.
Regarding the continuous data stream, isn't this how device files work under Linux? If I run
cat /dev/hidraw0
I get a live stream of data. I was hoping to get the same behaviour. I'm happy to try and fix any bug but think this is probably the best way of doing this. Alternatively, we can have to files, a live stream file (eg./tmp/itd/mnt/live/motion
) and a one-shot file (eg./tmp/itd/mnt/info/motion
). Would you be opposed to this?I'm not 100% sure what you mean with the
cat
example. This works perfectly for me:In the
itd
log I also see the Done method of the watcher getting called. Could you perhaps elaborate on the problem you see?Try this with something that returns data less commonly, such as
battery
. Every timecat
tries to read from the file, it executes a syscall. Since data isn't returned until ITD receives it from the watch, the syscall doesn't return until then either. What this means is that whatever program is reading the file will be executing kernel code, which prevents you from killing it.Linux does this kind of stuff with a char device file. Unfortunately, FUSE doesn't provide a way to make a char device. There seems to be a workaround, but it only works for the root user, so it would not work in ITD. I did test that, and I can confirm it doesn't work.
Okay, fair enough. Sorry for doubting you. How do you feel about keeping a stream-type file for motion? I'm not 100% sure how much sense the instantaneous motion makes.. Otherwise, I'm going to remove the entire channel stuff.
Yeah, instantaneous motion data doesn't make much sense, but the continuous motion data file also doesn't work all the time. InfiniTime will stop sending motion data if you have Raise To Wake and ShakeWake disabled and you put the watch to sleep with the button. This will cause programs to once again read forever without the ability to be killed, so I think it would be better to remove the continuous data entirely. I would love to have it, but if it breaks most programs, it's not worth it, especially since
itctl
can be used instead.@ -0,0 +299,4 @@
var _ fs.FileFlusher = (*bytesFileWriteHandle)(nil)
func (fh *bytesFileWriteHandle) Flush(ctx context.Context) (errno syscall.Errno) {
if len(fh.content) == 0 {
return 0
This is not correct. If no content has been written, you should still create the file because the user might want to create an empty file using a command like
touch
.Oh, good point, thank you. Should be fixed now.
84c7a33
: added unmount.go 613d33ff4dSorry it took me so long to respond but I have now removed the live data thing. This simplifies the
sensorFileReadHandle
(008f6b3
) since it just needs to contain the content of the buffer and send it out in due timeHope this is what you wanted?
PS: when merging, it's probably a good idea to squash the commits since this has gotten quite messy.. sorry..
I just tested it, and it looks good to me, merging now. Thanks!