[PATCH] missleading function name

[PATCH] missleading function name

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 336 bytes --]


IMHO the name of this function is highly misleading.  It makes one think
it just checks something, but it actually modifies our variables.  I think
"adjust" would fit better than "check".

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

[-- Attachment #2: check_range.diff --]
[-- Type: text/x-diff, Size: 1319 bytes --]

diff -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/kern/disk.c ./kern/disk.c
--- ../grub2/kern/disk.c	2008-02-08 13:22:51.000000000 +0100
+++ ./kern/disk.c	2008-06-04 16:15:24.000000000 +0200
@@ -323,7 +323,7 @@ grub_disk_close (grub_disk_t disk)
 }
 
 static grub_err_t
-grub_disk_check_range (grub_disk_t disk, grub_disk_addr_t *sector,
+grub_disk_adjust_range (grub_disk_t disk, grub_disk_addr_t *sector,
 		       grub_off_t *offset, grub_size_t size)
 {
   *sector += *offset >> GRUB_DISK_SECTOR_BITS;
@@ -364,7 +364,7 @@ grub_disk_read (grub_disk_t disk, grub_d
   grub_dprintf ("disk", "Reading `%s'...\n", disk->name);
   
   /* First of all, check if the region is within the disk.  */
-  if (grub_disk_check_range (disk, &sector, &offset, size) != GRUB_ERR_NONE)
+  if (grub_disk_adjust_range (disk, &sector, &offset, size) != GRUB_ERR_NONE)
     {
       grub_error_push ();
       grub_dprintf ("disk", "Read out of range: sector 0x%llx (%s).\n",
@@ -502,7 +502,7 @@ grub_disk_write (grub_disk_t disk, grub_
   
   grub_dprintf ("disk", "Writing `%s'...\n", disk->name);
 
-  if (grub_disk_check_range (disk, &sector, &offset, size) != GRUB_ERR_NONE)
+  if (grub_disk_adjust_range (disk, &sector, &offset, size) != GRUB_ERR_NONE)
     return -1;
 
   real_offset = offset;

Re: [PATCH] missleading function name

[Pavel Roskin]

On Thu, 2008-06-05 at 01:30 +0200, Robert Millan wrote:
> IMHO the name of this function is highly misleading.  It makes one think
> it just checks something, but it actually modifies our variables.  I think
> "adjust" would fit better than "check".

But nothing can be better that a good old-fashioned connect before the
function.  It looks like it does several things:

1) it makes sectors disk relative from partition relative (I guess
that's what grub-emu forgets)

2) it normalizes offset to be less than the sector size

3) it verifies that the range is inside the partition.

-- 
Regards,
Pavel Roskin

Re: [PATCH] missleading function name

[Robert Millan]

On Wed, Jun 04, 2008 at 10:08:28PM -0400, Pavel Roskin wrote:
> On Thu, 2008-06-05 at 01:30 +0200, Robert Millan wrote:
> > IMHO the name of this function is highly misleading.  It makes one think
> > it just checks something, but it actually modifies our variables.  I think
> > "adjust" would fit better than "check".
> 
> But nothing can be better that a good old-fashioned connect before the
> function.

What do you mean?

> It looks like it does several things:
> 
> 1) it makes sectors disk relative from partition relative (I guess
> that's what grub-emu forgets)

Ah, that explains it.  Maybe the code in util/biosdisk.c comes from a time
in which kern/disk.c didn't do this.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] missleading function name

[Pavel Roskin]

On Thu, 2008-06-05 at 23:11 +0200, Robert Millan wrote:
> On Wed, Jun 04, 2008 at 10:08:28PM -0400, Pavel Roskin wrote:
> > On Thu, 2008-06-05 at 01:30 +0200, Robert Millan wrote:
> > > IMHO the name of this function is highly misleading.  It makes one think
> > > it just checks something, but it actually modifies our variables.  I think
> > > "adjust" would fit better than "check".
> > 
> > But nothing can be better that a good old-fashioned connect before the
> > function.
> 
> What do you mean?

I mean "comment", sorry.

-- 
Regards,
Pavel Roskin

Re: [PATCH] missleading function name

[Robert Millan]

On Wed, Jun 04, 2008 at 10:08:28PM -0400, Pavel Roskin wrote:
> On Thu, 2008-06-05 at 01:30 +0200, Robert Millan wrote:
> > IMHO the name of this function is highly misleading.  It makes one think
> > it just checks something, but it actually modifies our variables.  I think
> > "adjust" would fit better than "check".
> 
> But nothing can be better that a good old-fashioned connect before the
> function.  It looks like it does several things:
> 
> 1) it makes sectors disk relative from partition relative (I guess
> that's what grub-emu forgets)
> 
> 2) it normalizes offset to be less than the sector size
> 
> 3) it verifies that the range is inside the partition.

Thanks, I made a comment for it based on your description.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)