[Bug 226714] zfsboot(8) erroneously suggests creating a BSD label

[bugzilla-noreply at freebsd.org]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=226714

--- Comment #14 from Warner Losh <imp at FreeBSD.org> ---
(In reply to Eugene Grosbein from comment #12)

So I'm not so sure that's the bug.

The bug appears to be here:
         pa.fd = open(devname, O_RDONLY);
        if (pa.fd == -1)
                return (0);
        ret = zfs_probe(pa.fd, ppa->pool_guid);
        if (ret == 0)
                return (0);
        /* Do we have BSD label here? */
        if (part->type == PART_FREEBSD) {
                pa.devname = devname;
                pa.pool_guid = ppa->pool_guid;
                pa.secsz = ppa->secsz;
                table = ptable_open(&pa, part->end - part->start + 1,
                    ppa->secsz, zfs_diskread);
                if (table != NULL) {
                        ptable_iterate(table, &pa, zfs_probe_partition);
                        ptable_close(table);
                }
        }

So if we can't open the s1 device (which we prohibit currently), then we
return. If we can't open the device, we should just not probe zfs on the slice,
but we should probe it for the BSD partitions. That bug is easy enough to fix.
The bug is here, not in libsa. It's behavior is working as designed.
ptable_open should recurse properly though.

It's kind of a moot distinction, though, because while we support non-zero
offsets for the zpool partition, we can't really boot off such partitions
because zfsboot assumes that the bootable zpool is in a partition that is at
offset 0 (need not be the first one, it appears). The current instructions
assume that zfsboot is installed in the ZFS boot zone, which also assumes that
the uber block is where it would be if the sliced started at location zero.

I think the following fix is the right one, but I'm not sure yet:
diff --git a/stand/zfs/zfs.c b/stand/zfs/zfs.c
index 0050bbe0056..f51c5c1efaa 100644
--- a/stand/zfs/zfs.c
+++ b/stand/zfs/zfs.c
@@ -488,15 +488,19 @@ zfs_probe_partition(void *arg, const char *partname,
                return (0);

        ppa = (struct zfs_probe_args *)arg;
-       strncpy(devname, ppa->devname, strlen(ppa->devname) - 1);
-       devname[strlen(ppa->devname) - 1] = '\0';
-       sprintf(devname, "%s%s:", devname, partname);
+       sprintf(devname, "%s%s:", ppa->devname, partname);
        pa.fd = open(devname, O_RDONLY);
-       if (pa.fd == -1)
-               return (0);
-       ret = zfs_probe(pa.fd, ppa->pool_guid);
-       if (ret == 0)
-               return (0);
+       if (pa.fd != -1) {
+               /*
+                * Only probe the slice if there's no BSD label
+                * on it. When we have a nested scheme, we'll not
+                * be able to open the slice, but we will be able
+                * to open the ptable.
+                */
+               ret = zfs_probe(pa.fd, ppa->pool_guid);
+               if (ret == 0)
+                       return (0);
+       }
        /* Do we have BSD label here? */
        if (part->type == PART_FREEBSD) {
                pa.devname = devname;

Oh, and I agree we should stop telling people to create a BSD partition, but we
should support it because sometimes it's necessary for a swap or a dump
partition.

-- 
You are receiving this mail because:
You are the assignee for the bug.