* [LTP] [PATCH] tst_find_backing_dev: fix logic in matching mount point
@ 2022-07-08 7:33 Jan Stancek
2022-07-11 6:52 ` Li Wang
2022-07-11 12:05 ` Richard Palethorpe
0 siblings, 2 replies; 7+ messages in thread
From: Jan Stancek @ 2022-07-08 7:33 UTC (permalink / raw)
To: ltp
If backing dev is btrfs root device, then starting best_match_len
from 1 creates an issue, because root (/) is never matched.
Also we should check that entire mount point string is present in
path we are matching against.
In case there's error also dump /proc/self/mountinfo before tst_brk.
This fixes test with following partition layout (TMPDIR is on /):
# cat /proc/self/mountinfo | grep btrfs
59 1 0:29 /root / rw,relatime shared:1 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=257,subvol=/root
93 59 0:29 /home /home rw,relatime shared:47 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=256,subvol=/home
Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
lib/tst_device.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/lib/tst_device.c b/lib/tst_device.c
index c34cbe6d1f56..414bf0eea816 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -506,12 +506,17 @@ unsigned long tst_dev_bytes_written(const char *dev)
return dev_bytes_written;
}
-static int count_match_len(const char *first, const char *second)
+static int str_starts_with(const char *str, const char *prefix)
{
int len = 0;
- while (*first && *first++ == *second++)
+ while (*prefix) {
+ if (!*str)
+ return 0;
+ if (*str++ != *prefix++)
+ return 0;
len++;
+ }
return len;
}
@@ -524,7 +529,7 @@ void tst_find_backing_dev(const char *path, char *dev)
char *pre = NULL;
char *next = NULL;
unsigned int dev_major, dev_minor, line_mjr, line_mnr;
- unsigned int len, best_match_len = 1;
+ unsigned int len, best_match_len = 0;
char mnt_point[PATH_MAX];
if (stat(path, &buf) < 0)
@@ -550,7 +555,7 @@ void tst_find_backing_dev(const char *path, char *dev)
break;
}
- len = count_match_len(path, mnt_point);
+ len = str_starts_with(path, mnt_point);
if (len > best_match_len) {
strcpy(dev, pre);
best_match_len = len;
@@ -559,8 +564,10 @@ void tst_find_backing_dev(const char *path, char *dev)
SAFE_FCLOSE(NULL, file);
- if (!*dev)
+ if (!*dev) {
+ tst_system("cat /proc/self/mountinfo");
tst_brkm(TBROK, NULL, "Cannot find block device for %s", path);
+ }
if (stat(dev, &buf) < 0)
tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);
--
2.27.0
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [LTP] [PATCH] tst_find_backing_dev: fix logic in matching mount point
2022-07-08 7:33 [LTP] [PATCH] tst_find_backing_dev: fix logic in matching mount point Jan Stancek
@ 2022-07-11 6:52 ` Li Wang
2022-07-11 12:05 ` Richard Palethorpe
1 sibling, 0 replies; 7+ messages in thread
From: Li Wang @ 2022-07-11 6:52 UTC (permalink / raw)
To: Jan Stancek; +Cc: LTP List
[-- Attachment #1.1: Type: text/plain, Size: 906 bytes --]
On Fri, Jul 8, 2022 at 3:33 PM Jan Stancek <jstancek@redhat.com> wrote:
> If backing dev is btrfs root device, then starting best_match_len
> from 1 creates an issue, because root (/) is never matched.
> Also we should check that entire mount point string is present in
> path we are matching against.
>
> In case there's error also dump /proc/self/mountinfo before tst_brk.
>
> This fixes test with following partition layout (TMPDIR is on /):
> # cat /proc/self/mountinfo | grep btrfs
> 59 1 0:29 /root / rw,relatime shared:1 - btrfs /dev/dasda2
> rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=257,subvol=/root
> 93 59 0:29 /home /home rw,relatime shared:47 - btrfs /dev/dasda2
> rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=256,subvol=/home
>
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
>
Make sense!
Reviewed-by: Li Wang <liwang@redhat.com>
--
Regards,
Li Wang
[-- Attachment #1.2: Type: text/html, Size: 1780 bytes --]
[-- Attachment #2: Type: text/plain, Size: 60 bytes --]
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [LTP] [PATCH] tst_find_backing_dev: fix logic in matching mount point
2022-07-08 7:33 [LTP] [PATCH] tst_find_backing_dev: fix logic in matching mount point Jan Stancek
2022-07-11 6:52 ` Li Wang
@ 2022-07-11 12:05 ` Richard Palethorpe
2022-07-11 13:04 ` Jan Stancek
1 sibling, 1 reply; 7+ messages in thread
From: Richard Palethorpe @ 2022-07-11 12:05 UTC (permalink / raw)
To: Jan Stancek; +Cc: ltp
Hello,
Jan Stancek <jstancek@redhat.com> writes:
> If backing dev is btrfs root device, then starting best_match_len
> from 1 creates an issue, because root (/) is never matched.
> Also we should check that entire mount point string is present in
> path we are matching against.
>
> In case there's error also dump /proc/self/mountinfo before tst_brk.
>
> This fixes test with following partition layout (TMPDIR is on /):
> # cat /proc/self/mountinfo | grep btrfs
> 59 1 0:29 /root / rw,relatime shared:1 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=257,subvol=/root
> 93 59 0:29 /home /home rw,relatime shared:47 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=256,subvol=/home
>
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
> lib/tst_device.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index c34cbe6d1f56..414bf0eea816 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -506,12 +506,17 @@ unsigned long tst_dev_bytes_written(const char *dev)
> return dev_bytes_written;
> }
>
> -static int count_match_len(const char *first, const char *second)
> +static int str_starts_with(const char *str, const char *prefix)
> {
> int len = 0;
>
> - while (*first && *first++ == *second++)
> + while (*prefix) {
> + if (!*str)
> + return 0;
> + if (*str++ != *prefix++)
> + return 0;
> len++;
> + }
I'm not sure this is better than the original. It's a seperate cleanup
in any case.
>
> return len;
> }
> @@ -524,7 +529,7 @@ void tst_find_backing_dev(const char *path, char *dev)
> char *pre = NULL;
> char *next = NULL;
> unsigned int dev_major, dev_minor, line_mjr, line_mnr;
> - unsigned int len, best_match_len = 1;
> + unsigned int len, best_match_len = 0;
> char mnt_point[PATH_MAX];
>
> if (stat(path, &buf) < 0)
> @@ -550,7 +555,7 @@ void tst_find_backing_dev(const char *path, char *dev)
> break;
> }
>
> - len = count_match_len(path, mnt_point);
> + len = str_starts_with(path, mnt_point);
> if (len > best_match_len) {
> strcpy(dev, pre);
> best_match_len = len;
> @@ -559,8 +564,10 @@ void tst_find_backing_dev(const char *path, char *dev)
>
> SAFE_FCLOSE(NULL, file);
>
> - if (!*dev)
> + if (!*dev) {
> + tst_system("cat /proc/self/mountinfo");
> tst_brkm(TBROK, NULL, "Cannot find block device for %s", path);
> + }
>
> if (stat(dev, &buf) < 0)
> tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);
> --
> 2.27.0
Makes sense. However I suspect this function can be replaced with the
standard library method used in tst_stat_mount_dev. I didn't try this
before because I'm not sure why it scans mountinfo instead of mounts.
--
Thank you,
Richard.
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [LTP] [PATCH] tst_find_backing_dev: fix logic in matching mount point
2022-07-11 12:05 ` Richard Palethorpe
@ 2022-07-11 13:04 ` Jan Stancek
2022-07-11 13:11 ` Richard Palethorpe
0 siblings, 1 reply; 7+ messages in thread
From: Jan Stancek @ 2022-07-11 13:04 UTC (permalink / raw)
To: rpalethorpe; +Cc: LTP List
On Mon, Jul 11, 2022 at 2:13 PM Richard Palethorpe <rpalethorpe@suse.de> wrote:
>
> Hello,
>
> Jan Stancek <jstancek@redhat.com> writes:
>
> > If backing dev is btrfs root device, then starting best_match_len
> > from 1 creates an issue, because root (/) is never matched.
> > Also we should check that entire mount point string is present in
> > path we are matching against.
> >
> > In case there's error also dump /proc/self/mountinfo before tst_brk.
> >
> > This fixes test with following partition layout (TMPDIR is on /):
> > # cat /proc/self/mountinfo | grep btrfs
> > 59 1 0:29 /root / rw,relatime shared:1 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=257,subvol=/root
> > 93 59 0:29 /home /home rw,relatime shared:47 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=256,subvol=/home
> >
> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> > ---
> > lib/tst_device.c | 17 ++++++++++++-----
> > 1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/tst_device.c b/lib/tst_device.c
> > index c34cbe6d1f56..414bf0eea816 100644
> > --- a/lib/tst_device.c
> > +++ b/lib/tst_device.c
> > @@ -506,12 +506,17 @@ unsigned long tst_dev_bytes_written(const char *dev)
> > return dev_bytes_written;
> > }
> >
> > -static int count_match_len(const char *first, const char *second)
> > +static int str_starts_with(const char *str, const char *prefix)
> > {
> > int len = 0;
> >
> > - while (*first && *first++ == *second++)
> > + while (*prefix) {
> > + if (!*str)
> > + return 0;
> > + if (*str++ != *prefix++)
> > + return 0;
> > len++;
> > + }
>
> I'm not sure this is better than the original. It's a seperate cleanup
> in any case.
The difference is that partial matches now returns 0. Previously it did not,
it only counted number of matching characters. So for example ("/foobar" "/foo")
appeared as better match than ("/foobar", "/")
>
> >
> > return len;
> > }
> > @@ -524,7 +529,7 @@ void tst_find_backing_dev(const char *path, char *dev)
> > char *pre = NULL;
> > char *next = NULL;
> > unsigned int dev_major, dev_minor, line_mjr, line_mnr;
> > - unsigned int len, best_match_len = 1;
> > + unsigned int len, best_match_len = 0;
> > char mnt_point[PATH_MAX];
> >
> > if (stat(path, &buf) < 0)
> > @@ -550,7 +555,7 @@ void tst_find_backing_dev(const char *path, char *dev)
> > break;
> > }
> >
> > - len = count_match_len(path, mnt_point);
> > + len = str_starts_with(path, mnt_point);
> > if (len > best_match_len) {
> > strcpy(dev, pre);
> > best_match_len = len;
> > @@ -559,8 +564,10 @@ void tst_find_backing_dev(const char *path, char *dev)
> >
> > SAFE_FCLOSE(NULL, file);
> >
> > - if (!*dev)
> > + if (!*dev) {
> > + tst_system("cat /proc/self/mountinfo");
> > tst_brkm(TBROK, NULL, "Cannot find block device for %s", path);
> > + }
> >
> > if (stat(dev, &buf) < 0)
> > tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);
> > --
> > 2.27.0
>
> Makes sense. However I suspect this function can be replaced with the
> standard library method used in tst_stat_mount_dev. I didn't try this
> before because I'm not sure why it scans mountinfo instead of mounts.
/proc/self/mounts doesn't contain major/minor numbers, which is primarily what
tst_find_backing_dev is using.
>
>
> --
> Thank you,
> Richard.
>
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [LTP] [PATCH] tst_find_backing_dev: fix logic in matching mount point
2022-07-11 13:04 ` Jan Stancek
@ 2022-07-11 13:11 ` Richard Palethorpe
2022-07-19 9:07 ` Jan Stancek
0 siblings, 1 reply; 7+ messages in thread
From: Richard Palethorpe @ 2022-07-11 13:11 UTC (permalink / raw)
To: Jan Stancek; +Cc: LTP List
Hello,
Jan Stancek <jstancek@redhat.com> writes:
> On Mon, Jul 11, 2022 at 2:13 PM Richard Palethorpe <rpalethorpe@suse.de> wrote:
>>
>> Hello,
>>
>> Jan Stancek <jstancek@redhat.com> writes:
>>
>> > If backing dev is btrfs root device, then starting best_match_len
>> > from 1 creates an issue, because root (/) is never matched.
>> > Also we should check that entire mount point string is present in
>> > path we are matching against.
>> >
>> > In case there's error also dump /proc/self/mountinfo before tst_brk.
>> >
>> > This fixes test with following partition layout (TMPDIR is on /):
>> > # cat /proc/self/mountinfo | grep btrfs
>> > 59 1 0:29 /root / rw,relatime shared:1 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=257,subvol=/root
>> > 93 59 0:29 /home /home rw,relatime shared:47 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=256,subvol=/home
>> >
>> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
>> > ---
>> > lib/tst_device.c | 17 ++++++++++++-----
>> > 1 file changed, 12 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/lib/tst_device.c b/lib/tst_device.c
>> > index c34cbe6d1f56..414bf0eea816 100644
>> > --- a/lib/tst_device.c
>> > +++ b/lib/tst_device.c
>> > @@ -506,12 +506,17 @@ unsigned long tst_dev_bytes_written(const char *dev)
>> > return dev_bytes_written;
>> > }
>> >
>> > -static int count_match_len(const char *first, const char *second)
>> > +static int str_starts_with(const char *str, const char *prefix)
>> > {
>> > int len = 0;
>> >
>> > - while (*first && *first++ == *second++)
>> > + while (*prefix) {
>> > + if (!*str)
>> > + return 0;
>> > + if (*str++ != *prefix++)
>> > + return 0;
>> > len++;
>> > + }
>>
>> I'm not sure this is better than the original. It's a seperate cleanup
>> in any case.
>
> The difference is that partial matches now returns 0. Previously it did not,
> it only counted number of matching characters. So for example ("/foobar" "/foo")
> appeared as better match than ("/foobar", "/")
Ah, sorry.
>
>>
>> >
>> > return len;
>> > }
>> > @@ -524,7 +529,7 @@ void tst_find_backing_dev(const char *path, char *dev)
>> > char *pre = NULL;
>> > char *next = NULL;
>> > unsigned int dev_major, dev_minor, line_mjr, line_mnr;
>> > - unsigned int len, best_match_len = 1;
>> > + unsigned int len, best_match_len = 0;
>> > char mnt_point[PATH_MAX];
>> >
>> > if (stat(path, &buf) < 0)
>> > @@ -550,7 +555,7 @@ void tst_find_backing_dev(const char *path, char *dev)
>> > break;
>> > }
>> >
>> > - len = count_match_len(path, mnt_point);
>> > + len = str_starts_with(path, mnt_point);
>> > if (len > best_match_len) {
>> > strcpy(dev, pre);
>> > best_match_len = len;
>> > @@ -559,8 +564,10 @@ void tst_find_backing_dev(const char *path, char *dev)
>> >
>> > SAFE_FCLOSE(NULL, file);
>> >
>> > - if (!*dev)
>> > + if (!*dev) {
>> > + tst_system("cat /proc/self/mountinfo");
>> > tst_brkm(TBROK, NULL, "Cannot find block device for %s", path);
>> > + }
>> >
>> > if (stat(dev, &buf) < 0)
>> > tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);
>> > --
>> > 2.27.0
>>
>> Makes sense. However I suspect this function can be replaced with the
>> standard library method used in tst_stat_mount_dev. I didn't try this
>> before because I'm not sure why it scans mountinfo instead of mounts.
>
> /proc/self/mounts doesn't contain major/minor numbers, which is primarily what
> tst_find_backing_dev is using.
Does anyone know a situation where checking the mount path doesn't work?
It seems like we are just checking the device numbers first for
historical reasons and not because it is necessary.
I haven't seen any failure reports for io_control01 which only checks
the path with tst_stat_mount_dev. However this test only runs if you
have CG v2 enabled by default.
>
>>
>>
>> --
>> Thank you,
>> Richard.
>>
--
Thank you,
Richard.
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [LTP] [PATCH] tst_find_backing_dev: fix logic in matching mount point
2022-07-11 13:11 ` Richard Palethorpe
@ 2022-07-19 9:07 ` Jan Stancek
2022-10-10 12:51 ` Richard Palethorpe
0 siblings, 1 reply; 7+ messages in thread
From: Jan Stancek @ 2022-07-19 9:07 UTC (permalink / raw)
To: rpalethorpe; +Cc: LTP List
On Mon, Jul 11, 2022 at 3:21 PM Richard Palethorpe <rpalethorpe@suse.de> wrote:
>
> Hello,
>
> Jan Stancek <jstancek@redhat.com> writes:
>
> > On Mon, Jul 11, 2022 at 2:13 PM Richard Palethorpe <rpalethorpe@suse.de> wrote:
> >>
> >> Hello,
> >>
> >> Jan Stancek <jstancek@redhat.com> writes:
> >>
> >> > If backing dev is btrfs root device, then starting best_match_len
> >> > from 1 creates an issue, because root (/) is never matched.
> >> > Also we should check that entire mount point string is present in
> >> > path we are matching against.
> >> >
> >> > In case there's error also dump /proc/self/mountinfo before tst_brk.
> >> >
> >> > This fixes test with following partition layout (TMPDIR is on /):
> >> > # cat /proc/self/mountinfo | grep btrfs
> >> > 59 1 0:29 /root / rw,relatime shared:1 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=257,subvol=/root
> >> > 93 59 0:29 /home /home rw,relatime shared:47 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=256,subvol=/home
> >> >
> >> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> >> > ---
> >> > lib/tst_device.c | 17 ++++++++++++-----
> >> > 1 file changed, 12 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/lib/tst_device.c b/lib/tst_device.c
> >> > index c34cbe6d1f56..414bf0eea816 100644
> >> > --- a/lib/tst_device.c
> >> > +++ b/lib/tst_device.c
> >> > @@ -506,12 +506,17 @@ unsigned long tst_dev_bytes_written(const char *dev)
> >> > return dev_bytes_written;
> >> > }
> >> >
> >> > -static int count_match_len(const char *first, const char *second)
> >> > +static int str_starts_with(const char *str, const char *prefix)
> >> > {
> >> > int len = 0;
> >> >
> >> > - while (*first && *first++ == *second++)
> >> > + while (*prefix) {
> >> > + if (!*str)
> >> > + return 0;
> >> > + if (*str++ != *prefix++)
> >> > + return 0;
> >> > len++;
> >> > + }
> >>
> >> I'm not sure this is better than the original. It's a seperate cleanup
> >> in any case.
> >
> > The difference is that partial matches now returns 0. Previously it did not,
> > it only counted number of matching characters. So for example ("/foobar" "/foo")
> > appeared as better match than ("/foobar", "/")
>
> Ah, sorry.
>
> >
> >>
> >> >
> >> > return len;
> >> > }
> >> > @@ -524,7 +529,7 @@ void tst_find_backing_dev(const char *path, char *dev)
> >> > char *pre = NULL;
> >> > char *next = NULL;
> >> > unsigned int dev_major, dev_minor, line_mjr, line_mnr;
> >> > - unsigned int len, best_match_len = 1;
> >> > + unsigned int len, best_match_len = 0;
> >> > char mnt_point[PATH_MAX];
> >> >
> >> > if (stat(path, &buf) < 0)
> >> > @@ -550,7 +555,7 @@ void tst_find_backing_dev(const char *path, char *dev)
> >> > break;
> >> > }
> >> >
> >> > - len = count_match_len(path, mnt_point);
> >> > + len = str_starts_with(path, mnt_point);
> >> > if (len > best_match_len) {
> >> > strcpy(dev, pre);
> >> > best_match_len = len;
> >> > @@ -559,8 +564,10 @@ void tst_find_backing_dev(const char *path, char *dev)
> >> >
> >> > SAFE_FCLOSE(NULL, file);
> >> >
> >> > - if (!*dev)
> >> > + if (!*dev) {
> >> > + tst_system("cat /proc/self/mountinfo");
> >> > tst_brkm(TBROK, NULL, "Cannot find block device for %s", path);
> >> > + }
> >> >
> >> > if (stat(dev, &buf) < 0)
> >> > tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);
> >> > --
> >> > 2.27.0
> >>
> >> Makes sense. However I suspect this function can be replaced with the
> >> standard library method used in tst_stat_mount_dev. I didn't try this
> >> before because I'm not sure why it scans mountinfo instead of mounts.
> >
> > /proc/self/mounts doesn't contain major/minor numbers, which is primarily what
> > tst_find_backing_dev is using.
>
> Does anyone know a situation where checking the mount path doesn't work?
You could mount multiple times to same mount path, which would make it harder
to find the right one.
> It seems like we are just checking the device numbers first for
> historical reasons and not because it is necessary.
Device numbers seem slightly less ambiguous.
>
> I haven't seen any failure reports for io_control01 which only checks
> the path with tst_stat_mount_dev. However this test only runs if you
> have CG v2 enabled by default.
>
> >
> >>
> >>
> >> --
> >> Thank you,
> >> Richard.
> >>
>
>
> --
> Thank you,
> Richard.
>
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [LTP] [PATCH] tst_find_backing_dev: fix logic in matching mount point
2022-07-19 9:07 ` Jan Stancek
@ 2022-10-10 12:51 ` Richard Palethorpe
0 siblings, 0 replies; 7+ messages in thread
From: Richard Palethorpe @ 2022-10-10 12:51 UTC (permalink / raw)
To: Jan Stancek; +Cc: LTP List
Hello,
Jan Stancek <jstancek@redhat.com> writes:
> On Mon, Jul 11, 2022 at 3:21 PM Richard Palethorpe <rpalethorpe@suse.de> wrote:
>>
>> Hello,
>>
>> Jan Stancek <jstancek@redhat.com> writes:
>>
>> > On Mon, Jul 11, 2022 at 2:13 PM Richard Palethorpe <rpalethorpe@suse.de> wrote:
>> >>
>> >> Hello,
>> >>
>> >> Jan Stancek <jstancek@redhat.com> writes:
>> >>
>> >> > If backing dev is btrfs root device, then starting best_match_len
>> >> > from 1 creates an issue, because root (/) is never matched.
>> >> > Also we should check that entire mount point string is present in
>> >> > path we are matching against.
>> >> >
>> >> > In case there's error also dump /proc/self/mountinfo before tst_brk.
>> >> >
>> >> > This fixes test with following partition layout (TMPDIR is on /):
>> >> > # cat /proc/self/mountinfo | grep btrfs
>> >> > 59 1 0:29 /root / rw,relatime shared:1 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=257,subvol=/root
>> >> > 93 59 0:29 /home /home rw,relatime shared:47 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=256,subvol=/home
>> >> >
>> >> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
>> >> > ---
>> >> > lib/tst_device.c | 17 ++++++++++++-----
>> >> > 1 file changed, 12 insertions(+), 5 deletions(-)
>> >> >
>> >> > diff --git a/lib/tst_device.c b/lib/tst_device.c
>> >> > index c34cbe6d1f56..414bf0eea816 100644
>> >> > --- a/lib/tst_device.c
>> >> > +++ b/lib/tst_device.c
>> >> > @@ -506,12 +506,17 @@ unsigned long tst_dev_bytes_written(const char *dev)
>> >> > return dev_bytes_written;
>> >> > }
>> >> >
>> >> > -static int count_match_len(const char *first, const char *second)
>> >> > +static int str_starts_with(const char *str, const char *prefix)
>> >> > {
>> >> > int len = 0;
>> >> >
>> >> > - while (*first && *first++ == *second++)
>> >> > + while (*prefix) {
>> >> > + if (!*str)
>> >> > + return 0;
>> >> > + if (*str++ != *prefix++)
>> >> > + return 0;
>> >> > len++;
>> >> > + }
>> >>
>> >> I'm not sure this is better than the original. It's a seperate cleanup
>> >> in any case.
>> >
>> > The difference is that partial matches now returns 0. Previously it did not,
>> > it only counted number of matching characters. So for example ("/foobar" "/foo")
>> > appeared as better match than ("/foobar", "/")
>>
>> Ah, sorry.
>>
>> >
>> >>
>> >> >
>> >> > return len;
>> >> > }
>> >> > @@ -524,7 +529,7 @@ void tst_find_backing_dev(const char *path, char *dev)
>> >> > char *pre = NULL;
>> >> > char *next = NULL;
>> >> > unsigned int dev_major, dev_minor, line_mjr, line_mnr;
>> >> > - unsigned int len, best_match_len = 1;
>> >> > + unsigned int len, best_match_len = 0;
>> >> > char mnt_point[PATH_MAX];
>> >> >
>> >> > if (stat(path, &buf) < 0)
>> >> > @@ -550,7 +555,7 @@ void tst_find_backing_dev(const char *path, char *dev)
>> >> > break;
>> >> > }
>> >> >
>> >> > - len = count_match_len(path, mnt_point);
>> >> > + len = str_starts_with(path, mnt_point);
>> >> > if (len > best_match_len) {
>> >> > strcpy(dev, pre);
>> >> > best_match_len = len;
>> >> > @@ -559,8 +564,10 @@ void tst_find_backing_dev(const char *path, char *dev)
>> >> >
>> >> > SAFE_FCLOSE(NULL, file);
>> >> >
>> >> > - if (!*dev)
>> >> > + if (!*dev) {
>> >> > + tst_system("cat /proc/self/mountinfo");
>> >> > tst_brkm(TBROK, NULL, "Cannot find block device for %s", path);
>> >> > + }
>> >> >
>> >> > if (stat(dev, &buf) < 0)
>> >> > tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);
>> >> > --
>> >> > 2.27.0
>> >>
>> >> Makes sense. However I suspect this function can be replaced with the
>> >> standard library method used in tst_stat_mount_dev. I didn't try this
>> >> before because I'm not sure why it scans mountinfo instead of mounts.
>> >
>> > /proc/self/mounts doesn't contain major/minor numbers, which is primarily what
>> > tst_find_backing_dev is using.
>>
>> Does anyone know a situation where checking the mount path doesn't work?
>
> You could mount multiple times to same mount path, which would make it harder
> to find the right one.
>
>> It seems like we are just checking the device numbers first for
>> historical reasons and not because it is necessary.
>
> Device numbers seem slightly less ambiguous.
I'm still not entirely sure about this, but if you want to merge then
please go ahead.
Acked-by: Richard Palethorpe <rpalethorpe@suse.com>
>
>>
>> I haven't seen any failure reports for io_control01 which only checks
>> the path with tst_stat_mount_dev. However this test only runs if you
>> have CG v2 enabled by default.
>>
>> >
>> >>
>> >>
>> >> --
>> >> Thank you,
>> >> Richard.
>> >>
>>
>>
>> --
>> Thank you,
>> Richard.
>>
--
Thank you,
Richard.
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-10-10 13:13 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-08 7:33 [LTP] [PATCH] tst_find_backing_dev: fix logic in matching mount point Jan Stancek
2022-07-11 6:52 ` Li Wang
2022-07-11 12:05 ` Richard Palethorpe
2022-07-11 13:04 ` Jan Stancek
2022-07-11 13:11 ` Richard Palethorpe
2022-07-19 9:07 ` Jan Stancek
2022-10-10 12:51 ` Richard Palethorpe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.