Re: FreeBSD port work

[Leif Lindholm]

Hi Eric,

On Fri, May 31, 2019 at 07:22:26AM -0400, Eric McCorkle wrote:
> I've been doing some work to refurbish the FreeBSD port, and I may take
> over maintaining it.  I also want to modify GRUB to pass GELI keys into
> the kernel using the newer keybuf mechanism, but that's later (I posted
> about this a while back).
> 
> Attached are some quick-and-dirty patches I created for getting the
> updated port to build in the FreeBSD ports tree.  Note: there's a
> "force-label" flag in the grub-install utility which is currently
> unimplemented; the old version used a kind of script, but the newer one
> is a C file, and I need to do more investigation to figure out how to
> implement it in the C file.
> 
> If people think any of these patches should go into the main tree, let
> me know what I need to do, or how I need to polish them up first and
> I'll get it done

Well, feels a bit silly to deal with the other main BSDs and not
FreeBSD... (especially as it looks like it was supported at some point
in the past).

Most of the below is out of my area of knowledge, but the patch seems
to have some whitespace-only changes (pointed out individually below)
that should probably be filtered out separately, or dropped.

There are probably also some uses of the __FreeBSD__ #define that need
to be cleaned up in the codebase - like the bit in util/getroot.c that
opens with
#if defined(__NetBSD__) || defined(__OpenBSD__)
and ends with
#endif /* defined(__NetBSD__) || defined(__FreeBSD__) || 
#defined(__FreeBSD_kernel__) */

And if you could send the patches out as a series with git send-email,
threaded mode with a cover letter, that would also be helpful.

A few comments inline.

> --- build-aux/test-driver.o   2013-07-29 08:36:33.775875020 -0400
> +++ build-aux/test-driver     2013-07-29 08:35:04.085870311 -0400
> @@ -0,0 +1,127 @@
> +#! /bin/sh
> +# test-driver - basic testsuite driver script.
> +
> +scriptversion=2012-06-27.10; # UTC
> +
> +# Copyright (C) 2011-2013 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2, or (at your option)
> +# any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# As a special exception to the GNU General Public License, if you
> +# distribute this file as part of a program that contains a
> +# configuration script generated by Autoconf, you may include it under
> +# the same distribution terms that you use for the rest of that program.
> +
> +# This file is maintained in Automake, please report
> +# bugs to <address@hidden> or send patches to
> +# <address@hidden>.
> +
> +# Make unconditional expansion of undefined variables an error.  This
> +# helps a lot in preventing typo-related bugs.
> +set -u
> +
> +usage_error ()
> +{
> +  echo "$0: $*" >&2
> +  print_usage >&2
> +  exit 2
> +}
> +
> +print_usage ()
> +{
> +  cat <<END
> +Usage:
> +  test-driver --test-name=NAME --log-file=PATH --trs-file=PATH
> +              [--expect-failure={yes|no}] [--color-tests={yes|no}]
> +              [--enable-hard-errors={yes|no}] [--] TEST-SCRIPT
> +The '--test-name', '--log-file' and '--trs-file' options are mandatory.
> +END
> +}
> +
> +# TODO: better error handling in option parsing (in particular, ensure
> +# TODO: $log_file, $trs_file and $test_name are defined).
> +test_name= # Used for reporting.
> +log_file=  # Where to save the output of the test script.
> +trs_file=  # Where to save the metadata of the test run.
> +expect_failure=no
> +color_tests=no
> +enable_hard_errors=yes
> +while test $# -gt 0; do
> +  case $1 in
> +  --help) print_usage; exit $?;;
> +  --version) echo "test-driver $scriptversion"; exit $?;;
> +  --test-name) test_name=$2; shift;;
> +  --log-file) log_file=$2; shift;;
> +  --trs-file) trs_file=$2; shift;;
> +  --color-tests) color_tests=$2; shift;;
> +  --expect-failure) expect_failure=$2; shift;;
> +  --enable-hard-errors) enable_hard_errors=$2; shift;;
> +  --) shift; break;;
> +  -*) usage_error "invalid option: '$1'";;
> +  esac
> +  shift
> +done
> +
> +if test $color_tests = yes; then
> +  # Keep this in sync with 'lib/am/check.am:$(am__tty_colors)'.
> +  red='' # Red.
> +  grn='' # Green.
> +  lgn='' # Light green.
> +  blu='' # Blue.
> +  mgn='' # Magenta.
> +  std=''     # No color.
> +else
> +  red= grn= lgn= blu= mgn= std=
> +fi
> +
> +do_exit='rm -f $log_file $trs_file; (exit $st); exit $st'
> +trap "st=129; $do_exit" 1
> +trap "st=130; $do_exit" 2
> +trap "st=141; $do_exit" 13
> +trap "st=143; $do_exit" 15
> +
> +# Test script is run here.
> +"$@" >$log_file 2>&1
> +estatus=$?
> +if test $enable_hard_errors = no && test $estatus -eq 99; then
> +  estatus=1
> +fi
> +
> +case $estatus:$expect_failure in
> +  0:yes) col=$red res=XPASS recheck=yes gcopy=yes;;
> +  0:*)   col=$grn res=PASS  recheck=no  gcopy=no;;
> +  77:*)  col=$blu res=SKIP  recheck=no  gcopy=yes;;
> +  99:*)  col=$mgn res=ERROR recheck=yes gcopy=yes;;
> +  *:yes) col=$lgn res=XFAIL recheck=no  gcopy=yes;;
> +  *:*)   col=$red res=FAIL  recheck=yes gcopy=yes;;
> +esac
> +
> +# Report outcome to console.
> +echo "${col}${res}${std}: $test_name"
> +
> +# Register the test result, and other relevant metadata.
> +echo ":test-result: $res" > $trs_file
> +echo ":global-test-result: $res" >> $trs_file
> +echo ":recheck: $recheck" >> $trs_file
> +echo ":copy-in-global-log: $gcopy" >> $trs_file
> +
> +# Local Variables:
> +# mode: shell-script
> +# sh-indentation: 2
> +# eval: (add-hook 'write-file-hooks 'time-stamp)
> +# time-stamp-start: "scriptversion="
> +# time-stamp-format: "%:y-%02m-%02d.%02H"
> +# time-stamp-time-zone: "UTC"
> +# time-stamp-end: "; # UTC"
> +# End:

> --- configure.ac.orig 2017-04-24 07:30:15.000000000 -0400
> +++ configure.ac      2019-05-27 13:03:39.615962000 -0400
> @@ -1787,19 +1787,26 @@
>  
>  if test x"$libzfs_excuse" = x ; then
>    # Only check for system headers if libzfs support has not been disabled.
> -  AC_CHECK_HEADERS(libzfs.h libnvpair.h)
> +  AC_CHECK_HEADERS(libzfs.h libnvpair.h, [], [], [
> +#include <stdint.h>
> +
> +typedef unsigned int uint_t;
> +typedef uint64_t hrtime_t;
> +typedef unsigned char uchar_t;
> +typedef int boolean_t;

I see these four lines repeated a couple of times below as well, but I
don't see any code in the tree making use of them. What are they
needed for? Is there some common header (i.e. grub/cpu/freebsd.h)
where they could go?

> +])
>  fi
>  
>  if test x"$libzfs_excuse" = x ; then
>    AC_CHECK_LIB([zfs], [libzfs_init],
>                 [],
> -               [libzfs_excuse="need zfs library"])
> +               [libzfs_excuse="need zfs library"], [-lavl -lnvpair -luutil 
> -lm])
>  fi
>  
>  if test x"$libzfs_excuse" = x ; then
>    AC_CHECK_LIB([nvpair], [nvlist_lookup_string],
>                 [],
> -               [libzfs_excuse="need nvpair library"])
> +               [libzfs_excuse="need nvpair library"], [-lavl -lnvpair 
> -luutil -lm])
>  fi
>  
>  if test x"$enable_libzfs" = xyes && test x"$libzfs_excuse" != x ; then
> @@ -1812,6 +1819,9 @@
>    AC_DEFINE([HAVE_LIBZFS], [1],
>              [Define to 1 if you have the ZFS library.])
>    LIBNVPAIR="-lnvpair"
> +  if test x$host_kernel = xkfreebsd; then
> +    LIBNVPAIR="$LIBNVPAIR -lavl -luutil -lm"
> +  fi
>    AC_DEFINE([HAVE_LIBNVPAIR], [1],
>              [Define to 1 if you have the NVPAIR library.])
>  fi

> --- grub-core/gnulib/argp-fmtstream.c.orig
> +++ grub-core/gnulib/argp-fmtstream.c
> @@ -47,7 +47,11 @@
>  #endif
>  
>  #define INIT_BUF_SIZE 200
> +#ifdef __FreeBSD__
> +#define PRINTF_SIZE_GUESS 32767
> +#else
>  #define PRINTF_SIZE_GUESS 150
> +#endif
>  
>  /* Return an argp_fmtstream that outputs to STREAM, and which prefixes lines
>     written on it with LMARGIN spaces and limits them to RMARGIN columns

> --- grub-core/gnulib/Makefile.am.orig 2019-05-27 11:52:29.110850000 -0400
> +++ grub-core/gnulib/Makefile.am      2019-05-27 11:52:58.816985000 -0400
> @@ -394,7 +394,7 @@
>         case '$(host_os)' in \
>           darwin[56]*) \
>             need_charset_alias=true ;; \
> -         darwin* | cygwin* | mingw* | pw32* | cegcc*) \
> +         darwin* | cygwin* | mingw* | pw32* | cegcc* | freebsd*) \
>             need_charset_alias=false ;; \
>           *) \
>             need_charset_alias=true ;; \

> --- grub-core/lib/libgcrypt_wrap/cipher_wrap.h.orig   2019-05-27 
> 20:34:31.465879000 -0400
> +++ grub-core/lib/libgcrypt_wrap/cipher_wrap.h        2019-05-27 
> 20:34:45.184492000 -0400
> @@ -41,6 +41,8 @@
>  
>  #define CIPHER_INFO_NO_WEAK_KEY    1
>  
> +typedef uint64_t u64;
> +
>  #define HAVE_U64_TYPEDEF 1
>  
>  /* Selftests are in separate modules.  */

> --- grub-core/osdep/unix/getroot.c.orig       2019-05-27 20:10:57.082707000 
> -0400
> +++ grub-core/osdep/unix/getroot.c    2019-05-27 20:11:08.847003000 -0400
> @@ -57,6 +57,11 @@
>  #include <sys/sysmacros.h>
>  #endif
>  
> +typedef unsigned int uint_t;
> +typedef unsigned char uchar_t;
> +typedef int boolean_t;
> +typedef uint64_t hrtime_t;
> +
>  #if defined(HAVE_LIBZFS) && defined(HAVE_LIBNVPAIR)
>  # include <grub/util/libzfs.h>
>  # include <grub/util/libnvpair.h>
> 

> --- include/grub/util/libzfs.h.orig
> +++ include/grub/util/libzfs.h
> @@ -22,6 +22,9 @@
>  #include <config.h>
>  
>  #ifdef HAVE_LIBZFS_H
> +#ifdef __FreeBSD__
> +typedef int boolean_t;
> +#endif
>  #include <libzfs.h>
>  #else /* ! HAVE_LIBZFS_H */
>  

> --- po/POTFILES.in.orig       2017-04-24 07:30:19.000000000 -0400
> +++ po/POTFILES.in    2019-05-27 12:18:39.807865000 -0400
> @@ -195,6 +195,7 @@
>  ./grub-core/fs/zfs/zfs_fletcher.c
>  ./grub-core/fs/zfs/zfs_lz4.c
>  ./grub-core/fs/zfs/zfs_lzjb.c
> +./grub-core/fs/zfs/zfs_lz4.c

This seems to add a file already existing 2 lines up.

>  ./grub-core/fs/zfs/zfs_sha256.c
>  ./grub-core/fs/zfs/zfscrypt.c
>  ./grub-core/fs/zfs/zfsinfo.c

> --- util/getroot.c.orig       2019-05-27 20:08:06.552358000 -0400
> +++ util/getroot.c    2019-05-27 20:08:26.241007000 -0400
> @@ -47,6 +47,11 @@
>  
>  #include <sys/types.h>
>  
> +typedef unsigned int uint_t;
> +typedef unsigned char uchar_t;
> +typedef int boolean_t;
> +typedef uint64_t hrtime_t;
> +
>  #if defined(HAVE_LIBZFS) && defined(HAVE_LIBNVPAIR)
>  # include <grub/util/libzfs.h>
>  # include <grub/util/libnvpair.h>

> --- util/grub-install.c.orig  2019-05-27 19:38:03.316491000 -0400
> +++ util/grub-install.c       2019-05-27 19:48:33.778506000 -0400
> @@ -45,6 +45,8 @@
>  
>  #include <string.h>
>  
> +#include <sys/utsname.h>
> +
>  #pragma GCC diagnostic ignored "-Wmissing-prototypes"
>  #pragma GCC diagnostic ignored "-Wmissing-declarations"
>  #include <argp.h>
> @@ -63,6 +65,7 @@
>  static char *bootdir = NULL;
>  static int allow_floppy = 0;
>  static int force_file_id = 0;
> +static int force_fs_label = 0;
>  static char *disk_module = NULL;
>  static char *efidir = NULL;
>  static char *macppcdir = NULL;
> @@ -86,17 +89,18 @@
>      OPTION_ROOT_DIRECTORY,
>      OPTION_TARGET,
>      OPTION_SETUP,
> -    OPTION_MKRELPATH, 
> -    OPTION_MKDEVICEMAP, 
> -    OPTION_PROBE, 
> -    OPTION_EDITENV, 
> -    OPTION_ALLOW_FLOPPY, 
> -    OPTION_RECHECK, 
> +    OPTION_MKRELPATH,
> +    OPTION_MKDEVICEMAP,
> +    OPTION_PROBE,
> +    OPTION_EDITENV,
> +    OPTION_ALLOW_FLOPPY,
> +    OPTION_RECHECK,

Getting rid of trailing whitespace is always good, but it should be a
separate patch. Same applies to rest of this file.

>      OPTION_FeORCE,
>      OPTION_FORCE_FILE_ID,
> -    OPTION_NO_NVRAM, 
> -    OPTION_REMOVABLE, 
> -    OPTION_BOOTLOADER_ID, 
> +    OPTION_FORCE_LABEL,
> +    OPTION_NO_NVRAM,
> +    OPTION_REMOVABLE,
> +    OPTION_BOOTLOADER_ID,
>      OPTION_EFI_DIRECTORY,
>      OPTION_FONT,
>      OPTION_DEBUG,
> @@ -114,13 +118,16 @@
>  
>  static int fs_probe = 1;
>  
> -static error_t 
> +static error_t
>  argp_parser (int key, char *arg, struct argp_state *state)
>  {
>    if (grub_install_parse (key, arg))
>      return 0;
>    switch (key)
>      {
> +    case OPTION_FORCE_LABEL:
> +      force_fs_label = 1;
> +      return 0;
>      case OPTION_FORCE_FILE_ID:
>        force_file_id = 1;
>        return 0;
> @@ -272,6 +279,8 @@
>     N_("install even if problems are detected"), 2},
>    {"force-file-id", OPTION_FORCE_FILE_ID, 0, 0,
>     N_("use identifier file even if UUID is available"), 2},
> +  {"force-label", OPTION_FORCE_LABEL, 0, 0,
> +   N_("use filesystem label even if UUID is available"), 2},
>    {"disk-module", OPTION_DISK_MODULE, N_("MODULE"), 0,
>     N_("disk module to use (biosdisk or native). "
>        "This option is only available on BIOS target."), 2},
> @@ -362,7 +371,7 @@
>    N_("Install GRUB on your drive.")"\v"
>    N_("INSTALL_DEVICE must be system device filename.\n"
>       "%s copies GRUB images into %s.  On some platforms, it"
> -     " may also install GRUB into the boot sector."), 
> +     " may also install GRUB into the boot sector."),
>    NULL, help_filter, NULL
>  };
>  
> @@ -389,13 +398,20 @@
>  push_partmap_module (const char *map, void *data __attribute__ ((unused)))
>  {
>    char buf[50];
> +  struct utsname name;
>  
> -  if (strcmp (map, "openbsd") == 0 || strcmp (map, "netbsd") == 0)
> +  if (strcmp (map, "openbsd") == 0 || strcmp (map, "netbsd") == 0 ||
> +      strcmp (map, "freebsd") == 0)

Why is both this

>      {
>        grub_install_push_module ("part_bsd");
>        return;
>      }
>  
> +  if (uname (&name) == 0 && strcmp (name.sysname, "FreeBSD") == 0)

and this needed?

> +    {
> +      grub_install_push_module ("part_bsd");
> +    }
> +

And if we match on sysname, why fall through instead of returning?

/
    Leif

>    snprintf (buf, sizeof (buf), "part_%s", map);
>    grub_install_push_module (buf);
>  }
> 





> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel