Hello Maintainers,

Creating an HFS volume with a volume name other than the default of
"untitled" is currently not possible. It does appear that it should be
possible. Here is output exhibiting this issue:

==================================================
# mkfs.hfs -b 512 -v 'grub' -h /tmp/hfs.img
Initialized /tmp/hfs.img as a 5120 MB HFS volume
# fsck.hfs /tmp/hfs.img

** /tmp/hfs.img

   Executing fsck_hfs (version 540.1-Linux).

** Checking HFS volume.

   The volume name is untitled
** Checking extents overflow file.
** Checking catalog file.
** Checking catalog hierarchy.
** Checking volume bitmap.
** Checking volume information.
** The volume untitled appears to be OK.
==================================================

Using -N to print the parameters shows that at least the option parsing
allows this:

==================================================
# mkfs.hfs -N -b 512b -v 'grub' -h /tmp/hfs.img
0 sectors at 512 bytes per sector
HFS format parameters:
        volume name: "grub"
        block-size: 262144
        total blocks: 20480
        first free catalog node id: 16
        initial catalog file size: 1048576
        initial extents file size: 1048576
        file clump size: 1048576
==================================================

Looking at the package source, I see that HFS volume creation is added
as a debian package patch and not part of the upstream source in patch
file debian/patches/0005-Re-add-support-for-creating-legacy-HFS-filesystems.patch.
The issue appears to be that the volume name passed in as the -v option
argument and being written to the hfs parameters struct is being
unconditionally overritten in line 325 of the patch file with the
default volume name. This can be fixed by swapping the first two
aruments to bcopy on that line, however, the comment and the lines
directly above it lead me to believe there's potentially more to it.

This is causing GRUB HFS tests to fail and it would be nice to get them
passing again without disabling the failing test.

Thanks,
Glenn

Hi Glenn!

Yes, this patch was created by me since we need legacy HFS support for the Apple
PowerMacs that we support in Debian Ports for the powerpc and ppc64 ports.

Thanks for debugging and reporting this. I will have a look at this bug and
fix it hopefully soon. Also, thanks for fixing so many issues in GRUB, I'm
on the GRUB mailing list as well and I'm seeing your regular influx of
patches there!

Adrian

Hi Glenn!

So, I just had a look at the code again and compared it with the original code
from the last version of the diskdev_cmds which supported legacy HFS [1].

The original code contains an embedded function to map UTF-8 into Mac encoding,
search for "Map UTF-8 input into a Mac encoding.". I didn't reimplement that
code back then because it uses some string code from Apple's CoreFoundation
libraries. I assumed no one would bother if we just hardwire the volume label
to "untitled" and moved on.

However, since you now reported this bug, I think it's reasonable to fix
this issue and I think it should be possible with the help of the sources
of version 332.25-11.

Looking at the corresponding patch [2], you can see that the patch implements
a similar approach of what you suggested, see:

+	mdbp->drVN[0] = strlen(defaults->volumeName);
+	bcopy(defaults->volumeName,&mdbp->drVN[1],mdbp->drVN[0]);

I'll think a bit over it to decide what approach we use. Reimplementing Apple's
code as close as possible without using the CoreFoundation stuff would be my
preferred solution.

Maybe you have a suggestion on how to reimplement Apple's UTF-8-to-Mac conversion
code from [1].

Adrian

Hi Adrian!

This dropped off my radar, but I'm thinking about this issue again.

On Tue, 29 Mar 2022 19:15:29 +0200 John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote:

Based on how the GRUB tests use the -v argument, I don't think we need
to do any re-encoding. The GRUB test uses recode to convert UTF8 to
macroman and uses that as the argument.

I haven't really looked at the code, but I imagine it should be fairly
easy to just copy the argument to -v into mdbp->drVN[1].

If we really needed to probably using libiconv would be best. That's
what recode does. But as I said above, I don't think we need to.

Glenn