Re: [PATCH] biosdisk, getroot for Cygwin

[Christian Franke]

Christian Franke wrote:
> Robert Millan wrote:
> > On Wed, May 07, 2008 at 10:42:23PM +0200, Christian Franke wrote:
> > > +#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.


Probably more readable and extensible - Use early returns:

#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
    LVM.  */
 os_dev = find_root_device ("/dev/mapper", st.st_dev);
 if (os_dev)
   return os_dev;

 /* The same applies to /dev/evms directory (for EVMS volumes).  */
 os_dev = find_root_device ("/dev/evms", st.st_dev);
 if (os_dev)
   return os_dev;
#endif /* __linux__ */

#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__ */

 return os_dev;
}


Christian