Re: Updated PedUnit patch

[Andrew Clausen]

On Tue, Jun 28, 2005 at 01:52:35AM +0200, Leslie Patrick Polzer wrote:
> > Well, you have to type "libtoolize".  I guess we could put a shell
> > script in CVS that runs aclocal, libtoolize, autoconf and automake
> > as necessary.
> If you want to go on that track, we might as well remove
> 'configure' and 'Makefile.in' (recursively) from CVS.

Agreed.

> > if (!ped_geometry_test_sector_inside (*range, *sector))
> Why is this necessary?

I see you included correctly it in your patch... it's needed to
test if the user type something a long way outside of the disk,
beyond the error in the unit size.

> > [local variables and line length problems]
> Fixed.
> I tried to put the local variable stuff into a separate function,

I'm not sure what you mean by "local variable stuff".

> but this way we'd end up passing all ped_unit_parse_custom arguments
> to it.


> > This arithmetic looks complicated.  I think a helper function to do
> > rounding is in order.
> Done.

See my comments in your code below.

> Proposal: change the signature of
> 
> ped_unit_format_custom (PedSector sector, PedDevice* dev, PedUnit unit)
> 
> to
> 
> ped_unit_format_custom (PedDevice* dev, PedSector sector, PedUnit unit)

Fine by me.

> +     {
> +         long long unit_size = ped_unit_get_size (dev, unit);
> +         PedSector unit_sectors = ped_div_round_up (unit_size, 
> PED_SECTOR_SIZE);
> +
> +         *sector = ped_div_round_up (num * unit_size, PED_SECTOR_SIZE);
> +         *range = geometry_from_centre_radius (dev, *sector, unit_sectors);
> +
> +         if (!ped_geometry_test_sector_inside (*range, *sector))
> +         {
> +             ped_exception_throw (
> +                     PED_EXCEPTION_ERROR,
> +                     PED_EXCEPTION_CANCEL,
> +                     _("Sector outside of device"));

You also need to free *sector and *range, and set them to NULL...

> @@ -404,3 +399,61 @@ ped_unit_parse_custom (const char* str, 
>       return 1;
>  }
>  
> +
> +static long long
> +get_sectors_per_cent (PedDevice* dev)
> +{
> +    return (long long)( 1 / (100.0 * (dev->length - 1) + 0.5) );
> +}

This isn't what I had in mind.  I was hoping for some kind of generic
division-with-rounding function.  Something like ped_div_round_up(), but
for floats?

Come to think of it, why is there any rounding here at all?

(That is, why is there a 0.5 in there?)

> +long long
> +ped_unit_get_size (PedDevice* dev, PedUnit unit)
> +{
> +    switch (unit)
> +    {
> +     case PED_UNIT_SECTOR:
> +         return PED_SECTOR_SIZE;
> +
> +     case PED_UNIT_BYTE:
> +         return 1;
> +
> +     case PED_UNIT_KILOBYTE:
> +         return PED_KILOBYTE_SIZE;

Any objections to this instead:

> +     case PED_UNIT_SECTOR:   return PED_SECTOR_SIZE;
> +     case PED_UNIT_BYTE:     return 1;
> +     case PED_UNIT_KILOBYTE: return PED_KILOBYTE_SIZE;

> +     case PED_UNIT_CYLINDER:
> +         {
> +             PedSector cyl_size = dev->bios_geom.heads * 
> dev->bios_geom.sectors;
> +             return ( cyl_size * PED_SECTOR_SIZE );
> +         }

Do we need the braces?
Also, this still spills of 80 characters.  Perhaps define cyl_size at
the top?

Cheers,
Andrew