Page MenuHomeFreeBSD

fusefs: add a test suite
AbandonedPublic

Authored by asomers on Mar 29 2019, 4:07 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 24 2024, 10:21 AM
Unknown Object (File)
Feb 24 2024, 3:47 AM
Unknown Object (File)
Dec 20 2023, 4:15 AM
Unknown Object (File)
Dec 14 2023, 6:29 PM
Unknown Object (File)
Nov 15 2023, 7:21 AM
Unknown Object (File)
Sep 25 2023, 4:10 PM
Unknown Object (File)
Sep 25 2023, 6:10 AM
Unknown Object (File)
Aug 23 2023, 11:13 PM
Subscribers

Details

Reviewers
cem
ngie
Summary

fusefs: add a test suite

This commit adds a test suite covering the FUSE kernel API. The kernel API
is a de-facto standard that describes communication between the kernel and
a file system server. Typically, the server's side of the protocol is
implemented by libfuse. To my knowledge, this is the first automated test
suite of a general FUSE client.

This commit is a merge from the projects/fuse2 branch, using the following
commands:
svn copy -r 345356 ^/projects/fuse2/tests/sys/fs/fusefs@345356 tests/sys/fs/fusefs
svn merge -r 345356:345386 -c 345563,345567 -r 345622:345623 -c 345690 ^/projects/fuse2
svn merge -c 344786 ^/projects/fuse2/sys sys

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 23384

Event Timeline

Remove svn:mergeinfo from sys, and attempt to get new files showing up in
phabricator.

Another attempt to make Phabricator show newly added files

Apparently arcanist doesn't understand newly added files within a newly added directory. But Phabricator handles them just fine if you create the diff with svn -x "-U 9999" diff --show-copies-as-adds.

ngie requested changes to this revision.Mar 29 2019, 6:26 PM

Some general comments; I'll look more in-depth later.

Also: can this review please be broken down in terms of the following parts?

  1. General infrastructure (make, supporting code).
  2. Individual tests.

This would make the change easier to review and merge into FreeBSD, bits at a time (for this reason I'm requesting changes).

Thanks so much :)!

tests/sys/fs/fusefs/Makefile
62–95

This is a lot of duplication. I think it makes sense to either append these values in a for-loop, or add them to SRCS (the former method would be more straightforward).

196–198

This could be simplified (I'm also using CXXFLAGS because these are C++ tests):

FUSEFS=    ${SRCTOP}/sys/fs/fuse
MOUNT=     ${SRCTOP}/sbin/mount

CXXFLAGS+= -I${SRCTOP}/tests
CXXFLAGS+= -I${MOUNT}
CXXFLAGS+= -I${FUSEFS}
tests/sys/fs/fusefs/allow_other.cc
57–60

FYI: there won't be a Kyua API for this :/. This should be set as a property on the testcase (when it can be made granular enough).

139

Why isn't this being handled via a TearDown(..) method in the class?

I'm not seeing actual cleanup here though -- only standard test logic.

152
  1. Why isn't the return code from wait being tested?
  2. Why isn't the PID being waited for, and instead it's waiting for the first child to complete?
198

Why?

This revision now requires changes to proceed.Mar 29 2019, 6:26 PM

Sadly, Phabricator is unwilling to display newly added files within a newly added directory.

In D19752#423384, @ngie wrote:

Some general comments; I'll look more in-depth later.

Also: can this review please be broken down in terms of the following parts?

  1. General infrastructure (make, supporting code).
  2. Individual tests.

This would make the change easier to review and merge into FreeBSD, bits at a time (for this reason I'm requesting changes).

Thanks so much :)!

It's not easy to break the review down without making the merging process more difficult. One alternative would be to review the files as they currently exist in the project branch. I could commit as I go, and manually create differential revisions. Once you're satisfied, then I'll merge to head with no additional review. Would that work better?

sys/fs/fuse/fuse_kernel.h
197

This was erroneously deleted in r239925 . Even though FUSE_MKNOD support was removed, the definition should've remained because it's part of the FUSE protocol.

tests/sys/fs/fusefs/Makefile
62–95

I can do the for-loop. For some reason just adding them to SRCS doesn't work (Make can't find getmntopts.c when I tried it that way).

tests/sys/fs/fusefs/allow_other.cc
57–60

What's the API for fetching a property?

139

You're too accustomed to ATF. Remember, in GoogleTest TearDown happens in the same process as the test body, immediately after the body. Cleanup happens in the parent simply because the parent returns unlike the child, which never returns.

152
  1. Good point
  2. Because there's only one child.
198

I deliberately leak file descriptors at the end of many tests so that the test logic doesn't get cluttered up with a bunch of FUSE_FLUSH and FUSE_RELEASE operations.

Ping. Would you prefer for me to close this review and open new ones, one or two files at a time? Or would some kind of post-commit review work better?

Ping. Would you prefer for me to close this review and open new ones, one or two files at a time? Or would some kind of post-commit review work better?

I'll start looking through the diff some more. No need to complicate things.

That being said, I think the tests and the functional changes to the code should be separated, like the fuse_mknod_in, etc changes to fuse_kernel.h and its corresponding support.

Submitted as r350665, along with many other changes.