Re: [PATCH] biosdisk / open_device() messing up offsets

[Pavel Roskin <proski@gnu.org>]

On Sun, 2008-06-08 at 14:44 +0800, Bean wrote:
> On Sun, Jun 8, 2008 at 2:00 PM, Pavel Roskin <proski@gnu.org> wrote:
> > On Sat, 2008-06-07 at 15:48 +0800, Bean wrote:
> >
> >> I believe the problem is with hexdump. It open the device with
> >> grub_disk_open, which returns the disk object related to the beginning
> >> of partition. However, it read it using disk->dev->read, which is a
> >> low level api that use absolute address. It should be using
> >> grub_disk_read instead.
> >
> > Nice catch!  Indeed, hexdump has special handling for the whole
> > partitions.  And it actually tries to satisfy the low level API by
> > converting offset to the sector number and skipping the remainder.  I
> > guess it could be simplified if grub_disk_read() is used.
> 
> The reason I add device support for hexdump is to debug the nand
> driver. I need to go around the disk cache and call the underlying
> disk driver directly, so I use disk->dev->read. For (nand), there is
> just one partition, so I didn't notice the problem then.

Here's the patch.  Everything seems to be OK.  "--skip=N" is not
recognized, but it's something in the option parsing code.  "-s N" is
working.

Please feel free to apply.

diff --git a/commands/hexdump.c b/commands/hexdump.c
index 6d97fe4..e459b88 100644
--- a/commands/hexdump.c
+++ b/commands/hexdump.c
@@ -99,35 +99,27 @@ grub_cmd_hexdump (struct grub_arg_list *state, int argc, char **args)
   else if ((args[0][0] == '(') && (args[0][namelen - 1] == ')'))
     {
       grub_disk_t disk;
-      grub_disk_addr_t sector;
-      grub_size_t ofs;
 
       args[0][namelen - 1] = 0;
       disk = grub_disk_open (&args[0][1]);
       if (! disk)
         return 0;
 
-      sector = (skip >> (GRUB_DISK_SECTOR_BITS + 2)) * 4;
-      ofs = skip & (GRUB_DISK_SECTOR_SIZE * 4 - 1);
       while (length)
         {
-          grub_size_t len, n;
+          grub_size_t len;
 
           len = length;
-          if (ofs + len > sizeof (buf))
-            len = sizeof (buf) - ofs;
+          if (len > sizeof (buf))
+            len = sizeof (buf);
 
-          n = ((ofs + len + GRUB_DISK_SECTOR_SIZE - 1)
-               >> GRUB_DISK_SECTOR_BITS);
-          if (disk->dev->read (disk, sector, n, buf))
+          if (grub_disk_read (disk, 0, skip, len, buf))
             break;
 
-          hexdump (skip, &buf[ofs], len);
+          hexdump (skip, buf, len);
 
-          ofs = 0;
           skip += len;
           length -= len;
-          sector += 4;
         }
 
       grub_disk_close (disk);


-- 
Regards,
Pavel Roskin