Re: [PATCH] biosdisk, getroot for Cygwin

[Robert Millan <rmh@aybabtu.com>]

On Wed, May 07, 2008 at 10:42:23PM +0200, Christian Franke wrote:
> +
> +  /* Check signature.  */
> +  if (!(buf[0x1fe] == 0x55 && buf[0x1ff] == 0xaa))
> +    return ~0;
> +
> +  /* Serial number offset depends on boot sector type.  */
> +  if (mbr)
> +    n = 0x1b8;
> +  else if (memcmp (buf + 0x03, "NTFS", 4) == 0)
> +    n = 0x048;
> +  else if (memcmp (buf + 0x52, "FAT32", 5) == 0)
> +    n = 0x043;
> +  else if (memcmp (buf + 0x36, "FAT", 3) == 0)
> +    n = 0x027;

Did you consider using a struct here?

> +  /* Cygwin returns the partition serial number in stat.st_dev.
> +     This is never identical to the device number of the emulated
> +     /dev/sdXN device, so above find_root_device () does not work.
> +     Search the partion with the same serial in boot sector instead.  */
> +  char devpath[sizeof ("/dev/sda15") + 13];

Where does this 13 come from?  Would be nice to make it explicit (e.g.
sizeof(something) or so).

> +#ifndef __CYGWIN__
>  #ifdef __linux__
>    /* We first try to find the device in the /dev/mapper directory.  If
>       we don't do this, we get useless device names like /dev/dm-0 for
> @@ -242,12 +373,19 @@ grub_guess_root_device (const char *dir)
>        os_dev = find_root_device ("/dev", st.st_dev);
>      }
>  
> +#else
> +  /* Cygwin specific function.  */
> +  os_dev = find_cygwin_root_device (dir, st.st_dev);
> +
> +#endif /* __CYGWIN__ */

The logic here seems a bit over-complicated.  Surely whenever you have __linux__
you don't have __CYGWIN__.  Are you sure it can't be simplified?

> +#ifndef __CYGWIN__
>    /* Check for LVM.  */
>    if (!strncmp (os_dev, "/dev/mapper/", 12))
>      return GRUB_DEV_ABSTRACTION_LVM;
> @@ -255,6 +393,7 @@ grub_util_get_dev_abstraction (const char *os_dev)
>    /* Check for RAID.  */
>    if (!strncmp (os_dev, "/dev/md", 7))
>      return GRUB_DEV_ABSTRACTION_RAID;
> +#endif /* !__CYGWIN__ */

I think what we're missing here is "ifdef __linux__" since both LVM and
dmRAID are Linux-specific.

-- 
Robert Millan

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