Re: [PATCH] biosdisk, getroot for Cygwin

[Christian Franke]

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