GNU bug report logs - #21372
df prioritizes some bind mounts over real ones.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 21372 in the body.
You can then email your comments to 21372 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dave Chiluk <chiluk <at> canonical.com>
To: bug-coreutils <at> gnu.org
Subject: df prioritizes some bind mounts over real ones.
Date: Fri, 28 Aug 2015 15:42:44 -0500

With the recent move to /proc/self/mountinfo `df` now returns bind mounts
instead of the original mount when the directory being bound to is shorter than
the original mount destination.

For example:
 * $ mount
<snip>
192.168.1.2:/raid on /raid type nfs
/dev/sdc5 on /data type ext4 (rw)
<snip>

$ mount -o bind /data/a on /a type none (rw,bind)
$ mount -o bind /raid/temp on /b type none (rw,bind)

$df
Filesystem 1K-blocks Used Available Use% Mounted on
<snip>
192.168.1.2:/raid 449830616 229975284 196982196 54% /a
/dev/sdc5 7752072192 5581343744 1780023296 76% /b
</snip>

I'd expect to see the original mount prioritized over the bind mount. Like so.
$ df
Filesystem 1K-blocks Used Available Use% Mounted on
<snip>
/dev/sdc5 449830616 229975284 196982196 54% /data
192.168.1.2:/raid 7752072192 5581438976 1779929088 76% /raid
<snip>

This is both mildly incorrect and misleading.  As the source device in both
cases isn't exactly what is being mounted at the destination directory.
Instead it is a subdir of the filesystem that is being mounted there.

I'm submitting patches to both gnulib and coreutils to correct this issue.

Thank you,
Dave Chiluk

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dave Chiluk <chiluk <at> canonical.com>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] df: fix prioritize real mounts over bind mounts
Date: Fri, 28 Aug 2015 15:42:45 -0500

Fixes an issue where bind mounts with shorter mount directories than the
original mount are prioritized when running df.  The root cause of this
is that /proc/self/mountinfo now lists the filesystem device with bind
mounts rather than the source directory. With /etc/mtab the source
device was listed as the originating directory so this was not an issue.

More information is available here.
https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/1432871
---
 src/df.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/df.c b/src/df.c
index 2e541b9..13e2661 100644
--- a/src/df.c
+++ b/src/df.c
@@ -652,9 +652,12 @@ filter_mount_list (bool devices_only)
               else if ((strchr (me->me_devname, '/')
                        /* let "real" devices with '/' in the name win.  */
                         && ! strchr (devlist->me->me_devname, '/'))
-                       /* let a shorter mountdir win.  */
-                       || (strlen (devlist->me->me_mountdir)
+                       /* let a shorter mountdir win. */
+                       /* Only if it's not a bind mount.*/
+                       || ((strlen (devlist->me->me_mountdir)
                            > strlen (me->me_mountdir))
+                          && (strlen (devlist->me->me_mountroot)
+                           > strlen(me->me_mountroot)))
                        /* let an entry overmounted on a new device win...  */
                        || (! STREQ (devlist->me->me_devname, me->me_devname)
                            /* ... but only when matching an existing mnt point,
-- 
1.9.1

Message #11 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Dave Chiluk <chiluk <at> canonical.com>, 21372 <at> debbugs.gnu.org
Subject: Re: bug#21372: [PATCH] df: fix prioritize real mounts over bind mounts
Date: Sat, 29 Aug 2015 01:37:55 +0100

On 28/08/15 21:42, Dave Chiluk wrote:
> Fixes an issue where bind mounts with shorter mount directories than the
> original mount are prioritized when running df.  The root cause of this
> is that /proc/self/mountinfo now lists the filesystem device with bind
> mounts rather than the source directory. With /etc/mtab the source
> device was listed as the originating directory so this was not an issue.
> 
> More information is available here.
> https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/1432871

It's debatable as to whether we should prefer standard mounts
over bind mounts, but I'm 60:40 for preferring mount points
(more towards) the root of the device.

> ---
>  src/df.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/df.c b/src/df.c
> index 2e541b9..13e2661 100644
> --- a/src/df.c
> +++ b/src/df.c
> @@ -652,9 +652,12 @@ filter_mount_list (bool devices_only)
>                else if ((strchr (me->me_devname, '/')
>                         /* let "real" devices with '/' in the name win.  */
>                          && ! strchr (devlist->me->me_devname, '/'))
> -                       /* let a shorter mountdir win.  */
> -                       || (strlen (devlist->me->me_mountdir)
> +                       /* let a shorter mountdir win. */
> +                       /* Only if it's not a bind mount.*/
> +                       || ((strlen (devlist->me->me_mountdir)
>                             > strlen (me->me_mountdir))
> +                          && (strlen (devlist->me->me_mountroot)
> +                           > strlen(me->me_mountroot)))

I think this should be:       >= strlen (me->me_mountroot)

>                         /* let an entry overmounted on a new device win...  */
>                         || (! STREQ (devlist->me->me_devname, me->me_devname)
>                             /* ... but only when matching an existing mnt point,
> 

Cool, so that depends on http://lists.gnu.org/archive/html/bug-gnulib/2015-08/msg00030.html

I had some personal notes RE the mountroot element,
which I'll add here for reference.

  "$ findmnt --df shows bind mounts (and BTRFS subvols) like:
    SOURCE          FSTYPE            SIZE  USED  AVAIL USE% TARGET
    /dev/sdb2       xfs              12.7G  9.4G   3.3G  74% /
    /dev/sdb2[/etc] xfs                                      /root/chroot
  Note the blank stats for the bind mount (df would probably use - - - here).
  The mountroot is presented in mountinfo and we could adjust mountlist.c
  to return this element, to allow df to present the bindmount info?
  Not sure about that actually since findmnt already does this, and also df is
  DEVICE oriented as per the name."

So while we mightn't distinguish the bind mounts like findmnt does,
it seems it's still useful to return the mountroot element.
This should not introduce API concerns since gnulib is not
released as a separate lib.

thanks!
Pádraig.

Message #14 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Dave Chiluk <chiluk <at> canonical.com>, 21372 <at> debbugs.gnu.org
Subject: Re: bug#21372: [PATCH] df: fix prioritize real mounts over bind mounts
Date: Sat, 29 Aug 2015 04:44:34 +0100

On 29/08/15 01:37, Pádraig Brady wrote:
> On 28/08/15 21:42, Dave Chiluk wrote:

>> diff --git a/src/df.c b/src/df.c
>> index 2e541b9..13e2661 100644
>> --- a/src/df.c
>> +++ b/src/df.c
>> @@ -652,9 +652,12 @@ filter_mount_list (bool devices_only)
>>                else if ((strchr (me->me_devname, '/')
>>                         /* let "real" devices with '/' in the name win.  */
>>                          && ! strchr (devlist->me->me_devname, '/'))
>> -                       /* let a shorter mountdir win.  */
>> -                       || (strlen (devlist->me->me_mountdir)
>> +                       /* let a shorter mountdir win. */
>> +                       /* Only if it's not a bind mount.*/
>> +                       || ((strlen (devlist->me->me_mountdir)
>>                             > strlen (me->me_mountdir))
>> +                          && (strlen (devlist->me->me_mountroot)
>> +                           > strlen(me->me_mountroot)))
> 
> I think this should be:       >= strlen (me->me_mountroot)

Also we need to cater for NULL me_mountroot.

Message #17 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Dave Chiluk <chiluk <at> canonical.com>
To: Pádraig Brady <P <at> draigBrady.com>, 
 21372 <at> debbugs.gnu.org
Subject: Re: bug#21372: [PATCH] df: fix prioritize real mounts over bind mounts
Date: Mon, 31 Aug 2015 15:11:28 -0500

On 08/28/2015 10:44 PM, Pádraig Brady wrote:
> On 29/08/15 01:37, Pádraig Brady wrote:
>> On 28/08/15 21:42, Dave Chiluk wrote:
> 
>>> diff --git a/src/df.c b/src/df.c
>>> index 2e541b9..13e2661 100644
>>> --- a/src/df.c
>>> +++ b/src/df.c
>>> @@ -652,9 +652,12 @@ filter_mount_list (bool devices_only)
>>>                else if ((strchr (me->me_devname, '/')
>>>                         /* let "real" devices with '/' in the name win.  */
>>>                          && ! strchr (devlist->me->me_devname, '/'))
>>> -                       /* let a shorter mountdir win.  */
>>> -                       || (strlen (devlist->me->me_mountdir)
>>> +                       /* let a shorter mountdir win. */
>>> +                       /* Only if it's not a bind mount.*/
>>> +                       || ((strlen (devlist->me->me_mountdir)
>>>                             > strlen (me->me_mountdir))
>>> +                          && (strlen (devlist->me->me_mountroot)
>>> +                           > strlen(me->me_mountroot)))
>>
>> I think this should be:       >= strlen (me->me_mountroot)
> 
> Also we need to cater for NULL me_mountroot.
> 

All good review suggestions, I'll post a revised patch shortly.

Dave.

Message #20 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Dave Chiluk <chiluk <at> canonical.com>
To: P <at> draigBrady.com,
	21372 <at> debbugs.gnu.org
Subject: [PATCH] df: fix prioritize real mounts over bind mounts
Date: Mon, 31 Aug 2015 16:05:00 -0500

Fixes an issue where bind mounts with shorter mount directories than the
original mount are prioritized when running df.  The root cause of this
is that /proc/self/mountinfo now lists the filesystem device with bind
mounts rather than the source directory. With /etc/mtab the source
device was listed as the originating directory so this was not an issue.

More information is available here.
https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/1432871
---
 src/df.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/df.c b/src/df.c
index 2e541b9..00c77c1 100644
--- a/src/df.c
+++ b/src/df.c
@@ -652,9 +652,14 @@ filter_mount_list (bool devices_only)
               else if ((strchr (me->me_devname, '/')
                        /* let "real" devices with '/' in the name win.  */
                         && ! strchr (devlist->me->me_devname, '/'))
-                       /* let a shorter mountdir win.  */
-                       || (strlen (devlist->me->me_mountdir)
+                       /* let a shorter mountdir win. */
+                       /* Only if it's not a bind mount. */
+                       || ((strlen (devlist->me->me_mountdir)
                            > strlen (me->me_mountdir))
+                          && (devlist->me->me_mntroot != NULL
+                             && me->me_mntroot != NULL
+                             && (strlen (devlist->me->me_mntroot)
+                                 > strlen(me->me_mntroot))))
                        /* let an entry overmounted on a new device win...  */
                        || (! STREQ (devlist->me->me_devname, me->me_devname)
                            /* ... but only when matching an existing mnt point,
-- 
1.9.1

Message #23 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Dave Chiluk <dave.chiluk <at> canonical.com>
To: Pádraig Brady <P <at> draigBrady.com>, 
 21372 <at> debbugs.gnu.org
Subject: Re: bug#21372: [PATCH] df: fix prioritize real mounts over bind mounts
Date: Fri, 11 Sep 2015 14:33:51 -0500

I sent out a revised patch here.
http://lists.gnu.org/archive/html/bug-coreutils/2015-08/msg00087.html

Have you had any chance to take a look at it?

Thanks,
Dave Chiluk

On 08/31/2015 03:11 PM, Dave Chiluk wrote:
> On 08/28/2015 10:44 PM, Pádraig Brady wrote:
>> On 29/08/15 01:37, Pádraig Brady wrote:
>>> On 28/08/15 21:42, Dave Chiluk wrote:
>>
>>>> diff --git a/src/df.c b/src/df.c
>>>> index 2e541b9..13e2661 100644
>>>> --- a/src/df.c
>>>> +++ b/src/df.c
>>>> @@ -652,9 +652,12 @@ filter_mount_list (bool devices_only)
>>>>                else if ((strchr (me->me_devname, '/')
>>>>                         /* let "real" devices with '/' in the name win.  */
>>>>                          && ! strchr (devlist->me->me_devname, '/'))
>>>> -                       /* let a shorter mountdir win.  */
>>>> -                       || (strlen (devlist->me->me_mountdir)
>>>> +                       /* let a shorter mountdir win. */
>>>> +                       /* Only if it's not a bind mount.*/
>>>> +                       || ((strlen (devlist->me->me_mountdir)
>>>>                             > strlen (me->me_mountdir))
>>>> +                          && (strlen (devlist->me->me_mountroot)
>>>> +                           > strlen(me->me_mountroot)))
>>>
>>> I think this should be:       >= strlen (me->me_mountroot)
>>
>> Also we need to cater for NULL me_mountroot.
>>
> 
> All good review suggestions, I'll post a revised patch shortly.
> 
> Dave.
> 

Message #26 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Dave Chiluk <chiluk <at> canonical.com>, 21372 <at> debbugs.gnu.org
Subject: Re: bug#21372: [PATCH] df: fix prioritize real mounts over bind mounts
Date: Fri, 11 Sep 2015 21:42:20 +0100

On 31/08/15 22:05, Dave Chiluk wrote:
> Fixes an issue where bind mounts with shorter mount directories than the
> original mount are prioritized when running df.  The root cause of this
> is that /proc/self/mountinfo now lists the filesystem device with bind
> mounts rather than the source directory. With /etc/mtab the source
> device was listed as the originating directory so this was not an issue.
> 
> More information is available here.
> https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/1432871
> ---
>  src/df.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/df.c b/src/df.c
> index 2e541b9..00c77c1 100644
> --- a/src/df.c
> +++ b/src/df.c
> @@ -652,9 +652,14 @@ filter_mount_list (bool devices_only)
>                else if ((strchr (me->me_devname, '/')
>                         /* let "real" devices with '/' in the name win.  */
>                          && ! strchr (devlist->me->me_devname, '/'))
> -                       /* let a shorter mountdir win.  */
> -                       || (strlen (devlist->me->me_mountdir)
> +                       /* let a shorter mountdir win. */
> +                       /* Only if it's not a bind mount. */
> +                       || ((strlen (devlist->me->me_mountdir)
>                             > strlen (me->me_mountdir))
> +                          && (devlist->me->me_mntroot != NULL
> +                             && me->me_mntroot != NULL
> +                             && (strlen (devlist->me->me_mntroot)
> +                                 > strlen(me->me_mntroot))))

As previously mentioned, should this be >= ?

We'll need to look at tests.
This will be quite tricky given the current
avoidance of /proc/self/mountinfo in tests.
I'll look at adding a separate test for this.

thanks,
Pádraig.

Message #29 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Dave Chiluk <dave.chiluk <at> canonical.com>
To: Pádraig Brady <P <at> draigBrady.com>, 
 Dave Chiluk <chiluk <at> canonical.com>, 21372 <at> debbugs.gnu.org
Subject: Re: bug#21372: [PATCH] df: fix prioritize real mounts over bind mounts
Date: Fri, 11 Sep 2015 16:55:07 -0500

On 09/11/2015 03:42 PM, Pádraig Brady wrote:
> On 31/08/15 22:05, Dave Chiluk wrote:
>> Fixes an issue where bind mounts with shorter mount directories than the
>> original mount are prioritized when running df.  The root cause of this
>> is that /proc/self/mountinfo now lists the filesystem device with bind
>> mounts rather than the source directory. With /etc/mtab the source
>> device was listed as the originating directory so this was not an issue.
>>
>> More information is available here.
>> https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/1432871
>> ---
>>  src/df.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/df.c b/src/df.c
>> index 2e541b9..00c77c1 100644
>> --- a/src/df.c
>> +++ b/src/df.c
>> @@ -652,9 +652,14 @@ filter_mount_list (bool devices_only)
>>                else if ((strchr (me->me_devname, '/')
>>                         /* let "real" devices with '/' in the name win.  */
>>                          && ! strchr (devlist->me->me_devname, '/'))
>> -                       /* let a shorter mountdir win.  */
>> -                       || (strlen (devlist->me->me_mountdir)
>> +                       /* let a shorter mountdir win. */
>> +                       /* Only if it's not a bind mount. */
>> +                       || ((strlen (devlist->me->me_mountdir)
>>                             > strlen (me->me_mountdir))
>> +                          && (devlist->me->me_mntroot != NULL
>> +                             && me->me_mntroot != NULL
>> +                             && (strlen (devlist->me->me_mntroot)
>> +                                 > strlen(me->me_mntroot))))
> 
> As previously mentioned, should this be >= ?
Sorry my apologies for not addressing that.  I don't think it should be
(strlen (devlist->me->me_mntroot)>= strlen(me->me_mntroot)), because
that would give preference to later mounts of the same me_mntroot
directory *(I'm specifically thinking of the case where both me_mntroot
= "/").

However, looking at the code again, I realized that by adding the NULL
checks I have actually changed the behavior for non-linux OS's since the
NULL checks would always defeat the (strlen (devlist->me->me_mountdir) >
strlen (me->me_mountdir) check.

I'm going to think through this nasty if case and see if I can make it
prettier.

> 
> We'll need to look at tests.
> This will be quite tricky given the current
> avoidance of /proc/self/mountinfo in tests.
> I'll look at adding a separate test for this.
> 
> thanks,
> Pádraig.
> 

Additionally I agree with you that we should mirror the find_mnt
bracketting behavior.
    /dev/sdb2       xfs              12.7G  9.4G   3.3G  74% /
    /dev/sdb2[/etc] xfs                                     /root/chroot

I'll submit that in a different patch once we have this more squared away.

Thanks,
Dave.

Message #32 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: dave.chiluk <at> canonical.com, Dave Chiluk <chiluk <at> canonical.com>, 
 21372 <at> debbugs.gnu.org
Subject: Re: bug#21372: [PATCH] df: fix prioritize real mounts over bind mounts
Date: Fri, 11 Sep 2015 23:02:33 +0100

On 11/09/15 22:55, Dave Chiluk wrote:
> On 09/11/2015 03:42 PM, Pádraig Brady wrote:
>> On 31/08/15 22:05, Dave Chiluk wrote:
>>> Fixes an issue where bind mounts with shorter mount directories than the
>>> original mount are prioritized when running df.  The root cause of this
>>> is that /proc/self/mountinfo now lists the filesystem device with bind
>>> mounts rather than the source directory. With /etc/mtab the source
>>> device was listed as the originating directory so this was not an issue.
>>>
>>> More information is available here.
>>> https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/1432871
>>> ---
>>>  src/df.c | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/df.c b/src/df.c
>>> index 2e541b9..00c77c1 100644
>>> --- a/src/df.c
>>> +++ b/src/df.c
>>> @@ -652,9 +652,14 @@ filter_mount_list (bool devices_only)
>>>                else if ((strchr (me->me_devname, '/')
>>>                         /* let "real" devices with '/' in the name win.  */
>>>                          && ! strchr (devlist->me->me_devname, '/'))
>>> -                       /* let a shorter mountdir win.  */
>>> -                       || (strlen (devlist->me->me_mountdir)
>>> +                       /* let a shorter mountdir win. */
>>> +                       /* Only if it's not a bind mount. */
>>> +                       || ((strlen (devlist->me->me_mountdir)
>>>                             > strlen (me->me_mountdir))
>>> +                          && (devlist->me->me_mntroot != NULL
>>> +                             && me->me_mntroot != NULL
>>> +                             && (strlen (devlist->me->me_mntroot)
>>> +                                 > strlen(me->me_mntroot))))
>>
>> As previously mentioned, should this be >= ?
> Sorry my apologies for not addressing that.  I don't think it should be
> (strlen (devlist->me->me_mntroot)>= strlen(me->me_mntroot)), because
> that would give preference to later mounts of the same me_mntroot
> directory *(I'm specifically thinking of the case where both me_mntroot
> = "/").
> 
> However, looking at the code again, I realized that by adding the NULL
> checks I have actually changed the behavior for non-linux OS's since the
> NULL checks would always defeat the (strlen (devlist->me->me_mountdir) >
> strlen (me->me_mountdir) check.
> 
> I'm going to think through this nasty if case and see if I can make it
> prettier.
> 
>>
>> We'll need to look at tests.
>> This will be quite tricky given the current
>> avoidance of /proc/self/mountinfo in tests.
>> I'll look at adding a separate test for this.
>>
>> thanks,
>> Pádraig.
>>
> 
> Additionally I agree with you that we should mirror the find_mnt
> bracketting behavior.
>     /dev/sdb2       xfs              12.7G  9.4G   3.3G  74% /
>     /dev/sdb2[/etc] xfs                                     /root/chroot
> 
> I'll submit that in a different patch once we have this more squared away.


Well I mentioned that df was "DEVICE oriented" and so should
probably not worry about distinguishing that, especially as
the functionality is already provided by findmnt(1).

thanks,
Pádraig

Message #35 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Dave Chiluk <chiluk <at> canonical.com>
To: 21372 <at> debbugs.gnu.org, P <at> draigBrady.com, Dave Chiluk <chiluk <at> canonical.com>
Subject: [PATCH v3] df: fix prioritize real mounts over bind mounts
Date: Mon, 21 Sep 2015 14:52:33 -0500

Fixes an issue where bind mounts with shorter mount directories than the
original mount are prioritized when running df.  The root cause of this
is that /proc/self/mountinfo now lists the filesystem device with bind
mounts rather than the source directory. With /etc/mtab the source
device was listed as the originating directory so this was not an issue.

More information is available here.
https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/1432871
---
 src/df.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/df.c b/src/df.c
index 2e541b9..fa13c25 100644
--- a/src/df.c
+++ b/src/df.c
@@ -652,9 +652,14 @@ filter_mount_list (bool devices_only)
               else if ((strchr (me->me_devname, '/')
                        /* let "real" devices with '/' in the name win.  */
                         && ! strchr (devlist->me->me_devname, '/'))
-                       /* let a shorter mountdir win.  */
-                       || (strlen (devlist->me->me_mountdir)
+                       /* let a shorter mountdir win. If it's mntroot is */
+                       /* also shorter. i.e. not a bind mount. */
+                       || ((strlen (devlist->me->me_mountdir)
                            > strlen (me->me_mountdir))
+                          && (devlist->me->me_mntroot == NULL
+                             || me->me_mntroot == NULL
+                             || (strlen (devlist->me->me_mntroot)
+                                 > strlen(me->me_mntroot))))
                        /* let an entry overmounted on a new device win...  */
                        || (! STREQ (devlist->me->me_devname, me->me_devname)
                            /* ... but only when matching an existing mnt point,
-- 
1.9.1

Message #38 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Dave Chiluk <dave.chiluk <at> canonical.com>
To: Dave Chiluk <chiluk <at> canonical.com>, 21372 <at> debbugs.gnu.org, 
 P <at> draigBrady.com
Subject: Re: [PATCH v3] df: fix prioritize real mounts over bind mounts
Date: Mon, 21 Sep 2015 14:57:20 -0500

Ignore this patch, I just had an epiphany realized why
 (strlen (devlist->me->me_mntroot) >= strlen(me->me_mntroot))))
should be the logic.  v4 coming shortly.

Sorry,
Dave.

On 09/21/2015 02:52 PM, Dave Chiluk wrote:
> Fixes an issue where bind mounts with shorter mount directories than the
> original mount are prioritized when running df.  The root cause of this
> is that /proc/self/mountinfo now lists the filesystem device with bind
> mounts rather than the source directory. With /etc/mtab the source
> device was listed as the originating directory so this was not an issue.
> 
> More information is available here.
> https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/1432871
> ---
>  src/df.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/df.c b/src/df.c
> index 2e541b9..fa13c25 100644
> --- a/src/df.c
> +++ b/src/df.c
> @@ -652,9 +652,14 @@ filter_mount_list (bool devices_only)
>                else if ((strchr (me->me_devname, '/')
>                         /* let "real" devices with '/' in the name win.  */
>                          && ! strchr (devlist->me->me_devname, '/'))
> -                       /* let a shorter mountdir win.  */
> -                       || (strlen (devlist->me->me_mountdir)
> +                       /* let a shorter mountdir win. If it's mntroot is */
> +                       /* also shorter. i.e. not a bind mount. */
> +                       || ((strlen (devlist->me->me_mountdir)
>                             > strlen (me->me_mountdir))
> +                          && (devlist->me->me_mntroot == NULL
> +                             || me->me_mntroot == NULL
> +                             || (strlen (devlist->me->me_mntroot)
> +                                 > strlen(me->me_mntroot))))
>                         /* let an entry overmounted on a new device win...  */
>                         || (! STREQ (devlist->me->me_devname, me->me_devname)
>                             /* ... but only when matching an existing mnt point,
> 

Message #41 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Dave Chiluk <chiluk <at> canonical.com>
To: 21372 <at> debbugs.gnu.org, P <at> draigBrady.com, Dave Chiluk <chiluk <at> canonical.com>
Subject: [PATCH v4] df: fix prioritize real mounts over bind mounts
Date: Mon, 21 Sep 2015 15:04:11 -0500

Fixes an issue where bind mounts with shorter mount directories than the
original mount are prioritized when running df.  The root cause of this
is that /proc/self/mountinfo now lists the filesystem device with bind
mounts rather than the source directory. With /etc/mtab the source
device was listed as the originating directory so this was not an issue.

More information is available here.
https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/1432871
---
 src/df.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/df.c b/src/df.c
index 2e541b9..4a3afa2 100644
--- a/src/df.c
+++ b/src/df.c
@@ -652,9 +652,14 @@ filter_mount_list (bool devices_only)
               else if ((strchr (me->me_devname, '/')
                        /* let "real" devices with '/' in the name win.  */
                         && ! strchr (devlist->me->me_devname, '/'))
-                       /* let a shorter mountdir win.  */
-                       || (strlen (devlist->me->me_mountdir)
+                       /* let a shorter mountdir win. If it's mntroot is */
+                       /* also shorter. i.e. not a bind mount. */
+                       || ((strlen (devlist->me->me_mountdir)
                            > strlen (me->me_mountdir))
+                          && (devlist->me->me_mntroot == NULL
+                             || me->me_mntroot == NULL
+                             || (strlen (devlist->me->me_mntroot)
+                                 >= strlen(me->me_mntroot))))
                        /* let an entry overmounted on a new device win...  */
                        || (! STREQ (devlist->me->me_devname, me->me_devname)
                            /* ... but only when matching an existing mnt point,
-- 
1.9.1

Message #46 received at 21372-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Dave Chiluk <chiluk <at> canonical.com>, 21372-done <at> debbugs.gnu.org
Subject: Re: bug#21372: [PATCH v4] df: fix prioritize real mounts over bind
 mounts
Date: Mon, 21 Sep 2015 23:44:53 +0100

On 21/09/15 21:04, Dave Chiluk wrote:
> Fixes an issue where bind mounts with shorter mount directories than the
> original mount are prioritized when running df.  The root cause of this
> is that /proc/self/mountinfo now lists the filesystem device with bind
> mounts rather than the source directory. With /etc/mtab the source
> device was listed as the originating directory so this was not an issue.
> 
> More information is available here.
> https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/1432871
> ---
>  src/df.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/df.c b/src/df.c
> index 2e541b9..4a3afa2 100644
> --- a/src/df.c
> +++ b/src/df.c
> @@ -652,9 +652,14 @@ filter_mount_list (bool devices_only)
>                else if ((strchr (me->me_devname, '/')
>                         /* let "real" devices with '/' in the name win.  */
>                          && ! strchr (devlist->me->me_devname, '/'))
> -                       /* let a shorter mountdir win.  */
> -                       || (strlen (devlist->me->me_mountdir)
> +                       /* let a shorter mountdir win. If it's mntroot is */
> +                       /* also shorter. i.e. not a bind mount. */
> +                       || ((strlen (devlist->me->me_mountdir)
>                             > strlen (me->me_mountdir))
> +                          && (devlist->me->me_mntroot == NULL
> +                             || me->me_mntroot == NULL
> +                             || (strlen (devlist->me->me_mntroot)
> +                                 >= strlen(me->me_mntroot))))
>                         /* let an entry overmounted on a new device win...  */
>                         || (! STREQ (devlist->me->me_devname, me->me_devname)
>                             /* ... but only when matching an existing mnt point,
> 

This logic looks correct thanks.
I'll adjust the comment a bit and maybe refactor
all the conditions to a single nearer_device_root boolean.

I'll push this later on,
with the associated gnulib update.

thanks,
Pádraig.

Message #49 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Dave Chiluk <chiluk <at> canonical.com>, 21372 <at> debbugs.gnu.org
Subject: Re: bug#21372: [PATCH v4] df: fix prioritize real mounts over bind
 mounts
Date: Tue, 22 Sep 2015 01:43:33 +0100

[Message part 1 (text/plain, inline)]

On 21/09/15 23:44, Pádraig Brady wrote:
> On 21/09/15 21:04, Dave Chiluk wrote:
>> Fixes an issue where bind mounts with shorter mount directories than the
>> original mount are prioritized when running df.  The root cause of this
>> is that /proc/self/mountinfo now lists the filesystem device with bind
>> mounts rather than the source directory. With /etc/mtab the source
>> device was listed as the originating directory so this was not an issue.
>>
>> More information is available here.
>> https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/1432871
>> ---
>>  src/df.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/df.c b/src/df.c
>> index 2e541b9..4a3afa2 100644
>> --- a/src/df.c
>> +++ b/src/df.c
>> @@ -652,9 +652,14 @@ filter_mount_list (bool devices_only)
>>                else if ((strchr (me->me_devname, '/')
>>                         /* let "real" devices with '/' in the name win.  */
>>                          && ! strchr (devlist->me->me_devname, '/'))
>> -                       /* let a shorter mountdir win.  */
>> -                       || (strlen (devlist->me->me_mountdir)
>> +                       /* let a shorter mountdir win. If it's mntroot is */
>> +                       /* also shorter. i.e. not a bind mount. */
>> +                       || ((strlen (devlist->me->me_mountdir)
>>                             > strlen (me->me_mountdir))
>> +                          && (devlist->me->me_mntroot == NULL
>> +                             || me->me_mntroot == NULL
>> +                             || (strlen (devlist->me->me_mntroot)
>> +                                 >= strlen(me->me_mntroot))))
>>                         /* let an entry overmounted on a new device win...  */
>>                         || (! STREQ (devlist->me->me_devname, me->me_devname)
>>                             /* ... but only when matching an existing mnt point,
>>
> 
> This logic looks correct thanks.
> I'll adjust the comment a bit and maybe refactor
> all the conditions to a single nearer_device_root boolean.
> 
> I'll push this later on,
> with the associated gnulib update.

Attached is the refactored patch,
with also lots of adjustments to commit messages.
I'll apply tomorrow.

thanks,
Pádraig.

[df-elide-bind-mounts.patch (text/x-patch, attachment)]

Message #52 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Dave Chiluk <dave.chiluk <at> canonical.com>
To: Pádraig Brady <P <at> draigBrady.com>, 
 Dave Chiluk <chiluk <at> canonical.com>, 21372 <at> debbugs.gnu.org
Subject: Re: bug#21372: [PATCH v4] df: fix prioritize real mounts over bind
 mounts
Date: Tue, 22 Sep 2015 17:03:34 -0500

[Message part 1 (text/plain, inline)]

I like and appreciate the refactor, but I think the variable name
"target_nearer_device_root" for the boolean, follows the existing code
better.  I also fixed the logic to match the new name.

Dave.

On 09/21/2015 07:43 PM, Pádraig Brady wrote:
> On 21/09/15 23:44, Pádraig Brady wrote:
>> On 21/09/15 21:04, Dave Chiluk wrote:
>>> Fixes an issue where bind mounts with shorter mount directories than the
>>> original mount are prioritized when running df.  The root cause of this
>>> is that /proc/self/mountinfo now lists the filesystem device with bind
>>> mounts rather than the source directory. With /etc/mtab the source
>>> device was listed as the originating directory so this was not an issue.
>>>
>>> More information is available here.
>>> https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/1432871
>>> ---
>>>  src/df.c | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/df.c b/src/df.c
>>> index 2e541b9..4a3afa2 100644
>>> --- a/src/df.c
>>> +++ b/src/df.c
>>> @@ -652,9 +652,14 @@ filter_mount_list (bool devices_only)
>>>                else if ((strchr (me->me_devname, '/')
>>>                         /* let "real" devices with '/' in the name win.  */
>>>                          && ! strchr (devlist->me->me_devname, '/'))
>>> -                       /* let a shorter mountdir win.  */
>>> -                       || (strlen (devlist->me->me_mountdir)
>>> +                       /* let a shorter mountdir win. If it's mntroot is */
>>> +                       /* also shorter. i.e. not a bind mount. */
>>> +                       || ((strlen (devlist->me->me_mountdir)
>>>                             > strlen (me->me_mountdir))
>>> +                          && (devlist->me->me_mntroot == NULL
>>> +                             || me->me_mntroot == NULL
>>> +                             || (strlen (devlist->me->me_mntroot)
>>> +                                 >= strlen(me->me_mntroot))))
>>>                         /* let an entry overmounted on a new device win...  */
>>>                         || (! STREQ (devlist->me->me_devname, me->me_devname)
>>>                             /* ... but only when matching an existing mnt point,
>>>
>>
>> This logic looks correct thanks.
>> I'll adjust the comment a bit and maybe refactor
>> all the conditions to a single nearer_device_root boolean.
>>
>> I'll push this later on,
>> with the associated gnulib update.
> 
> Attached is the refactored patch,
> with also lots of adjustments to commit messages.
> I'll apply tomorrow.
> 
> thanks,
> Pádraig.
> 

[0001-df-prioritize-mounts-nearer-the-device-root.patch (text/x-patch, attachment)]

Message #55 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Dave Chiluk <dave.chiluk <at> canonical.com>
To: Pádraig Brady <P <at> draigBrady.com>, 
 21372 <at> debbugs.gnu.org
Subject: Re: bug#21372: [PATCH v4] df: fix prioritize real mounts over bind
 mounts
Date: Tue, 22 Sep 2015 17:28:17 -0500

[Message part 1 (text/plain, inline)]

The patch I just sent out broke existing behavior on non-linux again.
I'm really beginning to hate that if statement.  Anyhow, I fixed that,
and changed it around for readability.

Dave.

On 09/22/2015 05:03 PM, Dave Chiluk wrote:
> I like and appreciate the refactor, but I think the variable name
> "target_nearer_device_root" for the boolean, follows the existing code
> better.  I also fixed the logic to match the new name.
> 
> Dave.
> 
> On 09/21/2015 07:43 PM, Pádraig Brady wrote:
>> On 21/09/15 23:44, Pádraig Brady wrote:
>>> On 21/09/15 21:04, Dave Chiluk wrote:
>>>> Fixes an issue where bind mounts with shorter mount directories than the
>>>> original mount are prioritized when running df.  The root cause of this
>>>> is that /proc/self/mountinfo now lists the filesystem device with bind
>>>> mounts rather than the source directory. With /etc/mtab the source
>>>> device was listed as the originating directory so this was not an issue.
>>>>
>>>> More information is available here.
>>>> https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/1432871
>>>> ---
>>>>  src/df.c | 9 +++++++--
>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/df.c b/src/df.c
>>>> index 2e541b9..4a3afa2 100644
>>>> --- a/src/df.c
>>>> +++ b/src/df.c
>>>> @@ -652,9 +652,14 @@ filter_mount_list (bool devices_only)
>>>>                else if ((strchr (me->me_devname, '/')
>>>>                         /* let "real" devices with '/' in the name win.  */
>>>>                          && ! strchr (devlist->me->me_devname, '/'))
>>>> -                       /* let a shorter mountdir win.  */
>>>> -                       || (strlen (devlist->me->me_mountdir)
>>>> +                       /* let a shorter mountdir win. If it's mntroot is */
>>>> +                       /* also shorter. i.e. not a bind mount. */
>>>> +                       || ((strlen (devlist->me->me_mountdir)
>>>>                             > strlen (me->me_mountdir))
>>>> +                          && (devlist->me->me_mntroot == NULL
>>>> +                             || me->me_mntroot == NULL
>>>> +                             || (strlen (devlist->me->me_mntroot)
>>>> +                                 >= strlen(me->me_mntroot))))
>>>>                         /* let an entry overmounted on a new device win...  */
>>>>                         || (! STREQ (devlist->me->me_devname, me->me_devname)
>>>>                             /* ... but only when matching an existing mnt point,
>>>>
>>>
>>> This logic looks correct thanks.
>>> I'll adjust the comment a bit and maybe refactor
>>> all the conditions to a single nearer_device_root boolean.
>>>
>>> I'll push this later on,
>>> with the associated gnulib update.
>>
>> Attached is the refactored patch,
>> with also lots of adjustments to commit messages.
>> I'll apply tomorrow.
>>
>> thanks,
>> Pádraig.
>>
> 

[0001-df-prioritize-mounts-nearer-the-device-root.patch (text/x-patch, attachment)]

Message #58 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: dave.chiluk <at> canonical.com, 21372 <at> debbugs.gnu.org
Subject: Re: bug#21372: [PATCH v4] df: fix prioritize real mounts over bind
 mounts
Date: Tue, 22 Sep 2015 23:47:47 +0100

On 22/09/15 23:28, Dave Chiluk wrote:
> The patch I just sent out broke existing behavior on non-linux again.
> I'm really beginning to hate that if statement.  Anyhow, I fixed that,
> and changed it around for readability.


   bool target_nearer_device_root = ! (devlist->me->me_mntroot != NULL
                                       && me->me_mntroot != NULL
                                       && (strlen (devlist->me->me_mntroot)
                                           < strlen(me->me_mntroot)));

While this is logically correct it's confusing
as mntroot is related to the source, not the target.
Also the >= implicit in the !< conflicts with "nearer",
where "as_near_or_nearer" would be more accurate.
That's why I kept the ! outside of the boolean.

I'll go with my orig naming unless there are major objections.

thanks,
Pádraig.

Message #61 received at 21372 <at> debbugs.gnu.org (full text, mbox):

From: Dave Chiluk <dave.chiluk <at> canonical.com>
To: Pádraig Brady <P <at> draigBrady.com>, 
 21372 <at> debbugs.gnu.org
Subject: Re: bug#21372: [PATCH v4] df: fix prioritize real mounts over bind
 mounts
Date: Wed, 23 Sep 2015 08:51:12 -0500

On 09/22/2015 05:47 PM, Pádraig Brady wrote:
> On 22/09/15 23:28, Dave Chiluk wrote:
>> The patch I just sent out broke existing behavior on non-linux again.
>> I'm really beginning to hate that if statement.  Anyhow, I fixed that,
>> and changed it around for readability.
> 
> 
>    bool target_nearer_device_root = ! (devlist->me->me_mntroot != NULL
>                                        && me->me_mntroot != NULL
>                                        && (strlen (devlist->me->me_mntroot)
>                                            < strlen(me->me_mntroot)));
> 
> While this is logically correct it's confusing
> as mntroot is related to the source, not the target.
> Also the >= implicit in the !< conflicts with "nearer",
> where "as_near_or_nearer" would be more accurate.
> That's why I kept the ! outside of the boolean.
> 
> I'll go with my orig naming unless there are major objections.
> 
> thanks,
> Pádraig.
> 

I was reading target as referring to the current me that is being
iterated on.  I still don't like the variable "source_below_root". This
implies to me that the source filesystem dir is below the root (i.e.
"/") of the filesystem, which obviously would not be possible.  How
about "longer_mntroot" or "source_longer_mntroot".  That way the later
logic reads
"target_nearer_root && ! source_longer_mntroot"

It's more explicit, and implies which variable within the mount_entry
that we care about.

I think at this point we're really just splitting hairs.

Dave.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.