diff mbox series

SAFE_MOUNT: print debug info about mounting operation

Message ID 20230604095117.3543342-1-jencce.kernel@gmail.com
State Superseded
Headers show
Series SAFE_MOUNT: print debug info about mounting operation | expand

Commit Message

Murphy Zhou June 4, 2023, 9:51 a.m. UTC 
Signed-off-by: Murphy Zhou <jencce.kernel@gmail.com>
---
 lib/safe_macros.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Li Wang June 5, 2023, 6:30 a.m. UTC | #1 
Hi Murphy,

This is helpful for knowing the mount info.

But as SAFE_MOUNT is not only used for relative path but
mount absolute path, so we'd better determine what the '*target'
path type.

How about changing to below:

       if (target[0] == '/') {
               strncpy(mpath, target, PATH_MAX-1);
               mpath[PATH_MAX-1] = '\0';
       } else {
               .... your relative path code ...
       }

       tst_resm_(file, lineno, TINFO,
               "Mounting %s to %s fstyp=%s flags=%x",
               source, mpath, filesystemtype, mountflags);


On Sun, Jun 4, 2023 at 5:51 PM Murphy Zhou <jencce.kernel@gmail.com> wrote:

> Signed-off-by: Murphy Zhou <jencce.kernel@gmail.com>
> ---
>  lib/safe_macros.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/lib/safe_macros.c b/lib/safe_macros.c
> index af6dd0716..a66285a0e 100644
> --- a/lib/safe_macros.c
> +++ b/lib/safe_macros.c
> @@ -898,7 +898,24 @@ int safe_mount(const char *file, const int lineno,
> void (*cleanup_fn)(void),
>                const void *data)
>  {
>         int rval = -1;
> +       char cdir[PATH_MAX], mpath[PATH_MAX];
>
> +       if (!getcwd(cdir, PATH_MAX)) {
> +               tst_resm(TWARN | TERRNO, "Failed to find current
> directory");
> +               return 0;
> +       }
> +
> +       rval = snprintf(mpath, PATH_MAX, "%s/%s", cdir, target);
> +       if (rval < 0 || rval >= PATH_MAX) {
> +               tst_resm(TWARN | TERRNO,
> +                        "snprintf() should have returned %d instead of
> %d",
> +                        PATH_MAX, rval);
> +               return 0;
> +       }
> +

+       tst_resm_(file, lineno, TINFO,
> +               "Mounting %s to %s fstyp=%s flags=%x",
> +               source, mpath, filesystemtype, mountflags);
>         /*
>          * Don't try using the kernel's NTFS driver when mounting NTFS,
> since
>          * the kernel's NTFS driver doesn't have proper write support.
> --
> 2.31.1
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>
Martin Doucha June 5, 2023, 8:39 a.m. UTC | #2 
Hi,

On 04. 06. 23 11:51, Murphy Zhou wrote:
> Signed-off-by: Murphy Zhou <jencce.kernel@gmail.com>
> ---
>   lib/safe_macros.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/lib/safe_macros.c b/lib/safe_macros.c
> index af6dd0716..a66285a0e 100644
> --- a/lib/safe_macros.c
> +++ b/lib/safe_macros.c
> @@ -898,7 +898,24 @@ int safe_mount(const char *file, const int lineno, void (*cleanup_fn)(void),
>   	       const void *data)
>   {
>   	int rval = -1;
> +	char cdir[PATH_MAX], mpath[PATH_MAX];
>   
> +	if (!getcwd(cdir, PATH_MAX)) {
> +		tst_resm(TWARN | TERRNO, "Failed to find current directory");
> +		return 0;

You return success when nothing was mounted. That is clearly wrong. 
Either call tst_brkm_(... TBROK ...) if the failure is so serious that 
the test cannot continue, or don't return at all.

> +	}
> +
> +	rval = snprintf(mpath, PATH_MAX, "%s/%s", cdir, target);

The C library has a function for resolving paths: realpath(). Please use 
that.

> +	if (rval < 0 || rval >= PATH_MAX) {
> +		tst_resm(TWARN | TERRNO,
> +			 "snprintf() should have returned %d instead of %d",
> +			 PATH_MAX, rval);
> +		return 0;

Returning here is also wrong.

> +	}
> +
> +	tst_resm_(file, lineno, TINFO,
> +		"Mounting %s to %s fstyp=%s flags=%x",
> +		source, mpath, filesystemtype, mountflags);
>   	/*
>   	 * Don't try using the kernel's NTFS driver when mounting NTFS, since
>   	 * the kernel's NTFS driver doesn't have proper write support.
Li Wang June 5, 2023, 9:56 a.m. UTC | #3 
On Mon, Jun 5, 2023 at 4:40 PM Martin Doucha <mdoucha@suse.cz> wrote:

> Hi,
>
> On 04. 06. 23 11:51, Murphy Zhou wrote:
> > Signed-off-by: Murphy Zhou <jencce.kernel@gmail.com>
> > ---
> >   lib/safe_macros.c | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> >
> > diff --git a/lib/safe_macros.c b/lib/safe_macros.c
> > index af6dd0716..a66285a0e 100644
> > --- a/lib/safe_macros.c
> > +++ b/lib/safe_macros.c
> > @@ -898,7 +898,24 @@ int safe_mount(const char *file, const int lineno,
> void (*cleanup_fn)(void),
> >              const void *data)
> >   {
> >       int rval = -1;
> > +     char cdir[PATH_MAX], mpath[PATH_MAX];
> >
> > +     if (!getcwd(cdir, PATH_MAX)) {
> > +             tst_resm(TWARN | TERRNO, "Failed to find current
> directory");
> > +             return 0;
>
> You return success when nothing was mounted. That is clearly wrong.
> Either call tst_brkm_(... TBROK ...) if the failure is so serious that
> the test cannot continue, or don't return at all.
>
> > +     }
> > +
> > +     rval = snprintf(mpath, PATH_MAX, "%s/%s", cdir, target);
>
> The C library has a function for resolving paths: realpath(). Please use
> that.
>


+1, good point. realpath() is more convenient, we don't
need to additionally copy string to the absolute path.

@Murphy, ignore my previous comments plz.



>
> > +     if (rval < 0 || rval >= PATH_MAX) {
> > +             tst_resm(TWARN | TERRNO,
> > +                      "snprintf() should have returned %d instead of
> %d",
> > +                      PATH_MAX, rval);
> > +             return 0;
>
> Returning here is also wrong.
>
> > +     }
> > +
> > +     tst_resm_(file, lineno, TINFO,
> > +             "Mounting %s to %s fstyp=%s flags=%x",
> > +             source, mpath, filesystemtype, mountflags);
> >       /*
> >        * Don't try using the kernel's NTFS driver when mounting NTFS,
> since
> >        * the kernel's NTFS driver doesn't have proper write support.
>
> --
> Martin Doucha   mdoucha@suse.cz
> SW Quality Engineer
> SUSE LINUX, s.r.o.
> CORSO IIa
> Krizikova 148/34
> 186 00 Prague 8
> Czech Republic
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>
Murphy Zhou June 6, 2023, 7:08 a.m. UTC | #4 
On Mon, Jun 5, 2023 at 5:57 PM Li Wang <liwang@redhat.com> wrote:
>
>
>
> On Mon, Jun 5, 2023 at 4:40 PM Martin Doucha <mdoucha@suse.cz> wrote:
>>
>> Hi,
>>
>> On 04. 06. 23 11:51, Murphy Zhou wrote:
>> > Signed-off-by: Murphy Zhou <jencce.kernel@gmail.com>
>> > ---
>> >   lib/safe_macros.c | 17 +++++++++++++++++
>> >   1 file changed, 17 insertions(+)
>> >
>> > diff --git a/lib/safe_macros.c b/lib/safe_macros.c
>> > index af6dd0716..a66285a0e 100644
>> > --- a/lib/safe_macros.c
>> > +++ b/lib/safe_macros.c
>> > @@ -898,7 +898,24 @@ int safe_mount(const char *file, const int lineno, void (*cleanup_fn)(void),
>> >              const void *data)
>> >   {
>> >       int rval = -1;
>> > +     char cdir[PATH_MAX], mpath[PATH_MAX];
>> >
>> > +     if (!getcwd(cdir, PATH_MAX)) {
>> > +             tst_resm(TWARN | TERRNO, "Failed to find current directory");
>> > +             return 0;
>>
>> You return success when nothing was mounted. That is clearly wrong.
>> Either call tst_brkm_(... TBROK ...) if the failure is so serious that
>> the test cannot continue, or don't return at all.
>>
>> > +     }
>> > +
>> > +     rval = snprintf(mpath, PATH_MAX, "%s/%s", cdir, target);
>>
>> The C library has a function for resolving paths: realpath(). Please use
>> that.
>
>
>
> +1, good point. realpath() is more convenient, we don't
> need to additionally copy string to the absolute path.
>
> @Murphy, ignore my previous comments plz.

All very good catches! Thanks for reviewing!

Cooking V2.

Murphy
>
>
>>
>>
>> > +     if (rval < 0 || rval >= PATH_MAX) {
>> > +             tst_resm(TWARN | TERRNO,
>> > +                      "snprintf() should have returned %d instead of %d",
>> > +                      PATH_MAX, rval);
>> > +             return 0;
>>
>> Returning here is also wrong.
>>
>> > +     }
>> > +
>> > +     tst_resm_(file, lineno, TINFO,
>> > +             "Mounting %s to %s fstyp=%s flags=%x",
>> > +             source, mpath, filesystemtype, mountflags);
>> >       /*
>> >        * Don't try using the kernel's NTFS driver when mounting NTFS, since
>> >        * the kernel's NTFS driver doesn't have proper write support.
>>
>> --
>> Martin Doucha   mdoucha@suse.cz
>> SW Quality Engineer
>> SUSE LINUX, s.r.o.
>> CORSO IIa
>> Krizikova 148/34
>> 186 00 Prague 8
>> Czech Republic
>>
>>
>> --
>> Mailing list info: https://lists.linux.it/listinfo/ltp
>>
>
>
> --
> Regards,
> Li Wang
diff mbox series

Patch

diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index af6dd0716..a66285a0e 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -898,7 +898,24 @@  int safe_mount(const char *file, const int lineno, void (*cleanup_fn)(void),
 	       const void *data)
 {
 	int rval = -1;
+	char cdir[PATH_MAX], mpath[PATH_MAX];
 
+	if (!getcwd(cdir, PATH_MAX)) {
+		tst_resm(TWARN | TERRNO, "Failed to find current directory");
+		return 0;
+	}
+
+	rval = snprintf(mpath, PATH_MAX, "%s/%s", cdir, target);
+	if (rval < 0 || rval >= PATH_MAX) {
+		tst_resm(TWARN | TERRNO,
+			 "snprintf() should have returned %d instead of %d",
+			 PATH_MAX, rval);
+		return 0;
+	}
+
+	tst_resm_(file, lineno, TINFO,
+		"Mounting %s to %s fstyp=%s flags=%x",
+		source, mpath, filesystemtype, mountflags);
 	/*
 	 * Don't try using the kernel's NTFS driver when mounting NTFS, since
 	 * the kernel's NTFS driver doesn't have proper write support.