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

[Mark Weiman]

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.

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

> 
> I also found this looking at the mountpoint code. It appears that the
> f_flag(s) only gives us access to whether the mountpoint is read-
> only.
> That also appears broken since the switch to meson (or earlier!):
> 
> e.g.
> lib/libalpm/diskspace.c;
> 
> #if defined(HAVE_GETMNTINFO_STATVFS) &&
> defined(HAVE_STRUCT_STATVFS_F_FLAG)
> 
> I don't see that first define being made anywhere.
> 

I'll look at it further later.

> Allan


Mark