Re: TODO: --total option to df

[Gustavo G. Rondina]

On 8/8/06, Eric Blake <address@hidden> wrote:
> According to Gustavo Rondina on 8/7/2006 8:11 PM:
> > Hello,
> >
> >
> > I was looking the coreutils TODO list and saw that there was still a
> > request
> > for a --total option to df. I have written a patch to implement this
> > feature. It is attached to this message. It worked for me, but I'm
> > no sure if it is fully functional and bug free. This is my first
> > "serious" patch. Any suggestions are welcome.
> 
> Thanks for the contribution.  However, it is not trivial, so to be
> incorporated, you will need to assign copyright to the FSF.  If you
> are willing, we can get that started.

There is no problem for me in assigning copyright to the FSF.  I am OK
with the paperwork.  First let me improve the code and test it for a
while.

> Can you convince your mailer to attach patches with a text MIME
> type?  It makes reviewing a lot easier.

Sorry for that.  Next time I will attach it as plain text.

> 
> I'm not the maintainer, so I can't say whether your patch will be
> accepted, even if you do complete copyright paperwork.  But I did
> notice some problems with just a brief glance.

Thanks for your comments.  I will correct the patch and test it
properly and then I will send a new one to the list.

> > ...

> > +    {
> > +      uintmax_t u100 = total_used.n * 100;
> > +      uintmax_t nonroot_total = total_used.n + total_available.n;
> > +      pct = u100 / nonroot_total + (u100 % nonroot_total != 0);
> > +    }
> > +  else
> > +    {
> > +      /* The calculation cannot be done easily with integer
> > +      arithmetic.  Fall back on floating point.  This can suffer
> > +      from minor rounding errors, but doing it exactly requires
> > +      multiple precision arithmetic, and it's not worth the
> > +      aggravation.  */
> 
> I'm not sure this is the right approach.  Do we really expect systems
> with more than 2^64 bytes of storage?
> 
> > +      double u = total_used.negative
> > +                 ? - (double) - total_used.n
> > +                 : total_used.n;
> 
> GNU Coding standards recommend () here:
>      double u = (total_used.negative
>                  ? - (double) - total_used.n
>                  : total_used.n);
> 
> > +
> > +      double a = total_available.negative
> > +                 ? - (double) - total_available.n
> > +                 : total_available.n;
> > +
> > +      double nonroot_total = u + a;
> > +      if (nonroot_total)
> > +     {
> > +       long int lipct = pct = u * 100 / nonroot_total;
> > +       double ipct = lipct;
> > +
> > +       /* Like `pct = ceil (dpct);', but avoid ceil so that
> > +          the math library needn't be linked.  */
> 
> Why not?  We already link it when using a replacement pow(), and the
> library's version is possibly more efficient.  Not to mention that I'm
> still not convinced you need to use floating point.
> 

This fragments you are referring to are from df itself.  They are
related to the percentage calculation and I wasn't pretty sure if I
should create a function or just copy them.  I think it is best to
write a function because there is too much redundant code in the way
that I did.  Sorry for not mentioning that this was not my code.  I will
do it properly now, with comments.  I will review this calculation code
and try to improve it, it seems possible.

Again, thanks for your advices.

Regards,
Gustavo