Add support for ZFS Boot Environments to SADE

[allanjude]

Summary

Add a optional, but default, pool layout that will work with the ZFS BE Menu
Add support for UEFI + ZFS
Add Compression option as well
Fix text and helptext for atime=off option
Fix a segfault when editing an existing ZFS entry via SADE

[smh]

usr.sbin/bsdinstall/partedit/gpart_ops.c

175

Should we default this to lz4? or provide an option for lz4, which is the replacement for lzjb.

IIRC this isn't the default for upstream as they don't have lz4 boot support but I believe we do.

739–749

Not related to this and it really doesn't make any difference but all other tests are strcmp(var, const)

741

Is &mountpoint[1] correct, typically would expect 0?

If would switch this to:

if (strdup((strlen(mountpoint) == 1) {
    zpool_name = "zroot";
} else {
    zpool_name = &mountpoint[1];
    for (i = 0; zpool_name[i] != 0; i++)
                if (!isalnum(zpool_name[i]))
                          zpool_name[i] = '_';
}

Also why are you doing the replacement, might be good to comment that?

usr.sbin/bsdinstall/partedit/partedit_x86.c

82–83

Unneeded wrap?

[allanjude]

usr.sbin/bsdinstall/partedit/gpart_ops.c

175

in rS286547 compression=on changed to mean lz4 if the zpool has the feature.

741

This code is just repeated from line 690, because in the case where newfs == NULL (we are not running newfs or zpool create, such as editing the mountpoint), the zpool name was NULL, and this segfaulted on strlen/strdup.

The 1 is to take everything after the /, whereas 0 would include the / which is not an allowed character.

741

The code you provided is a bit wrong. You probably don't want the strdup in the if, without assigning it to something. Also, you DO want to strdup the &mountpoint[1], else when we edit it to replace non-alpha characters, like / with _, we'll screw up the mountpoint variable used for fstab

[nwhitehorn]

This is a great feature! Thanks for the work. There are three inline comments. Two are nits, but the third (about fstab) is a more complicated issue.

usr.sbin/bsdinstall/partedit/gpart_ops.c

153

What is the /mnt for here? This looks like left-in debugging code.

usr.sbin/bsdinstall/partedit/partedit.c

60

How does this fixed layout interact with bsdinstall/scripts/umount? The existing partedit zfs code relies on putting ZFS filesystems in /etc/fstab, which this design does not do. zfs_enable="YES" doesn't get added to rc.conf, either, so I think these won't even be mounted.

421

It would be great while you are editing this if you could remove the shell script that somehow ended up in here and make this whole sequence write to the logfile directly with C commands. The system("echo") calls are a little bizarre.

Moreover, all of the logging code here is a little bogus because, as sade, this can run without a logfile defined outside of the context of the installer.

[allanjude]

usr.sbin/bsdinstall/partedit/gpart_ops.c

153

In the case of the boot environment created a few minutes later, the mountpoint is set to /, so we don't want that to mount over top of the running system, but be rooted in /mnt. It doesn't hurt the non-BE case because the mountpoint is set to 'none'

739–749

fixed

usr.sbin/bsdinstall/partedit/partedit.c

60

because the entries are not added to fstab (it does still add an entry if you disable the boot environment option), they do not get unmounted. The same is true of the setup done by zfsboot. I mostly had failed to consider the case of running bsdinstall outside of the installer media, where you would reboot when the installer was finished.

There is an open PR about this issue here: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=204506

also nice catch, I totally forgot about the zfs_enable="YES" part.

421

In the case of sade, or the env var not being set, there is:
if (getenv("BSDINSTALL_LOG") == NULL)

		setenv("BSDINSTALL_LOG", "/dev/null", 1);

But yeah, better logging would be good.

[allanjude]

Fixes suggested by smh@ and nwhitehorn@

Write /etc/rc.conf (via /tmp/bsdinstall_etc/rc.conf.zfs if BSDINSTALL_TMPETC is set)

[allanjude]

usr.sbin/bsdinstall/partedit/partedit.c

421

I am quite new to C, what do you suggest as the best way to do the logging? opening the commands with popen()?

[allanjude]

Update

[nwhitehorn]

This is getting there now! I have some inline replies to your comments.

usr.sbin/bsdinstall/partedit/gpart_ops.c

153

That will clobber any existing filesystem at /mnt if the code is run as sade from a multiuser system. Nothing this code does should have the side effect of mounting any file systems anywhere for that reason. This is why the existing code passes "-m none".

usr.sbin/bsdinstall/partedit/partedit.c

60

Hrm, this is unfortunate. Is there a way to add them to fstab until that PR is finished? It's very hard to do the kind of logic that the installer/sade needs in a robust way with ZFS automounting.

421

You can just fopen() (or open()) and fwrite() (or write()) the logfile in append mode. This code should never execute any other commands, via system() or via popen().

421

The check on BSDINSTALL_LOG sounds reasonable. Better would be conditioning all the logic on the presence of that variable (which is easy in the context of the C file IO commands).

477

I feel *extremely* uncomfortable with this code in the sade cast. Have sade modify my rc.conf is a somewhat astonishing outcome. Moreover, I may already have zfs_enable="foo" lines in my rc.conf that this will duplicate. For the installer case, it works well. Maybe, for sade, it's best if it prompts you to add the relevant lines to your rc.conf in a dialog after finishing?

[allanjude]

usr.sbin/bsdinstall/partedit/partedit.c

421

I think you missed my point re: popen. How would you get the output of stdout and stderr from the commands being run (zfs create)?

Or do you want to only log the commands being run, not the output? (which is important in the case of errors)

[nwhitehorn]

usr.sbin/bsdinstall/partedit/partedit.c

421

That is true! Sorry for missing it. The logging code is OK as-is, then. Running ZFS commands directly from here breaks a bunch of layering in the partition editor code, though. Can these commands be added to a partition metadata field? For example, you could set bootenv to be a pointer to zfs_be_datasets rather than a flag, which would improve the encapsulation of the code.

[dteske]

usr.sbin/bsdinstall/partedit/partedit.c

421

I would use posix_spawnp(3). See head/lib/libdpv/util.c:60-107 (function shell_spawn_pipecmd()).

However, I think changing this is well beyond the scope of this review and a PR should be filed, leading to a new review.