Added FUSE support #55
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
itctlor developing 3rd party tools.Features
1a5970a4d3bc1ed401a5970a0) 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
startFUSEinstead 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
BuildRootNodebecause 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 devicefiles, _ := 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 eventfor 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 socketif k.Bool("fuse.enabled") {FUSE should be started before the socket (socket should always be last)
Done in
3b96901I'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 failedHow would you recommend doing this? I suppose it could fail for all sorts of reasons such as
ENOENTEIOEINVALECONNABORTEDECONNREFUSEDECONNRESETEISNAMIn other places such as when we open a file, we could have
EISDIREEXISTAgain, I'm sorry but I'm not a Go expert and don't really now how to do this properly.. especially when dealing with
FSErrorThe
errcan be several different kinds of errors, andFSErroris 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 anerrorand 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
TODOwhere 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 FSos.Mkdir(k.String("fuse.mountpoint"), 0755)Use
os.MkdirAllhere 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
fusefspackage, add a file calledunmount.gowith the following contents:unmount.goThen, you can simply call that function at the top of
startFUSElike 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 errreturn errerris returned twice hereOh, sorry, fixed now
@@ -0,0 +97,4 @@// root folderr := 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 tocatthe 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 useitctlif 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/hidraw0I 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
catexample. This works perfectly for me:In the
itdlog 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 timecattries 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
itctlcan 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 0This 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!