Re: Error (typo) in quotacheck(8)

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Tue, 7 Oct 2008 15:54:04 +0200
    From:        Edgar =?iso-8859-1?B?RnXf?= <ef%math.uni-bonn.de@localhost>
    Message-ID:  <20081007135403.GA8024%gumme.math.uni-bonn.de@localhost>

  | Any quota experts round here?

Not really me any more, but ...

  | where the marked "blocks" should clearly be "inodes".

Yes.

  | It would also seem more logically to me to reverse the order of the last
  | two assignments to keep the blocks/inodes order consistent.

Maybe, but changing that changes nothing, but makes things different than
they were, so I don't think that's worth "fixing" now.

  | While investigating that code, I had quite some trouble in finding out what
  | it was precisely meant to do.

It's meant to adjust recorded usages to match reality ... but I suspect
you knew that.  Ideally it should not affect the grace time limits,
unless its adjustments cause the usage to go above the soft limit,
in which case the time should start now (being the best estimate of when
it really happened that we have.)

  | My first impression was that quotacheck(8) wanted to avoid fiddling with the
  | time fields and rather leave that to the kernel,

I'd need to look at that more, when I last dealt with that code, time fields
didn't exist - it was warning counters instead (3 warnings and you're dead,
or something like that...)   Times are more rational, and harder to get
right...

  | While there, I found that in both setquota() and setuse(), the ndq variables
  | seem to be redundant.

You mean that the code could just use dq instead?  (this is in
sys/ufs/ufs/ufs_quota.c right?)

Yes, it probably could, now.   When it was written, dq was register...
and it isn't legal to apply & to a variable of register storage class.

Now all the register declarations are gone, and the compiler left to
work that out itself, it might be better to remove ndq - it might still
be worth looking at what code gcc gnerates if that happens, as it is now,
gcc ought to be able to deduce that dq should live in a register (on
architectures where that is possible) and never touch ram, but after
&dq is used, that would require more complex analysis of the code,
which I have no idea if gcc does or not.

  | Also, in chkdqchg, ncurblocks is (signed) long, while dq_curblocks is 
u_int32_t.
  | Is it possible for the value to become negative?

One would hope not, but it is possible (ncurblocks) as it gets
calculated by applying a delta (+ or -) and so, could become < 0
if the delta is larger than the current count.

I suspect bad things would happen if that ever occurred, but as
the code is written, I think the data types are correct.

  | Aditionally, the wording of man quotactl(2), Q_SETUSE, reads "Set disk usage
  | limits", where, at least to me as a non-native speaker, the "limits" seems
  | both superflous and confusing.

It probably should be "values" or "counters" instead.  That one may very
well be an original error from my time...

  | And, in both dqget() and setquota(), I don't quite understand the special
  | handling of dq_id being zero when manipulating the tine values.

zero is root, root gets no quotas by definition, so calculating grace
times would be fairly meaningless.   At least I expect that's what it
is doing, given that I have never looked at the time part of quotas.
Certainly root never accumulated warnings, whatever quotas might have
been set, and I suspect this is just the same thing in the new context.

  | Comments, anyone?

Perhaps someone else who has been near the quota code more recently than I
will give better answers.

kre

ps: you should probably file a PR on the major problem to make sure it
doesn't get forgotten.