I'm pretty confident with my changes to fuse_device.c, but
fuse_vfsops.c wasn't that obvious.
Details
make buildkernel
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
This is an interesting new feature. But how do we use it? There aren't any examples yet, or any man page. We could easily add test cases for the mount and kqueue errors to the fusefs test suite.
sys/fs/fuse/fuse_device.c | ||
---|---|---|
412 | "SET_ERROR0" includes the "ktrexterr" dtrace probe. That makes this custom SDT probe redundant. I think you should delete it. Also, delete every other "trace" probe that's adjacent to SET_ERROR0. The fusefs "trace" probes aren't very useful anyway. They're leftover from the original GSoC contribution. I rarely use them. | |
462 | ||
530 | It's no longer a "Linux" errno by this point. | |
580 | Because ohead.unique == 0, we aren't looking at an error code. We're looking at a notification code. And it isn't necessarily unknown; it could be a fallthrough from one of the unimplemented codes. | |
sys/fs/fuse/fuse_vfsops.c | ||
412 |
sys/fs/fuse/fuse_device.c | ||
---|---|---|
412 | There is no dtrace probe in SET_ERRORX(). It provides ktrace tracing only, at least ATM. Removal of the SDT probes should be a separate commit. It might be that Gleb should just not bother to de-dup the string literals, instead copy-pasting them from SDT to SET_ERROR() for this change. |
sys/fs/fuse/fuse_device.c | ||
---|---|---|
412 | What about fbt:kernel:ktrexterr:entry ? |
sys/fs/fuse/fuse_device.c | ||
---|---|---|
412 | I do not object, but it should be made by somebody else, not me. Feel free to add it. |
I'm going to submit another diff that removes trace probes after finishing with exterrorizing.
sys/fs/fuse/fuse_device.c | ||
---|---|---|
369 | Yes, but I don't understand the code well enough to discern between these 3 ENODEV returns. Maybe @asomers can help to reformulate these exterrors? On the other hand, this is still more descriptive than just returning ENODEV without any exterror. |
sys/fs/fuse/fuse_device.c | ||
---|---|---|
198 | and remove the next line. |
sys/fs/fuse/fuse_vfsops.c | ||
---|---|---|
592–595 | It looks to me like you rebased your code? There should be another EXTERROR here. |
sys/fs/fuse/fuse_vfsops.c | ||
---|---|---|
592–595 |
Yes, I did that to make use of EXTERROR.
Should I deduplicate the error string via static char array here? |
sys/fs/fuse/fuse_vfsops.c | ||
---|---|---|
592–595 |
Yes please. Unlike the dtrace probe, the fuse_warn will not be going away. |
This change is ok with me. But I would still like to see a man page and some usage examples from userland.
What man page? The information on how to read exterrors is located in kdumps manpage.
Where? That man page hasn't been changed since 2022. And uexterr_gettext and exterrctl need man pages.
Ah, sorry, it is in ktrace man page: 96f4be881e8e9e0cb9a6ad2cd9f17f4440983600
And uexterr_gettext and exterrctl need man pages.
Yes, but this is orthogonal to this diff, I think?
We did not finished with stabilizing the kernel interface yet, and not much started looking at the proper userspace facilities. It might end up built-in into err()/warn() so uexterr_gettext() would be left as some internal function. I simply did not considered that we are there yet.
For the exterrctl(2), the man page would be eventually added, but with the warning similar to that for thr*(2) and sigfastblock(2) that the interface is for C runtime and should not be used by the app code.
So is it intended that kdump/ktrace an err/warn will be the primary way to view these messages?
Probably warn/err when I enable them. Ktrace is indeed already there, but it cannot be the primary user interface.
I do not want to do the more intrusive pass on userspace to avoid the needs to iterate, as I have to do for vm/vm_mmap.c in kernel.
sys/fs/fuse/fuse_device.c | ||
---|---|---|
600 | Was the change from EINVAL intentional? It is causing one of the fusefs tests to fail. |
sys/fs/fuse/fuse_device.c | ||
---|---|---|
600 | No, it is a copy-paste mistake. I can push a fix if you approve. |
sys/fs/fuse/fuse_device.c | ||
---|---|---|
600 | You do not need approval. Just commit it. |