Re: GRUB2: *BSD and more patch

[Marco Gerards]

Sergey Matveychuk <address@hidden> writes:

> Here is a third version.

Great!

After reading your patch I found some other problems, sorry for not
looking good enough the first time.  Most problems I found are minor
nitpicks.

If it is ok for Okuji I can make the changes I proposed (and have a
quick look at the changelog entry) and commit the patch.

> -- 
> Sem.
> diff -ruNp grub2/ChangeLog grub2.test/ChangeLog
> --- grub2/ChangeLog   Fri Mar 19 23:55:21 2004
> +++ grub2.test/ChangeLog      Sat Mar 20 01:38:47 2004
> @@ -1,3 +1,19 @@
> +2004-03-15  Sergey Matveychuk  <address@hidden>
> +
> +     * util/i386/pc/biosdisk.c: Added headers for BSD.

Please say exactly what header files you are including.

> +     (pupa_util_biosdisk_get_pupa_dev): Do not call

Please end a sentence with a ".".

> +     make_device_name(drive,-1-1) if not a block device.
> +     * util/i386/pc/getroot.c: Update copyright.

No need to notice the copyright update when changing years.  It is
only required when changing the copyright holders (Correct me if I am
wrong).

> +     (find_root_device): Do not check for a block device.
> +     * util/misc.c: Include malloc.h only if it's needed.
> +     (pupa_memalign): Change memalign() with malloc() where unavailable.
> +     * util/pupa-emu.c: Include malloc.h only if it's needed.
> +     (main): Move pupa_util_biosdisk_init() above pupa_guess_root_device().

I do not see the configure.ac changes in the changelog entry.  Same
for including config.h in misc.c.

This is only what I noticed at first.  Please make sure the changelog
entries are complete.

> -/* Define it to \"addr32\" or \"addr32;\" to make GAS happy */
> +/* Define it to "addr32" or "addr32;" to make GAS happy */

Why are you doing this?  I assume this is what the autotools
generated?

> -/* Define it to \"data32\" or \"data32;\" to make GAS happy */
> +/* Define it to "data32" or "data32;" to make GAS happy */

Same here.

> +AC_CHECK_HEADER(malloc.h)
> +AC_CHECK_FUNC(memalign)

This is what I meant, great!

> +#if !defined(__FreeBSD__)
>    if (! S_ISBLK (st.st_mode))
>      return make_device_name (drive, -1, -1);
> +#endif

make_device_name does not have to be called at all?

> -#elif defined(__GNU__)
> -  /* GNU uses "/dev/[hs]d[0-9]+(s[0-9]+[a-z]?)?".  */
> +#elif defined(__GNU__) || defined(__FreeBSD__) || defined(__NetBSD__) || 
> defined(__OpenBSD__)
> +  /* GNU uses "/dev/[ahsw]d[0-9]+(s[0-9]+[a-z]?)?".  */

Is this comment correct?  It says something about GNU, now you changed
the regexp. so it matches *BSD as well.

Thanks,
Marco