mkuzip: detect underlying filesystem when building uzip header

[rew]

Summary

mkuzip inserts a 128-byte length header at the beginning of uzip'ed
images. This header is a shell script that, when run, will mount the
uzip'ed image to the provied mountpoint. The type of compression used
when generating the uzip'ed image is also embedded in this shell script.

The problem is that the shell script is hardcoded to only mount cd96660
filesystems instead of using the mount command of the underlying
filesytem in the uzip'ed image.

This commit solves this problem by detecting the type of filesystem of
the uzip'ed image when constructing the header.

Current support is for ufs, cd9660, zfs, and msdosfs.

PR: 276174

[delphij]

Not sure if the length of magic allows you to squeeze in something like [ -n "$1" ] && before mounts (if the user didn't specify a mountpoint, the image should work like before the change).

usr.bin/mkuzip/mkuzip.c

139

What if the user didn't specify a mountpoint? (same applies to cd9660 and msdosfs)

153

For zpool it's probably reasonable to use -R $1.

[rew]

incorporate feedback:

• use [ -z "$1" ] && exit 1; to bail out early

The following command:
mkuzip -A zstd -o zfs.uzip zpool.img

Produces the following header (123 bytes):

#!/bin/sh
#Z4.0 Format
[ -z "$1" ] && exit 1;kldload -n geom_uzip && mdconfig $0 && zpool import -o readonly=on $1;exit $?

For ZFS, the argument passed to the shell script is the
pool name to import.

For all other filesystems, the argument passed to the shell
script is the directory where the filesystem will be mounted.

[rew]

usr.bin/mkuzip/mkuzip.c

139

With the updated changes, if a mountpoint isn't provided the script will exit with no error message and exit code = 1.

if the provided argument isn't valid, mount will error out with a message.

the error handling isn't great but, not much extra room to work with.

153

for ZFS, $1 is the pool name to import.

-R would require passing another argument, adding this would push it over the 128-byte limit.

[delphij]

In D43425#990231, @rew wrote:

For ZFS, the argument passed to the shell script is the
pool name to import.

Ah you are right, but the name is not guaranteed to be identical to the filename. Probably zpool import -a -d /dev/$(mdconfig $0).uzip instead? (and maybe -R $1)

[rew]

In D43425#990237, @delphij wrote:

In D43425#990231, @rew wrote:

For ZFS, the argument passed to the shell script is the
pool name to import.

Ah you are right, but the name is not guaranteed to be identical to the filename. Probably zpool import -a -d /dev/$(mdconfig $0).uzip instead? (and maybe -R $1)

yea, that would work - it bumps it over the limit though, so if we want zpool import -d ... then [ -z $1 ] && exit 1 would need to be removed.

I'm fine either way, I had almost considered removing the "run uzip image as a script" functionality altogether since it has been broken so long anyways.

[delphij]

In D43425#990303, @rew wrote:

In D43425#990237, @delphij wrote:

In D43425#990231, @rew wrote:

For ZFS, the argument passed to the shell script is the
pool name to import.

Ah you are right, but the name is not guaranteed to be identical to the filename. Probably zpool import -a -d /dev/$(mdconfig $0).uzip instead? (and maybe -R $1)

yea, that would work - it bumps it over the limit though, so if we want zpool import -d ... then [ -z $1 ] && exit 1 would need to be removed.

I'm fine either way, I had almost considered removing the "run uzip image as a script" functionality altogether since it has been broken so long anyways.

I see..

It's probably reasonable to not attach zpool in that case: without -R a zpool could have a mountpoint=/ (or other datasets that would overlays the regular mountpoints) and it would be a potential attack vector, and if we specify -N it would diverge from other filesystems. The functionality of attaching of zpool was not available in the past, so in my opinion it's should be okay to leave it unimplemented and just treat it as an unrecognized filesystem (I don't have strong opinion here, and the rest of the change looked fine to me).

[sobomax]

Looks OKish (I never used that feature myself, only for testing), but the fact that this is now runs by default and tries to do bunch of unnecessary magic under the hood is of a big concern. I already have a problem with pkg starting to open /dev/null just in case, I don't want this to cause my builds to start suddenly failing right and left.

usr.bin/mkuzip/mkuzip.c

291

Ughh, no please no. If you want to add this complex procedure just to run mkuzip either make it optional or ignore when it fails for whatever reason. I don't really care what it puts into that area, but by default mkuzip should be ROBUST. I often run it on chrooted environment, so ability to work without having any /dev/crap is of paramount importance!

[sobomax]

P.S. popen(3) under the hood uses /bin/sh, so it needs tons of stuff to be present and working. Not to mention having fstat around is not guranteed.

[rew]

usr.bin/mkuzip/mkuzip.c

291

Should the "run the image like a shell script" feature just be dropped?

The shell shebang and compression type will have to stay intact to remain compatible with the kernel. And then remove the documentation that references the shell script in the first part of the uzip image.

Let me know and I'll update this review to either drop the feature or to have build_header() always succeed.

[sobomax]

usr.bin/mkuzip/mkuzip.c

291

Making it always succeed and reverting to a pre-baked default is fine with me. As long as it does not cause any corruption to the generated image or weird result code. :)