Re: [PATCH] biosdisk, getroot for Cygwin

[Christian Franke <Christian.Franke@t-online.de>]

Robert Millan wrote:
> 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?
>
>   

There are 4 boot sector types, a clean declaration would require a union 
of 4 structs. IMO not worth the effort.


>> +  /* 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).
>
>   

13 "paranoia" bytes added to the required size :-)


>> +#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?
>
>   

An alternative would be to move the #ifndef __CYGWIN__ behind the #ifdef 
__linux__ block:

#ifdef __linux__
...
  os_dev = find_root_device ("/dev/mapper", st.st_dev);
...
  if (! os_dev)
#endif
#ifndef __CYGWIN__
    {
      /* This might be truly slow, but is there any better way?  */
      os_dev = find_root_device ("/dev", st.st_dev);
    }

#else /* __CYGWIN__ */
  /* Cygwin specific function.  */
  os_dev = find_cygwin_root_device (dir, st.st_dev);

#endif /* __CYGWIN__ */


I'm not sure whether this would be more readable.


>> +#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.
>
>   

Yes.

Christian