[V5,01/10] tst_device: Add tst_is_mounted() helper

This patch moves the ismount() helper added to the fsmount syscall tests
to the standard library and renames it to tst_is_mounted(). The helper
can be used across different files now.

Minor modifications are also done to the helper.

Acked-by: Li Wang <liwang@redhat.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/tst_device.h                          |  6 +++++
 lib/tst_device.c                              | 14 +++++++++++
 testcases/kernel/syscalls/fsmount/fsmount01.c | 25 +------------------
 3 files changed, 21 insertions(+), 24 deletions(-)

Hi!
> This patch moves the ismount() helper added to the fsmount syscall tests
> to the standard library and renames it to tst_is_mounted(). The helper
> can be used across different files now.
> 
> Minor modifications are also done to the helper.
> 
> Acked-by: Li Wang <liwang@redhat.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  include/tst_device.h                          |  6 +++++
>  lib/tst_device.c                              | 14 +++++++++++
>  testcases/kernel/syscalls/fsmount/fsmount01.c | 25 +------------------
>  3 files changed, 21 insertions(+), 24 deletions(-)
> 
> diff --git a/include/tst_device.h b/include/tst_device.h
> index f5609f5986bb..bd6910672d2f 100644
> --- a/include/tst_device.h
> +++ b/include/tst_device.h
> @@ -36,6 +36,12 @@ extern struct tst_device *tst_device;
>   */
>  int tst_umount(const char *path);
>  
> +/*
> + * Verifies if an earlier mount is successful or not.
> + * @path: Mount path to verify
> + */
> +int tst_is_mounted(const char *path);
> +
>  /*
>   * Clears a first few blocks of the device. This is needed when device has
>   * already been formatted with a filesystems, subset of mkfs.foo utils aborts
> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index 8b5459def1cb..8bf6dacf5973 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -386,6 +386,20 @@ int tst_umount(const char *path)
>  	return -1;
>  }
>  
> +int tst_is_mounted(const char *path)
> +{
> +	char cmd[PATH_MAX];
> +	int ret;
> +
> +	snprintf(cmd, sizeof(cmd), "mountpoint -q %s", path);
> +	ret = tst_system(cmd);

I'm not sure that depending on mountpoint command is right choice, there
are all kinds of embedded systems out there that may be missing it.

Also this does not even handle the case that the command is missing.

Looking at the v4 version, all we need is to correctly parse each line
from from /proc/mounts. I would just use strsep() with space as a
delimited and took first token that starts with a slash i.e. '/', then
we can just strcmp() it against the path. Or do I miss something?

> +	if (ret)
> +		tst_resm(TINFO, "No device is mounted at %s", path);
> +
> +	return !ret;
> +}
> +
>  int find_stat_file(const char *dev, char *path, size_t path_len)
>  {
>  	const char *devname = strrchr(dev, '/') + 1;
> diff --git a/testcases/kernel/syscalls/fsmount/fsmount01.c b/testcases/kernel/syscalls/fsmount/fsmount01.c
> index 83185b48aedd..8e29a1537334 100644
> --- a/testcases/kernel/syscalls/fsmount/fsmount01.c
> +++ b/testcases/kernel/syscalls/fsmount/fsmount01.c
> @@ -12,30 +12,10 @@
>  #include "tst_test.h"
>  #include "lapi/fcntl.h"
>  #include "lapi/fsmount.h"
> -#include "tst_safe_stdio.h"
>  
> -#define LINELENGTH 256
>  #define MNTPOINT "newmount_point"
>  static int sfd, mfd, is_mounted;
>  
> -static int ismount(char *mntpoint)
> -{
> -	int ret = 0;
> -	FILE *file;
> -	char line[LINELENGTH];
> -
> -	file = SAFE_FOPEN("/proc/mounts", "r");
> -
> -	while (fgets(line, sizeof(line), file)) {
> -		if (strstr(line, mntpoint) != NULL) {
> -			ret = 1;
> -			break;
> -		}
> -	}
> -	SAFE_FCLOSE(file);
> -	return ret;
> -}
> -
>  static void cleanup(void)
>  {
>  	if (is_mounted)
> @@ -76,12 +56,9 @@ static void test_fsmount(void)
>  	tst_res(TPASS, "move_mount() attached to the mount point");
>  	SAFE_CLOSE(mfd);
>  
> -	if (ismount(MNTPOINT)) {
> -		tst_res(TPASS, "device mounted");
> +	if (tst_is_mounted(MNTPOINT)) {
>  		SAFE_UMOUNT(MNTPOINT);
>  		is_mounted = 0;
> -	} else {
> -		tst_res(TFAIL, "device not mounted");
>  	}
>  }
>  
> -- 
> 2.21.0.rc0.269.g1a574e7a288b
>

On Fri, Mar 6, 2020 at 8:45 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> ...
> >
> > +int tst_is_mounted(const char *path)
> > +{
> > +     char cmd[PATH_MAX];
> > +     int ret;
> > +
> > +     snprintf(cmd, sizeof(cmd), "mountpoint -q %s", path);
> > +     ret = tst_system(cmd);
>
> I'm not sure that depending on mountpoint command is right choice, there
> are all kinds of embedded systems out there that may be missing it.


Good point, we'd better avoid involving other packages as the dependence of
LTP.


> Also this does not even handle the case that the command is missing.
>
> Looking at the v4 version, all we need is to correctly parse each line
> from from /proc/mounts. I would just use strsep() with space as a
> delimited and took first token that starts with a slash i.e. '/', then
> we can just strcmp() it against the path. Or do I miss something?
>

I'm afraid strcmp() can not satisfy the requirement for us. As you know LTP
creates the MNTPOINT in temp dir that means it could not accurately match
the string path which extracts from /proc/mounts with a slash.

e.g
#define MNTPOINT "fallocate"
...
/dev/loop4 on /tmp/FPp7kh/fallocate type xfs
(rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota)
...
strcmp("/tmp/FPp7kh/fallocate", MNTPOINT) will never ruturn 0 to us.

What I can think of is to use strrchr() to cut the string after last '/',
but that can only work for test mount fs in LTP ways. Other situations
might not satisfy.

On 07-03-20, 20:42, Li Wang wrote:
> On Fri, Mar 6, 2020 at 8:45 PM Cyril Hrubis <chrubis@suse.cz> wrote:
> 
> > ...
> > >
> > > +int tst_is_mounted(const char *path)
> > > +{
> > > +     char cmd[PATH_MAX];
> > > +     int ret;
> > > +
> > > +     snprintf(cmd, sizeof(cmd), "mountpoint -q %s", path);
> > > +     ret = tst_system(cmd);
> >
> > I'm not sure that depending on mountpoint command is right choice, there
> > are all kinds of embedded systems out there that may be missing it.
> 
> 
> Good point, we'd better avoid involving other packages as the dependence of
> LTP.
> 
> 
> > Also this does not even handle the case that the command is missing.
> >
> > Looking at the v4 version, all we need is to correctly parse each line
> > from from /proc/mounts. I would just use strsep() with space as a
> > delimited and took first token that starts with a slash i.e. '/', then
> > we can just strcmp() it against the path. Or do I miss something?
> >
> 
> I'm afraid strcmp() can not satisfy the requirement for us. As you know LTP
> creates the MNTPOINT in temp dir that means it could not accurately match
> the string path which extracts from /proc/mounts with a slash.
> 
> e.g
> #define MNTPOINT "fallocate"
> ...
> /dev/loop4 on /tmp/FPp7kh/fallocate type xfs
> (rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota)
> ...
> strcmp("/tmp/FPp7kh/fallocate", MNTPOINT) will never ruturn 0 to us.
> 
> What I can think of is to use strrchr() to cut the string after last '/',
> but that can only work for test mount fs in LTP ways. Other situations
> might not satisfy.

@Cyril, can we please finalize what you guys want me to do here ? I
don't really want to repost the patch, which still has issues :)

Hi!
> > I'm afraid strcmp() can not satisfy the requirement for us. As you know LTP
> > creates the MNTPOINT in temp dir that means it could not accurately match
> > the string path which extracts from /proc/mounts with a slash.
> > 
> > e.g
> > #define MNTPOINT "fallocate"
> > ...
> > /dev/loop4 on /tmp/FPp7kh/fallocate type xfs
> > (rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota)
> > ...
> > strcmp("/tmp/FPp7kh/fallocate", MNTPOINT) will never ruturn 0 to us.
> > 
> > What I can think of is to use strrchr() to cut the string after last '/',
> > but that can only work for test mount fs in LTP ways. Other situations
> > might not satisfy.
> 
> @Cyril, can we please finalize what you guys want me to do here ? I
> don't really want to repost the patch, which still has issues :)

Sorry for the back and forth.

I remmebered yesterday that there is setmntent() in libc, so i we call
that on /proc/mounts, the whole problem would be reduced to a path
comparsion.

Hi!
> > Also this does not even handle the case that the command is missing.
> >
> > Looking at the v4 version, all we need is to correctly parse each line
> > from from /proc/mounts. I would just use strsep() with space as a
> > delimited and took first token that starts with a slash i.e. '/', then
> > we can just strcmp() it against the path. Or do I miss something?
> >
> 
> I'm afraid strcmp() can not satisfy the requirement for us. As you know LTP
> creates the MNTPOINT in temp dir that means it could not accurately match
> the string path which extracts from /proc/mounts with a slash.
> 
> e.g
> #define MNTPOINT "fallocate"
> ...
> /dev/loop4 on /tmp/FPp7kh/fallocate type xfs
> (rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota)
> ...
> strcmp("/tmp/FPp7kh/fallocate", MNTPOINT) will never ruturn 0 to us.
> 
> What I can think of is to use strrchr() to cut the string after last '/',
> but that can only work for test mount fs in LTP ways. Other situations
> might not satisfy.

Hmm, for that we have to have compose the path for the comparsion
anyways, since unless we pass an absoule path we can never be user if we
have a right match or not. There may be leftover mount points from a
failed tests that have faile to cleanup properly as well.

So I guess that we need one more function, with tmpdir in name, that
would compose the right path for us and then call the tst_is_mntpoint().

I would have called that:

int tst_is_mntpoint_at_tmpdir(const char *path);

On Wed, Mar 11, 2020 at 6:26 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > > Also this does not even handle the case that the command is missing.
> > >
> > > Looking at the v4 version, all we need is to correctly parse each line
> > > from from /proc/mounts. I would just use strsep() with space as a
> > > delimited and took first token that starts with a slash i.e. '/', then
> > > we can just strcmp() it against the path. Or do I miss something?
> > >
> >
> > I'm afraid strcmp() can not satisfy the requirement for us. As you know
> LTP
> > creates the MNTPOINT in temp dir that means it could not accurately match
> > the string path which extracts from /proc/mounts with a slash.
> >
> > e.g
> > #define MNTPOINT "fallocate"
> > ...
> > /dev/loop4 on /tmp/FPp7kh/fallocate type xfs
> > (rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota)
> > ...
> > strcmp("/tmp/FPp7kh/fallocate", MNTPOINT) will never ruturn 0 to us.
> >
> > What I can think of is to use strrchr() to cut the string after last '/',
> > but that can only work for test mount fs in LTP ways. Other situations
> > might not satisfy.
>
> Hmm, for that we have to have compose the path for the comparsion
> anyways, since unless we pass an absoule path we can never be user if we
> have a right match or not. There may be leftover mount points from a
> failed tests that have faile to cleanup properly as well.
>
> So I guess that we need one more function, with tmpdir in name, that
> would compose the right path for us and then call the tst_is_mntpoint().
>

+1 it makes sense to me.


>
> I would have called that:
>
> int tst_is_mntpoint_at_tmpdir(const char *path);
>

Hmm, the return value shouldn't the full right path, why return int here?
or I misunderstand here?

On Wed, Mar 11, 2020 at 8:45 PM Li Wang <liwang@redhat.com> wrote:

> ...
>>
>> int tst_is_mntpoint_at_tmpdir(const char *path);
>>
>
> Hmm, the return value shouldn't the full right path, why return int here?
> or I misunderstand here?
>
>
s/shouldn't/should

On 11-03-20, 11:26, Cyril Hrubis wrote:
> Hmm, for that we have to have compose the path for the comparsion
> anyways, since unless we pass an absoule path we can never be user if we
> have a right match or not. There may be leftover mount points from a
> failed tests that have faile to cleanup properly as well.
> 
> So I guess that we need one more function, with tmpdir in name, that
> would compose the right path for us and then call the tst_is_mntpoint().
> 
> I would have called that:
> 
> int tst_is_mntpoint_at_tmpdir(const char *path);

Is everyone fine with this code now :)

int tst_is_mounted(const char *path)
{
        char line[PATH_MAX];
        FILE *file;
        int ret = 0;

        file = SAFE_FOPEN(NULL, "/proc/mounts", "r");

        while (fgets(line, sizeof(line), file)) {
                if (strstr(line, path) != NULL) {
                        ret = 1;
                        break;
                }
        }

        SAFE_FCLOSE(NULL, file);

        if (!ret)
                tst_resm(TINFO, "No device is mounted at %s", path);

        return ret;
}

int tst_is_mounted_at_tmpdir(const char *path)
{
        char cdir[PATH_MAX], mpath[PATH_MAX];
        int ret;

        if (!getcwd(cdir, PATH_MAX))
                return 0;

        ret = snprintf(mpath, PATH_MAX, "%s/%s", cdir, path);
        if (ret < 0 || ret >= PATH_MAX)
                return 0;

        return tst_is_mounted(mpath);
}

Hi Viresh,

> Is everyone fine with this code now :)

> int tst_is_mounted(const char *path)
> {
>         char line[PATH_MAX];
>         FILE *file;
>         int ret = 0;

>         file = SAFE_FOPEN(NULL, "/proc/mounts", "r");

>         while (fgets(line, sizeof(line), file)) {
>                 if (strstr(line, path) != NULL) {
>                         ret = 1;
>                         break;
>                 }
>         }

>         SAFE_FCLOSE(NULL, file);

>         if (!ret)
>                 tst_resm(TINFO, "No device is mounted at %s", path);

>         return ret;
> }

> int tst_is_mounted_at_tmpdir(const char *path)
> {
>         char cdir[PATH_MAX], mpath[PATH_MAX];
>         int ret;

>         if (!getcwd(cdir, PATH_MAX))
>                 return 0;
LGTM. I guess we can ignore this, but maybe tst_res(TWARN | TERRNO, "..."),
could be added here. But maybe it's not important.

>         ret = snprintf(mpath, PATH_MAX, "%s/%s", cdir, path);
>         if (ret < 0 || ret >= PATH_MAX)
>                 return 0;

>         return tst_is_mounted(mpath);
> }

Kind regards,
Petr