| Message ID | 20210417034524.204755-2-mark.weiman@markzz.com |
|---|---|
| State | Under Review |
| Headers | show |
| Series | Various fixes for FreeBSD (and perhaps others) | expand |
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@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(" ? > -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 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. Allan
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@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
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@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.
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@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
On 20/4/21 1:50 pm, Mark Weiman wrote: > 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. That would be great. I think this one was too hacky to put in as is. Allan
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>'''], ] 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') -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
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@markzz.com> --- meson.build | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)