Re: GRUB2: *BSD and more patch

[Yoshinori K. Okuji]

On Thursday 18 March 2004 09:15, Sergey Matveychuk wrote:
> diff -ruN grub2/util/i386/pc/biosdisk.c 
grub2.devel/util/i386/pc/biosdisk.c
> --- grub2/util/i386/pc/biosdisk.c       Tue Mar 16 02:24:00 2004
> +++ grub2.devel/util/i386/pc/biosdisk.c Tue Mar 16 04:25:21 2004
> @@ -37,6 +37,17 @@
>  #include <errno.h>
>  #include <limits.h>
>  
> +#if defined(__FreeBSD__) || defined(__NetBSD__) || 
defined(__OpenBSD__)
> +# include <sys/ioctl.h>                /* ioctl */
> +# include <sys/disklabel.h>
> +#if defined(__FreeBSD__)
> +#include <sys/param.h>
> +#if __FreeBSD_version >= 500040
> +#include <sys/disk.h>
> +#endif
> +#endif
> +#endif /* __FreeBSD__ || __NetBSD__ || __OpenBSD__ */
> +

This part should be indented appropriately, according to the GNU Coding 
Standards. Please take a look at how I applied your patch to GRUB 
legacy.

> +#if !defined(__FreeBSD__)
> +    if (fstat (fd, &st) < 0 || ! S_ISBLK (st.st_mode))
> +#else
> +    if (fstat (fd, &st) < 0)
> +#endif

I think this part can omit the check using S_ISBLK regardless of the OS.

> +#if !defined(__FreeBSD__) || __FreeBSD_version < 500040
> +    if (ioctl (fd, DIOCGDINFO, &hdg))
> +      goto fail;
> +    
> +    disk->total_sectors = hdg.d_secperunit;
> +#else
> +    u_int    u, secsize;
> +    off_t    mediasize;
> +
> +    if(ioctl(fd, DIOCGSECTORSIZE, &secsize) != 0)
> +       secsize = 512;
> +
> +    if (ioctl(fd, DIOCGMEDIASIZE, &mediasize) != 0)
> +       goto fail;
> +
> +    hdg.d_secperunit = mediasize / secsize;
> +
> +    if (ioctl(fd, DIOCGFWSECTORS, &u) == 0)
> +           hdg.d_nsectors = u;
> +    else
> +           hdg.d_nsectors = 63;
> +    if (ioctl(fd, DIOCGFWHEADS, &u) == 0)
> +           hdg.d_ntracks = u;
> +    else if (hdg.d_secperunit <= 63*1*1024)
> +           hdg.d_ntracks = 1;
> +    else if (hdg.d_secperunit <= 63*16*1024)
> +           hdg.d_ntracks = 16;
> +    else
> +           hdg.d_ntracks = 255;
> +    hdg.d_secpercyl = hdg.d_ntracks * hdg.d_nsectors;
> +    hdg.d_ncylinders = hdg.d_secperunit / hdg.d_secpercyl;
> +
> +    disk->total_sectors = hdg.d_secperunit;
> +
> +#endif

I prefer not using hdg here. Again, look at my way in GRUB legacy.

> diff -ruN grub2/util/i386/pc/getroot.c 
grub2.devel/util/i386/pc/getroot.c
> --- grub2/util/i386/pc/getroot.c        Sun Mar 14 05:44:20 2004
> +++ grub2.devel/util/i386/pc/getroot.c  Tue Mar 16 03:20:05 2004
> @@ -1,7 +1,7 @@
>  /* getroot.c - Get root device */
>  /*
>   *  PUPA  --  Preliminary Universal Programming Architecture for GRUB
> - *  Copyright (C) 1999,2000,2001,2002,2003  Free Software Foundation, 
Inc.
> + *  Copyright (C) 1999,2000,2001,2002,2003,2004  Free Software 
Foundation, Inc.
>   *
>   *  PUPA is free software; you can redistribute it and/or modify
>   *  it under the terms of the GNU General Public License as published 
by
> @@ -177,8 +177,12 @@
>               return res;
>             }
>         }
> -
> +      /* On last FreeBSDs there are no block devices */
> +#if !defined(__FreeBSD__)
>        if (S_ISBLK (st.st_mode) && st.st_rdev == dev)
> +#else
> +      if (st.st_rdev == dev)
> +#endif

I think this can be generic, too. The check for a block device is not 
very important.

> diff -ruN grub2/util/misc.c grub2.devel/util/misc.c
> --- grub2/util/misc.c   Sun Mar 14 05:44:20 2004
> +++ grub2.devel/util/misc.c     Mon Mar 15 20:14:40 2004
> @@ -24,7 +24,9 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <sys/times.h>
> +#if !defined(__FreeBSD__)
>  #include <malloc.h>
> +#endif

I think this should be probed by the configure script.

> +#if !defined(__FreeBSD__)
>    p = memalign (align, size);
> +#else
> +  p = malloc(size);
> +#endif

Hmm, FreeBSD doesn't have memalign? Since memalign is different from 
malloc, it is not good to just replace memalign with malloc.

BTW, have you signed a copyright assignment for GRUB? I'm sorry, but I 
don't remember well. If not, I'd like you to do that.

Regards,
Okuji