[pacman-dev] [PATCH 1/5] Fix check for FSSTATSTYPE

[Allan McRae]

On 20/4/21 12:25 am, Mark Weiman wrote:
> On Mon, 2021-04-19 at 17:01 +1000, Allan McRae wrote:
>> On 17/4/21 1:45 pm, Mark Weiman wrote:
>>> On FreeBSD, both `struct statvfs` and `struct statfs` satisfy the
>>> conditions where the `f_flag` and `f_flags` fields are present in
>>> both
>>> respectively.
>>>
>>> This patch accomplishes a couple of things:
>>>
>>>   1. Adds a check for `f_mntonname` in both structs and makes a
>>> decision
>>>      to choose statfs if the field is not present in statvfs, but
>>> is in
>>>      statfs.
>>>   2. Corrects a small error where the values of those checks are
>>> just
>>>      checked for their presence and not whether its true or false.
>>>
>>> Signed-off-by: Mark Weiman <mark.weiman at markzz.com>
>>
>> Thanks for the BSD fixes!  It is clear that we do not test there very
>> often...
>>
>>> ---
>>>  meson.build | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 579ff2ed..483c4fbd 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -154,6 +154,9 @@ foreach member : [
>>>      ['struct statvfs', 'f_flag', '''#include <sys/statvfs.h>'''],
>>>      ['struct statfs', 'f_flags', '''#include <sys/param.h>
>>>                                      #include <sys/mount.h>'''],
>>> +    ['struct statvfs', 'f_mntonname', '''#include
>>> <sys/statvfs.h>'''],
>>> +    ['struct statfs', 'f_mntonname', '''#include <sys/param.h>
>>> +                                        #include
>>> <sys/mount.h>'''],
>>
>> OK
>>
>>>    ]
>>>    have = cc.has_member(member[0], member[1], prefix : member[2])
>>>    conf.set('HAVE_' + '_'.join([member[0],
>>> member[1]]).underscorify().to_upper(), have)
>>> @@ -174,9 +177,13 @@ foreach type : [
>>>    endif
>>>  endforeach
>>>  
>>> -if conf.has('HAVE_STRUCT_STATVFS_F_FLAG')
>>> +if conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false)
>>>    conf.set('FSSTATSTYPE', 'struct statvfs')
>>
>> Should this be "if conf.get( ... true)", or "if not conf.get(" ?
>>
> 
> No, the singature to conf.get() is the key you're checking first, then
> the default value you want if it doesn't exist. In this case, we want
> false to be the default.
> 

Thanks - it was suprisingly hard to find documentation for this...

>>> -elif conf.has('HAVE_STRUCT_STATFS_F_FLAGS')
>>> +elif conf.get('HAVE_STRUCT_STATFS_F_FLAGS', false)
>>> +  conf.set('FSSTATSTYPE', 'struct statfs')
>>> +endif
>>> +
>>> +if not conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false) and
>>> conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false)
>>>    conf.set('FSSTATSTYPE', 'struct statfs')
>>>  endif
>>>  
>>
>> This just seems wrong to me.  We require f_mntonname for either
>> statfs
>> or statvfs, so just test for this when making the original assignment
>> to
>> FSSTATSTYPE.  It appears the f_flag(s) part is not actually useful in
>> determining which of statfs or statvfs we should use.
>>
>> (As an aside, is there some statvfs without the f_mntonname field?)
>>
> 
> I didn't quite know how to address this. On Arch Linux, it appears that
> f_mntonname doesn't exist at all, but on FreeBSD, it exists in statfs,
> but not statvfs. And yes, statvfs exists without f_mntonname.
> 
> I figured getting it to build and choosing the correct one on both
> Linux and FreeBSD was what was important here and doing some homework
> later on what exactly to do. On Linux, its statvfs (I think), but on
> FreeBSD, that's statfs.
> 

Ah...  f_mntonname is only used with getmntinfo.  So BSD land.  I had
forgotten the pain of implementing this...

So now I am on the right page, lets go back and look at the patch!
Here is the bit I am looking at:


-if conf.has('HAVE_STRUCT_STATVFS_F_FLAG')
+if conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false)
   conf.set('FSSTATSTYPE', 'struct statvfs')
-elif conf.has('HAVE_STRUCT_STATFS_F_FLAGS')
+elif conf.get('HAVE_STRUCT_STATFS_F_FLAGS', false)
+  conf.set('FSSTATSTYPE', 'struct statfs')
+endif
+
+if not conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false) and
conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false)
   conf.set('FSSTATSTYPE', 'struct statfs')
 endif


My reading is this is about equivalent to:

if conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false)
  conf.set('FSSTATSTYPE', 'struct statvfs')
elif conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false)
  conf.set('FSSTATSTYPE', 'struct statfs')
elif conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false)
  conf.set('FSSTATSTYPE', 'struct statvfs')
elif conf.get('HAVE_STRUCT_STATFS_F_FLAGS', false)
  conf.set('FSSTATSTYPE', 'struct statfs')
endif

Do we just fail on compile if FSSTATSTYPE is undefined?  That seems not
ideal.


This really does not correspond to the checks in the code!

HAVE_GETMNTENT  -> statvfs
HAVE_GETMNTINFO_STATVFS + HAVE_STRUCT_STATVFS_F_FLAG -> statvfs
HAVE_GETMNTINFO_STATFS + HAVE_STRUCT_STATFS_F_FLAGS -> statfs

I think we want to match the code (and fix any of the code that is wrong):

if conf.get('HAVE_GETMNTENT', false) and
conf.get('HAVE_STRUCT_STATVFS_F_FLAG', false)
  conf.set('FSSTATSTYPE', 'struct statvfs')
elif conf.get('HAVE_GETMNTINFO', false) and
conf.get('HAVE_STRUCT_STATVFS_F_MNTONNAME', false)
  conf.set('FSSTATSTYPE', 'struct statvfs')
elif conf.get('HAVE_GETMNTINFO', false) and
conf.get('HAVE_STRUCT_STATFS_F_MNTONNAME', false)
  conf.set('FSSTATSTYPE', 'struct statfs')
endif


Looks like the F_FLAG/F_FLAGS check within the getmntinfo are only to
see if we can detect read-only filesystems, and are not a requirement.