Unnecessary error messages for non-errors

willscott / go-nfs

golang NFSv3 server

Apache License 2.0

684 stars 74 forks source link

Unnecessary error messages for non-errors #117

Closed rminnich closed 5 months ago

rminnich commented 9 months ago

While using go-nfs for github.com/u-root/sidecore, I see messages like this: 2023/12/29 20:47:15 [ERROR] No file for lookup of debconf-communicate 20:47:15 [ERROR] call to 0x102a2b490 failed: No such file or directory: file does not exist

I don't think this should be an error, or, at least, more information would be helpful. I'm still trying to see why this happens. Any ideas?

Thanks.

rminnich commented 9 months ago

oh ho, it's supposed to be there but is not. It's almost as though it came back from a readdir but the stat failed?

One thing that would be helpful is some way to enable per-request logging, since it is not always possible to use wireshark (I'm running nfs3 over ssh).

More as I learn it.

rminnich commented 9 months ago

Also, I should know this, but what is 0x10431b490

rminnich commented 9 months ago

ah, ok, it seems to be the kernel trying to do something? maybe set atime? still looking.

rminnich commented 9 months ago

OK, the code for lookup I think could be changed. nfs_onlookup should be able to use Lstat, not Readdir, as readdir is a very inefficient way to do a lookup (just image a directory with 10k+ entries and repeated lookups ... this happens. Or 1M. This happens).

I'm surprised that billy has no lookup operation, but I think Lstat would serve. Thoughts?

DUOLabs333 commented 9 months ago

I was actually thinking the same thing (in practice, directories are not usually large enough to make a difference, but it's a simple optimization to make).

rminnich commented 9 months ago

I just discovered LOG_LEVEL, which removes the extra prints. It seems to default to LOG_ERROR? I think it ought to be a higher level. Although, in general, log messages of any kind in a Go package are frowned upon ...

willscott commented 9 months ago

when using this library in a larger program, you can connect it up to whatever logging system is in use via SetLogger https://github.com/willscott/go-nfs/blob/master/log.go#L61
I think you're right that it may be worthwhile to differentiate between recoverable/non-recoverable errors at the library versus at the protocol level.
The inefficiency in onlookup is a good call. I'll file a separate issue to track that.

rminnich commented 5 months ago

https://github.com/willscott/go-nfs/blob/ea1b85ea632bdbe1417274b7f55c93aa30c298f5/conn.go#L130

I'd suggest removing this print. especially for shells, which do a lot of stats for PATH resolution, I am getting a lot of prints.

This is a file system error, and there are always a lot of these, and they are not really NFS-level failures,