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
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
'''], ['struct statfs', 'f_flags', '''#include #include '''], + ['struct statvfs', 'f_mntonname', '''#include '''], + ['struct statfs', 'f_mntonname', '''#include + #include '''], 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