[PATCH V2 1/2] hfsplus: Change finder_info to u32

[PATCH V2 1/2] hfsplus: Change finder_info to u32

[Matthew Garrett]

The finder_info block in the hfsplus volume header is currently defined as
an array of 8 bit values, but TN1150 defines it as being an array of 32 bit
values. Fix for convenience.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 fs/hfsplus/hfsplus_raw.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/hfsplus/hfsplus_raw.h b/fs/hfsplus/hfsplus_raw.h
index 927cdd6..921967e 100644
--- a/fs/hfsplus/hfsplus_raw.h
+++ b/fs/hfsplus/hfsplus_raw.h
@@ -117,7 +117,7 @@ struct hfsplus_vh {
 	__be32 write_count;
 	__be64 encodings_bmp;
 
-	u8 finder_info[32];
+	u32 finder_info[8];
 
 	struct hfsplus_fork_raw alloc_file;
 	struct hfsplus_fork_raw ext_file;
-- 
1.7.7.1

[PATCH V2 2/2] hfsplus: Add an ioctl to bless files

[Matthew Garrett]

Making an hfsplus partition bootable requires the ability to "bless" a
file by putting its inode number in the volume header. Doing this from
userspace on a mounted filesystem is impractical since the kernel will
write back the original values on unmount. Add an ioctl to allow userspace
to update the volume header information based on the target file.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---

V2 - remember to fix the endianness.

 Documentation/ioctl/ioctl-number.txt |    1 +
 fs/hfsplus/hfsplus_fs.h              |    1 +
 fs/hfsplus/ioctl.c                   |   22 ++++++++++++++++++++++
 3 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 4840334..6965c1b 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -213,6 +213,7 @@ Code  Seq#(hex)	Include File		Comments
 'f'	00-0F	fs/ext4/ext4.h		conflict!
 'f'	00-0F	linux/fs.h		conflict!
 'f'	00-0F	fs/ocfs2/ocfs2_fs.h	conflict!
+'f'	20-2F	fs/hfsplus/hfsplus_fs.h
 'g'	00-0F	linux/usb/gadgetfs.h
 'g'	20-2F	linux/usb/g_printer.h
 'h'	00-7F				conflict! Charon filesystem
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 21a5b7f..1036936 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -315,6 +315,7 @@ static inline unsigned short hfsplus_min_io_size(struct super_block *sb)
 #define HFSPLUS_IOC_EXT2_GETFLAGS	FS_IOC_GETFLAGS
 #define HFSPLUS_IOC_EXT2_SETFLAGS	FS_IOC_SETFLAGS
 
+#define HFSPLUS_IOC_BLESS		_IO('f', 0x20)
 
 /*
  * Functions in any *.c used in other files
diff --git a/fs/hfsplus/ioctl.c b/fs/hfsplus/ioctl.c
index f66c765..6d22b49 100644
--- a/fs/hfsplus/ioctl.c
+++ b/fs/hfsplus/ioctl.c
@@ -20,6 +20,26 @@
 #include <asm/uaccess.h>
 #include "hfsplus_fs.h"
 
+static int hfsplus_ioctl_bless(struct file *file, int __user *user_flags)
+{
+	struct inode *inode = file->f_dentry->d_inode;
+	struct hfsplus_sb_info *sbi = HFSPLUS_SB(inode->i_sb);
+	struct hfsplus_vh *vh = sbi->s_vhdr;
+	struct hfsplus_vh *bvh = sbi->s_backup_vhdr;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	mutex_lock(&sbi->vh_mutex);
+	vh->finder_info[0] = bvh->finder_info[0] =
+		cpu_to_be32(parent_ino(file->f_dentry));
+	vh->finder_info[1] = bvh->finder_info[1] = cpu_to_be32(inode->i_ino);
+	vh->finder_info[5] = bvh->finder_info[5] =
+		cpu_to_be32(parent_ino(file->f_dentry));
+	mutex_unlock(&sbi->vh_mutex);
+	return 0;
+}
+
 static int hfsplus_ioctl_getflags(struct file *file, int __user *user_flags)
 {
 	struct inode *inode = file->f_path.dentry->d_inode;
@@ -108,6 +128,8 @@ long hfsplus_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		return hfsplus_ioctl_getflags(file, argp);
 	case HFSPLUS_IOC_EXT2_SETFLAGS:
 		return hfsplus_ioctl_setflags(file, argp);
+	case HFSPLUS_IOC_BLESS:
+		return hfsplus_ioctl_bless(file, argp);
 	default:
 		return -ENOTTY;
 	}
-- 
1.7.7.1

Re: [PATCH V2 1/2] hfsplus: Change finder_info to u32

[Christoph Hellwig]

On Thu, Feb 02, 2012 at 03:39:50PM -0500, Matthew Garrett wrote:
> The finder_info block in the hfsplus volume header is currently defined as
> an array of 8 bit values, but TN1150 defines it as being an array of 32 bit
> values. Fix for convenience.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

Looks good, I'll queue it up.

Re: [PATCH V2 2/2] hfsplus: Add an ioctl to bless files

[Christoph Hellwig]

+#define HFSPLUS_IOC_BLESS              _IO('f', 0x20)

I'd probably move this to fs.h and follow the numbering there,
otherwise we are bound to run into conflicts.

> +static int hfsplus_ioctl_bless(struct file *file, int __user *user_flags)

Care to add a little comment on what this ioctl does, bless is a bit
of a generic name.

> +{
> +	struct inode *inode = file->f_dentry->d_inode;

Please use file->f_path.dentry instead of the f_dentry define, please.

Also given that we use the dentry almost as often I'd pull it into a
local variable as well.

> +	mutex_lock(&sbi->vh_mutex);
> +	vh->finder_info[0] = bvh->finder_info[0] =
> +		cpu_to_be32(parent_ino(file->f_dentry));
> +	vh->finder_info[1] = bvh->finder_info[1] = cpu_to_be32(inode->i_ino);
> +	vh->finder_info[5] = bvh->finder_info[5] =
> +		cpu_to_be32(parent_ino(file->f_dentry));

Any idea why we write the parent twice?



Not directly relevant, but where do you plan to put the userspace to
call this ioctl?

Re: [PATCH V2 2/2] hfsplus: Add an ioctl to bless files

[Matthew Garrett]

On Mon, Feb 06, 2012 at 12:35:53PM -0500, Christoph Hellwig wrote:
> +#define HFSPLUS_IOC_BLESS              _IO('f', 0x20)
> 
> I'd probably move this to fs.h and follow the numbering there,
> otherwise we are bound to run into conflicts.

Ok. Any problem with leaving something filesystem specific in there?

> > +static int hfsplus_ioctl_bless(struct file *file, int __user *user_flags)
> 
> Care to add a little comment on what this ioctl does, bless is a bit
> of a generic name.

Will do.

> > +{
> > +	struct inode *inode = file->f_dentry->d_inode;
> 
> Please use file->f_path.dentry instead of the f_dentry define, please.

Ok.

> > +	mutex_lock(&sbi->vh_mutex);
> > +	vh->finder_info[0] = bvh->finder_info[0] =
> > +		cpu_to_be32(parent_ino(file->f_dentry));
> > +	vh->finder_info[1] = bvh->finder_info[1] = cpu_to_be32(inode->i_ino);
> > +	vh->finder_info[5] = bvh->finder_info[5] =
> > +		cpu_to_be32(parent_ino(file->f_dentry));
> 
> Any idea why we write the parent twice?

>From the spec:

"finderInfo[0] contains the directory ID of the directory containing the 
bootable system (for example, the System Folder in Mac OS 8 or 9, or 
/System/Library/CoreServices in Mac OS X). It is zero if there is no 
bootable system on the volume. This value is typically equal to either 
finderInfo[3] or finderInfo[5]."

finderInfo[3] is the OS8/9 system folder, finderInfo[5] is the OS X one. 
I'd guess it's just to indicate whether the boot media should default to 
OS8/9 or OS X, but it's not entirely clear.

> Not directly relevant, but where do you plan to put the userspace to
> call this ioctl?

There's a git repo at git://cavan.codon.org.uk/mactel-boot

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH V2 2/2] hfsplus: Add an ioctl to bless files

[Christoph Hellwig]

On Mon, Feb 06, 2012 at 05:45:49PM +0000, Matthew Garrett wrote:
> On Mon, Feb 06, 2012 at 12:35:53PM -0500, Christoph Hellwig wrote:
> > +#define HFSPLUS_IOC_BLESS              _IO('f', 0x20)
> > 
> > I'd probably move this to fs.h and follow the numbering there,
> > otherwise we are bound to run into conflicts.
> 
> Ok. Any problem with leaving something filesystem specific in there?

Leaving it in hfsplus sounds fine, but I'd avoid using 'f' then just to
reduce the chance for overlap.

> >From the spec:
> 
> "finderInfo[0] contains the directory ID of the directory containing the 
> bootable system (for example, the System Folder in Mac OS 8 or 9, or 
> /System/Library/CoreServices in Mac OS X). It is zero if there is no 
> bootable system on the volume. This value is typically equal to either 
> finderInfo[3] or finderInfo[5]."
> 
> finderInfo[3] is the OS8/9 system folder, finderInfo[5] is the OS X one. 
> I'd guess it's just to indicate whether the boot media should default to 
> OS8/9 or OS X, but it's not entirely clear.

Care to add this explanation as a comment?

Re: [PATCH V2 2/2] hfsplus: Add an ioctl to bless files

[Matthew Garrett]

On Mon, Feb 06, 2012 at 12:49:43PM -0500, Christoph Hellwig wrote:
> On Mon, Feb 06, 2012 at 05:45:49PM +0000, Matthew Garrett wrote:
> > On Mon, Feb 06, 2012 at 12:35:53PM -0500, Christoph Hellwig wrote:
> > > +#define HFSPLUS_IOC_BLESS              _IO('f', 0x20)
> > > 
> > > I'd probably move this to fs.h and follow the numbering there,
> > > otherwise we are bound to run into conflicts.
> > 
> > Ok. Any problem with leaving something filesystem specific in there?
> 
> Leaving it in hfsplus sounds fine, but I'd avoid using 'f' then just to
> reduce the chance for overlap.

Using the same range but putting it in a different file seems like a 
good way to have someone miss it. I added the range to ioctl-list.txt so 
in theory nobody should step on it - do you think that's likely to be 
inadequate?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH V2] hfsplus: Add an ioctl to bless files

[Matthew Garrett]

Making an hfsplus partition bootable requires the ability to "bless" a
file by putting its inode number in the volume header. Doing this from
userspace on a mounted filesystem is impractical since the kernel will
write back the original values on unmount. Add an ioctl to allow userspace
to update the volume header information based on the target file.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
Kept the ioctl in the hfs code, but moved it to a different range to reduce
reduce the chances of someone stepping on it with another filesystem.

 Documentation/ioctl/ioctl-number.txt |    1 +
 fs/hfsplus/hfsplus_fs.h              |    5 +++++
 fs/hfsplus/ioctl.c                   |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 4840334..37a4248 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -218,6 +218,7 @@ Code  Seq#(hex)	Include File		Comments
 'h'	00-7F				conflict! Charon filesystem
 					<mailto:zapman@interlan.net>
 'h'	00-1F	linux/hpet.h		conflict!
+'h'	80-8F	fs/hfsplus/ioctl.c
 'i'	00-3F	linux/i2o-dev.h		conflict!
 'i'	0B-1F	linux/ipmi.h		conflict!
 'i'	80-8F	linux/i8k.h
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 21a5b7f..4e75ac6 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -317,6 +317,11 @@ static inline unsigned short hfsplus_min_io_size(struct super_block *sb)
 
 
 /*
+ * hfs+-specific ioctl for making the filesystem bootable
+ */
+#define HFSPLUS_IOC_BLESS _IO('h', 0x80)
+
+/*
  * Functions in any *.c used in other files
  */
 
diff --git a/fs/hfsplus/ioctl.c b/fs/hfsplus/ioctl.c
index f66c765..c640ba5 100644
--- a/fs/hfsplus/ioctl.c
+++ b/fs/hfsplus/ioctl.c
@@ -20,6 +20,38 @@
 #include <asm/uaccess.h>
 #include "hfsplus_fs.h"
 
+/*
+ * "Blessing" an HFS+ filesystem writes metadata to the superblock informing
+ * the platform firmware which file to boot from
+ */
+static int hfsplus_ioctl_bless(struct file *file, int __user *user_flags)
+{
+	struct dentry *dentry = file->f_path.dentry;
+	struct inode *inode = dentry->d_inode;
+	struct hfsplus_sb_info *sbi = HFSPLUS_SB(inode->i_sb);
+	struct hfsplus_vh *vh = sbi->s_vhdr;
+	struct hfsplus_vh *bvh = sbi->s_backup_vhdr;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	mutex_lock(&sbi->vh_mutex);
+
+	/* Directory containing the bootable system */
+	vh->finder_info[0] = bvh->finder_info[0] =
+		cpu_to_be32(parent_ino(dentry));
+
+	/* Bootloader */
+	vh->finder_info[1] = bvh->finder_info[1] = cpu_to_be32(inode->i_ino);
+
+	/* Per spec, the OS X system folder - same as finder_info[0] here */
+	vh->finder_info[5] = bvh->finder_info[5] =
+		cpu_to_be32(parent_ino(dentry));
+
+	mutex_unlock(&sbi->vh_mutex);
+	return 0;
+}
+
 static int hfsplus_ioctl_getflags(struct file *file, int __user *user_flags)
 {
 	struct inode *inode = file->f_path.dentry->d_inode;
@@ -108,6 +140,8 @@ long hfsplus_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		return hfsplus_ioctl_getflags(file, argp);
 	case HFSPLUS_IOC_EXT2_SETFLAGS:
 		return hfsplus_ioctl_setflags(file, argp);
+	case HFSPLUS_IOC_BLESS:
+		return hfsplus_ioctl_bless(file, argp);
 	default:
 		return -ENOTTY;
 	}
-- 
1.7.7.6

Re: [PATCH V2] hfsplus: Add an ioctl to bless files

[Christoph Hellwig]

On Mon, Feb 06, 2012 at 03:14:40PM -0500, Matthew Garrett wrote:
> Making an hfsplus partition bootable requires the ability to "bless" a
> file by putting its inode number in the volume header. Doing this from
> userspace on a mounted filesystem is impractical since the kernel will
> write back the original values on unmount. Add an ioctl to allow userspace
> to update the volume header information based on the target file.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

Looks good, thanks.

Re: [PATCH V2] hfsplus: Add an ioctl to bless files

[Henrik Rydberg]

On Mon, Feb 06, 2012 at 03:14:40PM -0500, Matthew Garrett wrote:
> Making an hfsplus partition bootable requires the ability to "bless" a
> file by putting its inode number in the volume header. Doing this from
> userspace on a mounted filesystem is impractical since the kernel will
> write back the original values on unmount. Add an ioctl to allow userspace
> to update the volume header information based on the target file.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---

Works nicely, great stuff.

    Tested-by: Henrik Rydberg <rydberg@euromail.se>

Thanks,
Henrik