On Tue, Dec 17, 2019 at 03:20:12PM +0000, Al Viro wrote:
> On Tue, Dec 17, 2019 at 01:19:56PM +0100, Lukas Czerner wrote:
> > On Tue, Dec 17, 2019 at 12:44:19AM +0000, Al Viro wrote:
> > > On Wed, Nov 06, 2019 at 11:14:42AM +0100, Lukas Czerner wrote:
> > > > + fsparam_string_empty
> > > > + ("usrjquota", Opt_usrjquota),
> > > > + fsparam_string_empty
> > > > + ("grpjquota", Opt_grpjquota),
> > >
> > > Umm... That makes ...,usrjquota,... equivalent to ...,usrjquota=,...
> > > unless I'm misreading the series. Different from mainline, right?
> >
> > Unfortunatelly yes, I do not think this is a problem, but if you have a
> > solution within the new mount api framework I am happy to use it.
>
> Er... Dump the fsparam_string_empty() use and instead of your
> + if (token == Opt_usrjquota) {
> + if (result.negated)
> + return clear_qf_name(sb, USRQUOTA);
> + else
> + return set_qf_name(sb, USRQUOTA, param);
> do
> + if (token == Opt_usrjquota) {
> + if (!param->string[0])
> + return clear_qf_name(sb, USRQUOTA);
> + else
> + return set_qf_name(sb, USRQUOTA, param);
> with the same for grpjquota?
Ah right, it's been a while I guess I didn't realize that it will accept
usrjquota as well as usrjquota=
It makes sense to check the string directly, thanks.
>
> > > > + fsparam_bool ("barrier", Opt_barrier),
> > > > + fsparam_flag ("nobarrier", Opt_nobarrier),
> > >
> > > That's even more interesting. Current mainline:
> > > barrier OK, sets EXT4_MOUNT_BARRIER
> > > barrier=0 OK, sets EXT4_MOUNT_BARRIER
> > > barrier=42 OK, sets EXT4_MOUNT_BARRIER
> > > barrier=yes error
> > > barrier=no error
> > > nobarrier OK, clears EXT4_MOUNT_BARRIER
> > > Unless I'm misreading your series, you get
> > > barrier error
> >
> > Not really, this seems to be working as expected. Assuming that this
> > didn't change since 5.4.0-rc6. I does make sense to me that specifying
> > bool type parameter without any options would express "true".
> >
> >
> > > barrier=0 OK, sets EXT4_MOUNT_BARRIER
> >
> >
> > > barrier=42 error
> > > barrier=yes OK, sets EXT4_MOUNT_BARRIER
> > > barrier=no OK, sets EXT4_MOUNT_BARRIER
> >
> > Those three are different, just because of how param_book() work. I do
> > not really see a problem with it, but if we want to keep it exactly the
> > same as current mainline it would be difficult with how the current api
> > works. Any suggestions ?
>
> If fsparam_bool() doesn't do the right thing, perhaps it shouldn't be
> used in the first place? Or changed, for that matter - it's not as if
> we had many users of that thing and the only in-tree one is definitely
> breaking userland ABI.
>
> In case of ext4, after rereading that code (and getting some sleep) the
> current behaviour is, AFAICS to accept barrier | nobarrier | barrier=<number>
> with the last case being equialent to nobarrier when number is 0 and barrier
> in all other cases. Is that an accurate description?
Yeah, the documentation says
barrier=0 / barrier=1 / barrier / nobarrier
but we do accept any number from 0 to 2147483647
>
> If so, I would prefer
> fsparam_flag_no("barrier", Opt_barrier), // barrier | nobarrier
> fsparam_u32("barrier", Opt_barrier), // barrier=<number>
> as the solution, with fs_parse() having been taught to allow argument-bearing
> and argument-less options with the same name, picking the right one. That
> way Opt_nobarrier gets removed as well...
>
> I'll push a branch with that stuff later today; will post when it's out...
That would be great, thanks.
-Lukas
