Re: [libvirt] [PATCH 2/4] virfiletst: Test virFileIsSharedFS

[Jiri Denemark]

On Tue, Oct 09, 2018 at 15:48:45 +0200, Michal Privoznik wrote:
> Introduce some basic test cases for virFileIsSharedFS(). More
> will be added later. In order to achieve desired result, mocks
> for setmntent() and statfs() need to be invented because the
> first thing that virFileIsSharedFS() does is calling the latter.
> If it finds a FUSE mount it'll call the former.
> 
> The mock might look a bit complicated, but in fact it's quite
> simple. The test sets LIBVIRT_MTAB env variable to hold the
> absolute path to a file containing mount table. Then, statfs()
> returns matching FS it finds, and setmntent() is there just to
> replace /proc/mounts with the file the test wants to load.
> 
> Adding this test also exposed a bug we have - because we assume
> the given path points to a file we cut off what we assume is a
> file name to obtain directory path and only then we call
> statfs(). This is buggy because the passed path could be already
> a mount point.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  tests/Makefile.am |   7 +-
>  tests/virfiledata/mounts3.txt |  33 
>  tests/virfilemock.c   | 154 ++
>  tests/virfiletest.c   |  62 +-
>  4 files changed, 254 insertions(+), 2 deletions(-)
>  create mode 100644 tests/virfiledata/mounts3.txt
>  create mode 100644 tests/virfilemock.c
...
> diff --git a/tests/virfilemock.c b/tests/virfilemock.c
> new file mode 100644
> index 00..d458616899
> --- /dev/null
> +++ b/tests/virfilemock.c
...
> +static int
> +statfs_mock(const char *mtab,
> +const char *path,
> +struct statfs *buf)
> +{
> +FILE *f;
> +struct mntent mb;
> +char mntbuf[1024];
> +int ret = -1;
> +
> +if (!(f = real_setmntent(mtab, "r"))) {
> +fprintf(stderr, "Unable to open %s", mtab);
> +abort();

I think just returning -1 would be better. The caller will take care of
reporting the error.

> +}
> +
> +while (getmntent_r(f, , mntbuf, sizeof(mntbuf))) {
> +int ftype;
> +
> +if (STRNEQ(mb.mnt_dir, path))
> +continue;

I was wondering how this can work as we call statfs on an arbitrary path
rather than on the exact mount point. But it works because
virFileIsSharedFSType calls statfs in a loop checking smaller part of
the path in each iteration. That said, the mocked function is not
generic, but it's good enough for our use case.

...

With the change suggested above
Reviewed-by: Jiri Denemark 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/4] virfiletst: Test virFileIsSharedFS

[Michal Privoznik]

Introduce some basic test cases for virFileIsSharedFS(). More
will be added later. In order to achieve desired result, mocks
for setmntent() and statfs() need to be invented because the
first thing that virFileIsSharedFS() does is calling the latter.
If it finds a FUSE mount it'll call the former.

The mock might look a bit complicated, but in fact it's quite
simple. The test sets LIBVIRT_MTAB env variable to hold the
absolute path to a file containing mount table. Then, statfs()
returns matching FS it finds, and setmntent() is there just to
replace /proc/mounts with the file the test wants to load.

Adding this test also exposed a bug we have - because we assume
the given path points to a file we cut off what we assume is a
file name to obtain directory path and only then we call
statfs(). This is buggy because the passed path could be already
a mount point.

Signed-off-by: Michal Privoznik 
---
 tests/Makefile.am |   7 +-
 tests/virfiledata/mounts3.txt |  33 
 tests/virfilemock.c   | 154 ++
 tests/virfiletest.c   |  62 +-
 4 files changed, 254 insertions(+), 2 deletions(-)
 create mode 100644 tests/virfiledata/mounts3.txt
 create mode 100644 tests/virfilemock.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 51be1509e1..d7ec7e3a6f 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -242,6 +242,7 @@ test_libraries += virusbmock.la \
virnetdevbandwidthmock.la \
virnumamock.la \
virtestmock.la \
+   virfilemock.la \
$(NULL)
 endif WITH_LINUX
 
@@ -1170,9 +1171,13 @@ virresctrltest_SOURCES = \
virresctrltest.c testutils.h testutils.c virfilewrapper.h 
virfilewrapper.c
 virresctrltest_LDADD = $(LDADDS)
 
+virfilemock_la_SOURCES = \
+   virfilemock.c
+virfilemock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+virfilemock_la_LIBADD = $(MOCKLIBS_LIBS)
 else ! WITH_LINUX
 EXTRA_DIST += vircaps2xmltest.c virnumamock.c virfilewrapper.c \
- virfilewrapper.h virresctrltest.c
+ virfilewrapper.h virresctrltest.c virfilemock.c
 endif ! WITH_LINUX
 
 if WITH_NSS
diff --git a/tests/virfiledata/mounts3.txt b/tests/virfiledata/mounts3.txt
new file mode 100644
index 00..226f67dc00
--- /dev/null
+++ b/tests/virfiledata/mounts3.txt
@@ -0,0 +1,33 @@
+/dev/root / xfs rw,noatime,attr2,inode64,noquota 0 0
+proc /proc proc rw,nosuid,nodev,noexec,relatime 0 0
+tmpfs /run tmpfs rw,nodev,relatime,size=3281436k,mode=755 0 0
+sysfs /sys sysfs rw,nosuid,nodev,noexec,relatime 0 0
+dev /dev devtmpfs rw,nosuid,relatime,size=10240k,nr_inodes=4093060,mode=755 0 0
+securityfs /sys/kernel/security securityfs rw,nosuid,nodev,noexec,relatime 0 0
+debugfs /sys/kernel/debug debugfs rw,nosuid,nodev,noexec,relatime 0 0
+mqueue /dev/mqueue mqueue rw,nosuid,nodev,noexec,relatime 0 0
+configfs /sys/kernel/config configfs rw,nosuid,nodev,noexec,relatime 0 0
+devpts /dev/pts devpts rw,nosuid,noexec,relatime,gid=5,mode=620,ptmxmode=000 0 0
+fusectl /sys/fs/fuse/connections fusectl rw,nosuid,nodev,noexec,relatime 0 0
+shm /dev/shm tmpfs rw,nosuid,nodev,noexec,relatime 0 0
+cgroup_root /sys/fs/cgroup tmpfs 
rw,nosuid,nodev,noexec,relatime,size=10240k,mode=755 0 0
+openrc /sys/fs/cgroup/openrc cgroup 
rw,nosuid,nodev,noexec,relatime,release_agent=/lib/rc/sh/cgroup-release-agent.sh,name=openrc
 0 0
+none /sys/fs/cgroup/unified cgroup2 rw,nosuid,nodev,noexec,relatime,nsdelegate 
0 0
+cpuset /sys/fs/cgroup/cpuset cgroup rw,nosuid,nodev,noexec,relatime,cpuset 0 0
+cpu /sys/fs/cgroup/cpu cgroup rw,nosuid,nodev,noexec,relatime,cpu 0 0
+cpuacct /sys/fs/cgroup/cpuacct cgroup rw,nosuid,nodev,noexec,relatime,cpuacct 
0 0
+blkio /sys/fs/cgroup/blkio cgroup rw,nosuid,nodev,noexec,relatime,blkio 0 0
+memory /sys/fs/cgroup/memory cgroup rw,nosuid,nodev,noexec,relatime,memory 0 0
+devices /sys/fs/cgroup/devices cgroup rw,nosuid,nodev,noexec,relatime,devices 
0 0
+freezer /sys/fs/cgroup/freezer cgroup rw,nosuid,nodev,noexec,relatime,freezer 
0 0
+net_cls /sys/fs/cgroup/net_cls cgroup rw,nosuid,nodev,noexec,relatime,net_cls 
0 0
+perf_event /sys/fs/cgroup/perf_event cgroup 
rw,nosuid,nodev,noexec,relatime,perf_event 0 0
+net_prio /sys/fs/cgroup/net_prio cgroup 
rw,nosuid,nodev,noexec,relatime,net_prio 0 0
+hugetlb /sys/fs/cgroup/hugetlb cgroup rw,nosuid,nodev,noexec,relatime,hugetlb 
0 0
+pids /sys/fs/cgroup/pids cgroup rw,nosuid,nodev,noexec,relatime,pids 0 0
+rdma /sys/fs/cgroup/rdma cgroup rw,nosuid,nodev,noexec,relatime,rdma 0 0
+binfmt_misc /proc/sys/fs/binfmt_misc binfmt_misc 
rw,nosuid,nodev,noexec,relatime 0 0
+hugetlbfs /hugepages2M hugetlbfs rw,relatime,mode=1777,pagesize=2M 0 0
+none /run/user/1000 tmpfs rw,relatime,mode=700,uid=1000 0 0
+host:/nfs /nfs nfs4 
rw,relatime,vers=4.1,rsize=1048576,wsize=1048576,namlen=255,hard,proto=tcp6,timeo=600,retrans=2,sec=sys,clientaddr=::,local_lock=none,addr=::
 0 0
+dev /nfs/blah devtmpfs