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

Tue Apr 20 03:50:14 UTC 2021

On Tue, 2021-04-20 at 12:26 +1000, Allan McRae wrote:
> 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.
> 

Yes, FSSTATSTYPE is used in alpm_mountpoint_t
(lib/libalpm/diskspace.h). This *could* be remedied if needed.

> 
> 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
> 

This is likely what would be appropriate, but perhaps with an else or
some preprocessor check for alpm_mountpoint_t to prevent a compilation
error if nothing in that block applies.

> 
> 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.

I can submit a v3 patch for this if you'd like. I wasn't entirely happy
with the patches I provided since they felt "hacky" to me, but they did
work on both FreeBSD and Arch Linux, so I felt it was at least a start.

Mark