Skip to contentSkip to conversation

missleading function name

PATCH In review Robert Millan 2 participants 5 messages Jun 4, 2008 – Jun 8, 2008

lore
Share & tools
lore

Robert Millan

author
#0 lore raw

PATCH 1/1 missleading function name

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".

Signature · 5 lines
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 /.)
kern/disk.c renamed from grub2/kern/disk.c +3 -3
diff -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/kern/disk.c ./kern/disk.c
@@ -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;
#1 lore raw
Quoted 3 lines · Robert Millan

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.

Signature · 2 lines
Regards,
Pavel Roskin
author
#2 lore raw
Quoted 7 lines · Pavel Roskin
Quoted 3 lines · Robert Millan

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?

Quoted 4 lines

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.

Signature · 5 lines
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 /.)
#3 lore raw
Quoted 10 lines · Robert Millan
Quoted 7 lines · Pavel Roskin
Quoted 3 lines · Robert Millan

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.

Signature · 2 lines
Regards,
Pavel Roskin
author
#4 lore raw
In reply to Pavel Roskin
Quoted 8 lines · Pavel Roskin
Quoted 3 lines · Robert Millan

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:

Quoted 6 lines

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.

Signature · 5 lines
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 /.)