* [PATCH] support of hfsx ( case comparaison ) @ 2009-01-12 21:52 Michael Scherer 2009-02-07 21:02 ` Robert Millan 0 siblings, 1 reply; 14+ messages in thread From: Michael Scherer @ 2009-01-12 21:52 UTC (permalink / raw) To: grub-devel [-- Attachment #1: Type: text/plain, Size: 263 bytes --] Hi, Here is the second patch, for hfsx support. This patch replace the precedent. It is slightly more complex, and I didn't tested it on hfsx as I do not have a proper partition. But so far, it detect correctly config file on hfsplus. -- Michael Scherer [-- Attachment #2: grub.hfsx.support.diff --] [-- Type: application/octet-stream, Size: 3127 bytes --] diff --git a/ChangeLog b/ChangeLog index b77f438..d52d8ad 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2009-01-12 Michael Scherer <misc@mandriva.org> + + * fs/hfsplus.c: Complete support of hfs+ and hfsx, by doing + case insensitive comparaison when needed ( ie for hfsplus, and depending + on some settings for hfsx, found in the header ) + 2009-01-12 Christian Franke <franke@computer.org> * disk/ata.c (grub_ata_pciinit): Fix bit numbers of compatibility diff --git a/fs/hfsplus.c b/fs/hfsplus.c index e6493ce..3302724 100644 --- a/fs/hfsplus.c +++ b/fs/hfsplus.c @@ -96,6 +96,13 @@ struct grub_hfsplus_btheader grub_uint32_t last_leaf_node; grub_uint16_t nodesize; grub_uint16_t keysize; + grub_uint32_t total_nodes; + grub_uint32_t free_nodes; + grub_uint16_t reserved1; + grub_uint32_t clump_size; // ignored + grub_uint8_t btree_type; + grub_uint8_t key_compare; + grub_uint32_t attributes; } __attribute__ ((packed)); /* The on disk layout of a catalog key. */ @@ -159,6 +166,9 @@ enum grub_hfsplus_filetype GRUB_HFSPLUS_FILETYPE_REG_THREAD = 4 }; +#define GRUB_HFSPLUSX_BINARYCOMPARE 0xCF +#define GRUB_HFSPLUSX_CASEFOLDING 0xBC + /* Internal representation of a catalog key. */ struct grub_hfsplus_catkey_internal { @@ -218,6 +228,7 @@ struct grub_hfsplus_data /* This is the offset into the physical disk for an embedded HFS+ filesystem (one inside a plain HFS wrapper). */ int embedded_offset; + int catalog_cmp_key; }; #ifndef GRUB_UTIL @@ -460,6 +471,7 @@ grub_hfsplus_mount (grub_disk_t disk) data->catalog_tree.root = grub_be_to_cpu32 (header.root); data->catalog_tree.nodesize = grub_be_to_cpu16 (header.nodesize); + data->catalog_cmp_key = header.key_compare; if (! grub_hfsplus_read_file (&data->extoverflow_tree.file, 0, sizeof (struct grub_hfsplus_btnode), @@ -691,6 +703,20 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree, } static int +grub_hfsplus_is_case_insentive (struct grub_hfsplus_data *data) +{ + switch (grub_be_to_cpu16 (data->volheader.magic)) + { + case GRUB_HFSPLUS_MAGIC: + return 1; + case GRUB_HFSPLUSX_MAGIC: + return data->catalog_cmp_key == GRUB_HFSPLUSX_CASEFOLDING; + default: + return 0; + } +} + +static int grub_hfsplus_iterate_dir (grub_fshelp_node_t dir, int NESTED_FUNC_ATTR (*hook) (const char *filename, @@ -698,6 +724,7 @@ grub_hfsplus_iterate_dir (grub_fshelp_node_t dir, grub_fshelp_node_t node)) { int ret = 0; + int fs_case_insensitive = grub_hfsplus_is_case_insentive(dir->data); auto int list_nodes (void *record); int list_nodes (void *record) @@ -767,6 +794,10 @@ grub_hfsplus_iterate_dir (grub_fshelp_node_t dir, for (i = 0; i < grub_be_to_cpu16 (catkey->namelen); i++) catkey->name[i] = grub_be_to_cpu16 (catkey->name[i]); + /* hfs+ is case insensitive */ + if (fs_case_insensitive) + type |= GRUB_FSHELP_CASE_INSENSITIVE; + /* Only accept valid nodes. */ if (grub_strlen (filename) == grub_be_to_cpu16 (catkey->namelen)) { [-- Attachment #3: Type: text/plain, Size: 1 bytes --] ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] support of hfsx ( case comparaison ) 2009-01-12 21:52 [PATCH] support of hfsx ( case comparaison ) Michael Scherer @ 2009-02-07 21:02 ` Robert Millan 2009-05-03 17:19 ` Michael Scherer 0 siblings, 1 reply; 14+ messages in thread From: Robert Millan @ 2009-02-07 21:02 UTC (permalink / raw) To: The development of GRUB 2; +Cc: Michael Scherer, assign On Mon, Jan 12, 2009 at 10:52:48PM +0100, Michael Scherer wrote: > Hi, > > Here is the second patch, for hfsx support. This patch replace the > precedent. > > It is slightly more complex, and I didn't tested it on hfsx as I do > not have a proper partition. > But so far, it detect correctly config file on hfsplus. Hi, Thanks for your contribution. Would you be willing to assign copyright to the FSF for this? If you're fine with it, please let the FSF copyright clerk (CCed) know so he can send you the form. Thanks! -- Robert Millan The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and how) you may access your data; but nobody's threatening your freedom: we still allow you to remove your data and not access it at all." ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] support of hfsx ( case comparaison ) 2009-02-07 21:02 ` Robert Millan @ 2009-05-03 17:19 ` Michael Scherer 2009-06-01 10:11 ` Vladimir 'phcoder' Serbinenko 0 siblings, 1 reply; 14+ messages in thread From: Michael Scherer @ 2009-05-03 17:19 UTC (permalink / raw) To: The development of GRUB 2 [-- Attachment #1: Type: text/plain, Size: 984 bytes --] Le 7 févr. 09 à 22:02, Robert Millan a écrit : > On Mon, Jan 12, 2009 at 10:52:48PM +0100, Michael Scherer wrote: >> Hi, >> >> Here is the second patch, for hfsx support. This patch replace the >> precedent. >> >> It is slightly more complex, and I didn't tested it on hfsx as I do >> not have a proper partition. >> But so far, it detect correctly config file on hfsplus. > > Hi, > > Thanks for your contribution. Would you be willing to assign > copyright to > the FSF for this? If you're fine with it, please let the FSF > copyright > clerk (CCed) know so he can send you the form. Thanks to some postal problems, it took some months to get the copyright assignement to me. So, now it is over and I think you can apply the patch, I have rediffed against latest svn. I tried to test it again, just in case, but grub is failling with "menuentry , command not found." I will investigate a little bit more. Here is a updated patch. [-- Attachment #2: grub-hfsplus.diff --] [-- Type: application/octet-stream, Size: 3089 bytes --] diff --git a/ChangeLog b/ChangeLog index 2cf8f8f..e7e8530 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2009-05-03 Michael Scherer <misc@mandriva.org> + + * fs/hfsplus.c: Complete support of hfs+ and hfsx, by doing + case insensitive comparaison when needed ( ie for hfsplus, and depending + on some settings for hfsx, found in the header ) + 2009-05-03 Bean <bean123ch@gmail.com> Vladimir Serbinenko <phcoder@gmail.com> FreeBSD 64-bit support diff --git a/fs/hfsplus.c b/fs/hfsplus.c index 82ec880..8950b57 100644 --- a/fs/hfsplus.c +++ b/fs/hfsplus.c @@ -98,6 +98,13 @@ struct grub_hfsplus_btheader grub_uint32_t last_leaf_node; grub_uint16_t nodesize; grub_uint16_t keysize; + grub_uint32_t total_nodes; + grub_uint32_t free_nodes; + grub_uint16_t reserved1; + grub_uint32_t clump_size; // ignored + grub_uint8_t btree_type; + grub_uint8_t key_compare; + grub_uint32_t attributes; } __attribute__ ((packed)); /* The on disk layout of a catalog key. */ @@ -163,6 +170,9 @@ enum grub_hfsplus_filetype GRUB_HFSPLUS_FILETYPE_REG_THREAD = 4 }; +#define GRUB_HFSPLUSX_BINARYCOMPARE 0xCF +#define GRUB_HFSPLUSX_CASEFOLDING 0xBC + /* Internal representation of a catalog key. */ struct grub_hfsplus_catkey_internal { @@ -223,6 +233,7 @@ struct grub_hfsplus_data /* This is the offset into the physical disk for an embedded HFS+ filesystem (one inside a plain HFS wrapper). */ int embedded_offset; + int catalog_cmp_key; }; #ifndef GRUB_UTIL @@ -465,6 +476,7 @@ grub_hfsplus_mount (grub_disk_t disk) data->catalog_tree.root = grub_be_to_cpu32 (header.root); data->catalog_tree.nodesize = grub_be_to_cpu16 (header.nodesize); + data->catalog_cmp_key = header.key_compare; if (! grub_hfsplus_read_file (&data->extoverflow_tree.file, 0, sizeof (struct grub_hfsplus_btnode), @@ -696,6 +708,20 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree, } static int +grub_hfsplus_is_case_insentive (struct grub_hfsplus_data *data) +{ + switch (grub_be_to_cpu16 (data->volheader.magic)) + { + case GRUB_HFSPLUS_MAGIC: + return 1; + case GRUB_HFSPLUSX_MAGIC: + return data->catalog_cmp_key == GRUB_HFSPLUSX_CASEFOLDING; + default: + return 0; + } +} + +static int grub_hfsplus_iterate_dir (grub_fshelp_node_t dir, int NESTED_FUNC_ATTR (*hook) (const char *filename, @@ -703,6 +729,7 @@ grub_hfsplus_iterate_dir (grub_fshelp_node_t dir, grub_fshelp_node_t node)) { int ret = 0; + int fs_case_insensitive = grub_hfsplus_is_case_insentive(dir->data); auto int list_nodes (void *record); int list_nodes (void *record) @@ -773,7 +800,8 @@ grub_hfsplus_iterate_dir (grub_fshelp_node_t dir, catkey->name[i] = grub_be_to_cpu16 (catkey->name[i]); /* hfs+ is case insensitive. */ - type |= GRUB_FSHELP_CASE_INSENSITIVE; + if (fs_case_insensitive) + type |= GRUB_FSHELP_CASE_INSENSITIVE; /* Only accept valid nodes. */ if (grub_strlen (filename) == grub_be_to_cpu16 (catkey->namelen)) [-- Attachment #3: Type: text/plain, Size: 20 bytes --] -- Michael Scherer ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] support of hfsx ( case comparaison ) 2009-05-03 17:19 ` Michael Scherer @ 2009-06-01 10:11 ` Vladimir 'phcoder' Serbinenko 2009-06-03 7:56 ` Michael Scherer 0 siblings, 1 reply; 14+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2009-06-01 10:11 UTC (permalink / raw) To: The development of GRUB 2 On Sun, May 3, 2009 at 7:19 PM, Michael Scherer <misc@mandriva.org> wrote: > > Le 7 févr. 09 à 22:02, Robert Millan a écrit : > >> On Mon, Jan 12, 2009 at 10:52:48PM +0100, Michael Scherer wrote: >>> >>> Hi, >>> >>> Here is the second patch, for hfsx support. This patch replace the >>> precedent. >>> >>> It is slightly more complex, and I didn't tested it on hfsx as I do >>> not have a proper partition. >>> But so far, it detect correctly config file on hfsplus. >> >> Hi, >> >> Thanks for your contribution. Would you be willing to assign copyright to >> the FSF for this? If you're fine with it, please let the FSF copyright >> clerk (CCed) know so he can send you the form. > > > Thanks to some postal problems, it took some months to get the copyright > assignement > to me. So, now it is over and I think you can apply the patch, I have > rediffed against latest svn. > > I tried to test it again, just in case, but grub is failling with "menuentry > , command not found." I will investigate a little > bit more. > Hello, thank you for your contribution static int +grub_hfsplus_is_case_insentive (struct grub_hfsplus_data *data) +{ You can declare this function as inline. This way you also doesn't need to temporarily save its result for performance @@ -218,6 +228,7 @@ struct grub_hfsplus_data /* This is the offset into the physical disk for an embedded HFS+ filesystem (one inside a plain HFS wrapper). */ int embedded_offset; + int catalog_cmp_key; Where is this used? I see that you set it to a value but don't see you actually using it > Here is a updated patch. > > > > -- > Michael Scherer > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > http://lists.gnu.org/mailman/listinfo/grub-devel > > -- Regards Vladimir 'phcoder' Serbinenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] support of hfsx ( case comparaison ) 2009-06-01 10:11 ` Vladimir 'phcoder' Serbinenko @ 2009-06-03 7:56 ` Michael Scherer 2009-06-03 9:26 ` Vladimir 'phcoder' Serbinenko 0 siblings, 1 reply; 14+ messages in thread From: Michael Scherer @ 2009-06-03 7:56 UTC (permalink / raw) To: The development of GRUB 2 [-- Attachment #1: Type: text/plain, Size: 2177 bytes --] Le lundi 01 juin 2009 à 12:11 +0200, Vladimir 'phcoder' Serbinenko a écrit : > On Sun, May 3, 2009 at 7:19 PM, Michael Scherer <misc@mandriva.org> wrote: > > > > Le 7 févr. 09 à 22:02, Robert Millan a écrit : > > > >> On Mon, Jan 12, 2009 at 10:52:48PM +0100, Michael Scherer wrote: > >>> > >>> Hi, > >>> > >>> Here is the second patch, for hfsx support. This patch replace the > >>> precedent. > >>> > >>> It is slightly more complex, and I didn't tested it on hfsx as I do > >>> not have a proper partition. > >>> But so far, it detect correctly config file on hfsplus. > >> > >> Hi, > >> > >> Thanks for your contribution. Would you be willing to assign copyright to > >> the FSF for this? If you're fine with it, please let the FSF copyright > >> clerk (CCed) know so he can send you the form. > > > > > > Thanks to some postal problems, it took some months to get the copyright > > assignement > > to me. So, now it is over and I think you can apply the patch, I have > > rediffed against latest svn. > > > > I tried to test it again, just in case, but grub is failling with "menuentry > > , command not found." I will investigate a little > > bit more. > > > Hello, thank you for your contribution Well, thanks for taking time to review :) > static int > +grub_hfsplus_is_case_insentive (struct grub_hfsplus_data *data) > +{ > You can declare this function as inline. This way you also doesn't > need to temporarily save its result for performance Indeed, here is a new patch. Just for my own curiosity, isn't the inlining of function automatic with gcc and recent compiler ? > @@ -218,6 +228,7 @@ struct grub_hfsplus_data > /* This is the offset into the physical disk for an embedded HFS+ > filesystem (one inside a plain HFS wrapper). */ > int embedded_offset; > + int catalog_cmp_key; > Where is this used? I see that you set it to a value but don't see you > actually using it in grub_hfsplus_is_case_insentive, in the case of GRUB_HFSPLUSX_MAGIC : + case GRUB_HFSPLUSX_MAGIC: + return data->catalog_cmp_key == GRUB_HFSPLUSX_CASEFOLDING; -- Michael Scherer [-- Attachment #2: grub-hfsplus.diff --] [-- Type: text/x-patch, Size: 3041 bytes --] diff --git a/ChangeLog b/ChangeLog index 893a21c..58c544c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2009-06-03 Michael Scherer <misc@mandriva.org> + + * fs/hfsplus.c: Complete support of hfs+ and hfsx, by doing + case insensitive comparaison when needed ( ie for hfsplus, and depending + on some settings for hfsx, found in the header ) + 2009-05-26 Pavel Roskin <proski@gnu.org> * genmk.rb: Avoid shadowing variable `s', rename the outer `s' @@ -3106,7 +3112,6 @@ (freetype_libs): Likewise. * util/grub-mkfont.c: New file. - 2009-01-12 Christian Franke <franke@computer.org> * disk/ata.c (grub_ata_pciinit): Fix bit numbers of compatibility diff --git a/fs/hfsplus.c b/fs/hfsplus.c index 69794c9..4e3a32a 100644 --- a/fs/hfsplus.c +++ b/fs/hfsplus.c @@ -99,6 +99,13 @@ struct grub_hfsplus_btheader grub_uint32_t last_leaf_node; grub_uint16_t nodesize; grub_uint16_t keysize; + grub_uint32_t total_nodes; + grub_uint32_t free_nodes; + grub_uint16_t reserved1; + grub_uint32_t clump_size; // ignored + grub_uint8_t btree_type; + grub_uint8_t key_compare; + grub_uint32_t attributes; } __attribute__ ((packed)); /* The on disk layout of a catalog key. */ @@ -164,6 +171,9 @@ enum grub_hfsplus_filetype GRUB_HFSPLUS_FILETYPE_REG_THREAD = 4 }; +#define GRUB_HFSPLUSX_BINARYCOMPARE 0xCF +#define GRUB_HFSPLUSX_CASEFOLDING 0xBC + /* Internal representation of a catalog key. */ struct grub_hfsplus_catkey_internal { @@ -224,6 +234,7 @@ struct grub_hfsplus_data /* This is the offset into the physical disk for an embedded HFS+ filesystem (one inside a plain HFS wrapper). */ int embedded_offset; + int catalog_cmp_key; }; static grub_dl_t my_mod; @@ -464,6 +475,7 @@ grub_hfsplus_mount (grub_disk_t disk) data->catalog_tree.root = grub_be_to_cpu32 (header.root); data->catalog_tree.nodesize = grub_be_to_cpu16 (header.nodesize); + data->catalog_cmp_key = header.key_compare; if (! grub_hfsplus_read_file (&data->extoverflow_tree.file, 0, sizeof (struct grub_hfsplus_btnode), @@ -694,6 +706,20 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree, } } +static inline int +grub_hfsplus_is_case_insentive (struct grub_hfsplus_data *data) +{ + switch (grub_be_to_cpu16 (data->volheader.magic)) + { + case GRUB_HFSPLUS_MAGIC: + return 1; + case GRUB_HFSPLUSX_MAGIC: + return data->catalog_cmp_key == GRUB_HFSPLUSX_CASEFOLDING; + default: + return 0; + } +} + static int grub_hfsplus_iterate_dir (grub_fshelp_node_t dir, int NESTED_FUNC_ATTR @@ -772,7 +798,8 @@ grub_hfsplus_iterate_dir (grub_fshelp_node_t dir, catkey->name[i] = grub_be_to_cpu16 (catkey->name[i]); /* hfs+ is case insensitive. */ - type |= GRUB_FSHELP_CASE_INSENSITIVE; + if (grub_hfsplus_is_case_insentive (dir->data)) + type |= GRUB_FSHELP_CASE_INSENSITIVE; /* Only accept valid nodes. */ if (grub_strlen (filename) == grub_be_to_cpu16 (catkey->namelen)) ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] support of hfsx ( case comparaison ) 2009-06-03 7:56 ` Michael Scherer @ 2009-06-03 9:26 ` Vladimir 'phcoder' Serbinenko 2009-06-03 10:54 ` Michael Scherer 0 siblings, 1 reply; 14+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2009-06-03 9:26 UTC (permalink / raw) To: The development of GRUB 2 > Well, thanks for taking time to review :) You're welcome > > Just for my own curiosity, isn't the inlining of function automatic with > gcc and recent compiler ? It is. However if you want to be sure better to say it explicitely > >> @@ -218,6 +228,7 @@ struct grub_hfsplus_data >> /* This is the offset into the physical disk for an embedded HFS+ >> filesystem (one inside a plain HFS wrapper). */ >> int embedded_offset; >> + int catalog_cmp_key; >> Where is this used? I see that you set it to a value but don't see you >> actually using it > > in grub_hfsplus_is_case_insentive, in the case of GRUB_HFSPLUSX_MAGIC : > > + case GRUB_HFSPLUSX_MAGIC: > + return data->catalog_cmp_key == GRUB_HFSPLUSX_CASEFOLDING; > Ok. I see a two last problems with your patch: + grub_uint32_t clump_size; // ignored We use /* Ignored. */ style of comments @@ -3106,7 +3112,6 @@ (freetype_libs): Likewise. * util/grub-mkfont.c: New file. - Bogus hunk > > -- > Michael Scherer > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > http://lists.gnu.org/mailman/listinfo/grub-devel > > -- Regards Vladimir 'phcoder' Serbinenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] support of hfsx ( case comparaison ) 2009-06-03 9:26 ` Vladimir 'phcoder' Serbinenko @ 2009-06-03 10:54 ` Michael Scherer 2009-06-03 21:23 ` Pavel Roskin 0 siblings, 1 reply; 14+ messages in thread From: Michael Scherer @ 2009-06-03 10:54 UTC (permalink / raw) To: The development of GRUB 2 [-- Attachment #1: Type: text/plain, Size: 436 bytes --] Le mercredi 03 juin 2009 à 11:26 +0200, Vladimir 'phcoder' Serbinenko a écrit : > I see a two last problems with your patch: > + grub_uint32_t clump_size; // ignored > We use /* Ignored. */ style of comments > @@ -3106,7 +3112,6 @@ > (freetype_libs): Likewise. > > * util/grub-mkfont.c: New file. > - > Bogus hunk Both are fixed in this (hopefully) last version of the patch. -- Michael Scherer [-- Attachment #2: grub-hfsplus.diff --] [-- Type: text/x-patch, Size: 2832 bytes --] diff --git a/ChangeLog b/ChangeLog index 893a21c..64a2e04 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2009-06-03 Michael Scherer <misc@mandriva.org> + + * fs/hfsplus.c: Complete support of hfs+ and hfsx, by doing + case insensitive comparaison when needed ( ie for hfsplus, and depending + on some settings for hfsx, found in the header ) + 2009-05-26 Pavel Roskin <proski@gnu.org> * genmk.rb: Avoid shadowing variable `s', rename the outer `s' diff --git a/fs/hfsplus.c b/fs/hfsplus.c index 69794c9..37445c9 100644 --- a/fs/hfsplus.c +++ b/fs/hfsplus.c @@ -99,6 +99,13 @@ struct grub_hfsplus_btheader grub_uint32_t last_leaf_node; grub_uint16_t nodesize; grub_uint16_t keysize; + grub_uint32_t total_nodes; + grub_uint32_t free_nodes; + grub_uint16_t reserved1; + grub_uint32_t clump_size; /* ignored */ + grub_uint8_t btree_type; + grub_uint8_t key_compare; + grub_uint32_t attributes; } __attribute__ ((packed)); /* The on disk layout of a catalog key. */ @@ -164,6 +171,9 @@ enum grub_hfsplus_filetype GRUB_HFSPLUS_FILETYPE_REG_THREAD = 4 }; +#define GRUB_HFSPLUSX_BINARYCOMPARE 0xCF +#define GRUB_HFSPLUSX_CASEFOLDING 0xBC + /* Internal representation of a catalog key. */ struct grub_hfsplus_catkey_internal { @@ -224,6 +234,7 @@ struct grub_hfsplus_data /* This is the offset into the physical disk for an embedded HFS+ filesystem (one inside a plain HFS wrapper). */ int embedded_offset; + int catalog_cmp_key; }; static grub_dl_t my_mod; @@ -464,6 +475,7 @@ grub_hfsplus_mount (grub_disk_t disk) data->catalog_tree.root = grub_be_to_cpu32 (header.root); data->catalog_tree.nodesize = grub_be_to_cpu16 (header.nodesize); + data->catalog_cmp_key = header.key_compare; if (! grub_hfsplus_read_file (&data->extoverflow_tree.file, 0, sizeof (struct grub_hfsplus_btnode), @@ -694,6 +706,20 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree, } } +static inline int +grub_hfsplus_is_case_insentive (struct grub_hfsplus_data *data) +{ + switch (grub_be_to_cpu16 (data->volheader.magic)) + { + case GRUB_HFSPLUS_MAGIC: + return 1; + case GRUB_HFSPLUSX_MAGIC: + return data->catalog_cmp_key == GRUB_HFSPLUSX_CASEFOLDING; + default: + return 0; + } +} + static int grub_hfsplus_iterate_dir (grub_fshelp_node_t dir, int NESTED_FUNC_ATTR @@ -772,7 +798,8 @@ grub_hfsplus_iterate_dir (grub_fshelp_node_t dir, catkey->name[i] = grub_be_to_cpu16 (catkey->name[i]); /* hfs+ is case insensitive. */ - type |= GRUB_FSHELP_CASE_INSENSITIVE; + if (grub_hfsplus_is_case_insentive (dir->data)) + type |= GRUB_FSHELP_CASE_INSENSITIVE; /* Only accept valid nodes. */ if (grub_strlen (filename) == grub_be_to_cpu16 (catkey->namelen)) ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] support of hfsx ( case comparaison ) 2009-06-03 10:54 ` Michael Scherer @ 2009-06-03 21:23 ` Pavel Roskin 2009-06-05 19:23 ` Michael Scherer 0 siblings, 1 reply; 14+ messages in thread From: Pavel Roskin @ 2009-06-03 21:23 UTC (permalink / raw) To: The development of GRUB 2 Hello! I think the ChangeLog needs to be improved. It's immodest to claim "complete" support for something. It's a very strong statement. It's better to say "improve". Or better yet, let's be specific. Also please spell check the entry. "insensitive" and "insentive" is not the same. You want the former. Please end sentences with a period. I would write the ChangeLog entry as: * fs/hfsplus.c: Use case sensitive comparison for hfsx when required by the filesystem settings. Actually, it would be better to list the functions involved. I just want to show how long descriptions can be improved. I also prefer not to use any negative logic, and it's easier to get it wrong. Humans are notoriously bad at logic. Let's have grub_hfsplus_is_case_sensitive(). I would write it like this: static inline int grub_hfsplus_is_case_sensitive (struct grub_hfsplus_data *data) { if ((grub_be_to_cpu16 (data->volheader.magic) == GRUB_HFSPLUSX_MAGIC) && (data->catalog_cmp_key == GRUB_HFSPLUSX_BINARYCOMPARE)) return 1; else return 0; } No need to handle unknown magic numbers. We may need to use a comparison table as in hfs.c, as least for the first 256 Unicode characters, but it's a separate issue. -- Regards, Pavel Roskin ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] support of hfsx ( case comparaison ) 2009-06-03 21:23 ` Pavel Roskin @ 2009-06-05 19:23 ` Michael Scherer 2009-06-05 19:46 ` Vladimir 'phcoder' Serbinenko 2009-06-05 21:08 ` Pavel Roskin 0 siblings, 2 replies; 14+ messages in thread From: Michael Scherer @ 2009-06-05 19:23 UTC (permalink / raw) To: The development of GRUB 2 [-- Attachment #1: Type: text/plain, Size: 1847 bytes --] Le mercredi 03 juin 2009 à 17:23 -0400, Pavel Roskin a écrit : > Hello! > > I think the ChangeLog needs to be improved. It's immodest to claim > "complete" support for something. It's a very strong statement. It's > better to say "improve". Or better yet, let's be specific. Also please > spell check the entry. "insensitive" and "insentive" is not the same. Hi, Sorry, I am far from being a native english speaker, so that's why some badly translated expression slipped in and I didn't paid much attention to the changelog, I was more concerned by the code formating. > You want the former. Please end sentences with a period. I would > write the ChangeLog entry as: > > * fs/hfsplus.c: Use case sensitive comparison for hfsx when > required by the filesystem settings. > > Actually, it would be better to list the functions involved. I just > want to show how long descriptions can be improved. > > I also prefer not to use any negative logic, and it's easier to get it > wrong. Humans are notoriously bad at logic. Let's have > grub_hfsplus_is_case_sensitive(). I would write it like this: > > static inline int > grub_hfsplus_is_case_sensitive (struct grub_hfsplus_data *data) > { > if ((grub_be_to_cpu16 (data->volheader.magic) == GRUB_HFSPLUSX_MAGIC) > && (data->catalog_cmp_key == GRUB_HFSPLUSX_BINARYCOMPARE)) > return 1; > else > return 0; > } > > No need to handle unknown magic numbers. Ok, I have taken in account your remark. Here is another patch. > > We may need to use a comparison table as in hfs.c, as least for the > first 256 Unicode characters, but it's a separate issue. That a little bit too complex for me, I have just patched grub for the simplest case, and for the issue I faced on my own computer. -- Michael Scherer [-- Attachment #2: grub-hfsplus.diff --] [-- Type: text/x-patch, Size: 2826 bytes --] diff --git a/ChangeLog b/ChangeLog index 60ed1a6..5b29a7c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2009-06-05 Michael Scherer <misc@mandriva.org> + + * fs/hfsplus.c (grub_hfsplus_iterate_dir, grub_hfsplus_is_case_sensitive): + use case sensitive comparison for hfsx when required by the filesystem + settings. + (grub_hfsplus_mount) : copy filesystem setting for case + sensitive comparison. + 2009-06-05 Vladimir Serbinenko <phcoder@gmail.com> * conf/i386-pc.rmk (efiemu_mod_CFLAGS): remove -Werror -Wall diff --git a/fs/hfsplus.c b/fs/hfsplus.c index 69794c9..1c559ee 100644 --- a/fs/hfsplus.c +++ b/fs/hfsplus.c @@ -99,6 +99,13 @@ struct grub_hfsplus_btheader grub_uint32_t last_leaf_node; grub_uint16_t nodesize; grub_uint16_t keysize; + grub_uint32_t total_nodes; + grub_uint32_t free_nodes; + grub_uint16_t reserved1; + grub_uint32_t clump_size; /* ignored */ + grub_uint8_t btree_type; + grub_uint8_t key_compare; + grub_uint32_t attributes; } __attribute__ ((packed)); /* The on disk layout of a catalog key. */ @@ -164,6 +171,9 @@ enum grub_hfsplus_filetype GRUB_HFSPLUS_FILETYPE_REG_THREAD = 4 }; +#define GRUB_HFSPLUSX_BINARYCOMPARE 0xCF +#define GRUB_HFSPLUSX_CASEFOLDING 0xBC + /* Internal representation of a catalog key. */ struct grub_hfsplus_catkey_internal { @@ -224,6 +234,7 @@ struct grub_hfsplus_data /* This is the offset into the physical disk for an embedded HFS+ filesystem (one inside a plain HFS wrapper). */ int embedded_offset; + int catalog_cmp_key; }; static grub_dl_t my_mod; @@ -464,6 +475,7 @@ grub_hfsplus_mount (grub_disk_t disk) data->catalog_tree.root = grub_be_to_cpu32 (header.root); data->catalog_tree.nodesize = grub_be_to_cpu16 (header.nodesize); + data->catalog_cmp_key = header.key_compare; if (! grub_hfsplus_read_file (&data->extoverflow_tree.file, 0, sizeof (struct grub_hfsplus_btnode), @@ -694,6 +706,16 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree, } } +static inline int +grub_hfsplus_is_case_sensitive (struct grub_hfsplus_data *data) +{ + if ((grub_be_to_cpu16 (data->volheader.magic) == GRUB_HFSPLUSX_MAGIC) + && (data->catalog_cmp_key == GRUB_HFSPLUSX_BINARYCOMPARE)) + return 1; + else + return 0; +} + static int grub_hfsplus_iterate_dir (grub_fshelp_node_t dir, int NESTED_FUNC_ATTR @@ -772,7 +794,8 @@ grub_hfsplus_iterate_dir (grub_fshelp_node_t dir, catkey->name[i] = grub_be_to_cpu16 (catkey->name[i]); /* hfs+ is case insensitive. */ - type |= GRUB_FSHELP_CASE_INSENSITIVE; + if (!grub_hfsplus_is_case_sensitive (dir->data)) + type |= GRUB_FSHELP_CASE_INSENSITIVE; /* Only accept valid nodes. */ if (grub_strlen (filename) == grub_be_to_cpu16 (catkey->namelen)) ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] support of hfsx ( case comparaison ) 2009-06-05 19:23 ` Michael Scherer @ 2009-06-05 19:46 ` Vladimir 'phcoder' Serbinenko 2009-06-07 2:18 ` Pavel Roskin 2009-06-05 21:08 ` Pavel Roskin 1 sibling, 1 reply; 14+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2009-06-05 19:46 UTC (permalink / raw) To: The development of GRUB 2 On Fri, Jun 5, 2009 at 9:23 PM, Michael Scherer<misc@mandriva.org> wrote: > >> >> We may need to use a comparison table as in hfs.c, as least for the >> first 256 Unicode characters, but it's a separate issue. > > That a little bit too complex for me, I have just patched grub for the > simplest case, and for the issue I faced on my own computer. It's actually a problem with grub_strcasecmp since it doesn't take utf8 into account. HFS+ unlike HFS uses Unicode (UTF-16 normalisation form d to be exact) so no need for special handling here. Improving strcasecmp is possible and may even be compact. Even if unicode counts a lot of alphabets only few are bicameral. AFAIK main ones are Latin, Greek, Cyrillic and Armenian. I hope that in most cases the lowercase conversion can be done with some simple arithmetic operations > > -- > Michael Scherer > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > http://lists.gnu.org/mailman/listinfo/grub-devel > > -- Regards Vladimir 'phcoder' Serbinenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] support of hfsx ( case comparaison ) 2009-06-05 19:46 ` Vladimir 'phcoder' Serbinenko @ 2009-06-07 2:18 ` Pavel Roskin 2009-06-07 9:10 ` Vladimir 'phcoder' Serbinenko 0 siblings, 1 reply; 14+ messages in thread From: Pavel Roskin @ 2009-06-07 2:18 UTC (permalink / raw) To: The development of GRUB 2 On Fri, 2009-06-05 at 21:46 +0200, Vladimir 'phcoder' Serbinenko wrote: > On Fri, Jun 5, 2009 at 9:23 PM, Michael Scherer<misc@mandriva.org> wrote: > > > >> > >> We may need to use a comparison table as in hfs.c, as least for the > >> first 256 Unicode characters, but it's a separate issue. > > > > That a little bit too complex for me, I have just patched grub for the > > simplest case, and for the issue I faced on my own computer. > It's actually a problem with grub_strcasecmp since it doesn't take > utf8 into account. HFS+ unlike HFS uses Unicode (UTF-16 normalisation > form d to be exact) so no need for special handling here. Good to know. I was wrong to assume that hfsplus uses the same weird case ordering as hfs extended to Unicode. On the other hand, hfsplus uses composition and decomposition, at least optionally. So implementing it correctly may require quite a lot of code. > Improving > strcasecmp is possible and may even be compact. Even if unicode counts > a lot of alphabets only few are bicameral. AFAIK main ones are Latin, > Greek, Cyrillic and Armenian. I hope that in most cases the lowercase > conversion can be done with some simple arithmetic operations We are using grub_strcasecmp() on data in other encodings in some cases. I think short names on the FAT filesystem are never in UTF-8. Also, case comparison depends on the locale. That's too much complexity for the code GRUB. I would prefer the case comparison for hfsplus to be in the hfsplus module if we decide to implement it correctly. -- Regards, Pavel Roskin ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] support of hfsx ( case comparaison ) 2009-06-07 2:18 ` Pavel Roskin @ 2009-06-07 9:10 ` Vladimir 'phcoder' Serbinenko 2009-06-07 17:40 ` Pavel Roskin 0 siblings, 1 reply; 14+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2009-06-07 9:10 UTC (permalink / raw) To: The development of GRUB 2 On Sun, Jun 7, 2009 at 4:18 AM, Pavel Roskin<proski@gnu.org> wrote: >> Improving >> strcasecmp is possible and may even be compact. Even if unicode counts >> a lot of alphabets only few are bicameral. AFAIK main ones are Latin, >> Greek, Cyrillic and Armenian. I hope that in most cases the lowercase >> conversion can be done with some simple arithmetic operations > > We are using grub_strcasecmp() on data in other encodings in some cases. > I think short names on the FAT filesystem are never in UTF-8. Normally not but fat code passes short filenames to grub without any encoding conversion. In other words fat code assumes short filenames are in utf8. A problem in both hfs and fat is that it uses local encoding and at least in case of fat it's impossible to know which one from filesystem alone. We would need something like mount option to get this right. It could be sth like hd0_0_codepage=437 > Also, > case comparison depends on the locale. That's too much complexity for > the code GRUB. I would prefer the case comparison for hfsplus to be in > the hfsplus module if we decide to implement it correctly. I agree with you. I see no reason for users to have unicode characters in kernel names. And so getting it right is too much work for an almost zero benefit > > -- > Regards, > Pavel Roskin > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > http://lists.gnu.org/mailman/listinfo/grub-devel > -- Regards Vladimir 'phcoder' Serbinenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] support of hfsx ( case comparaison ) 2009-06-07 9:10 ` Vladimir 'phcoder' Serbinenko @ 2009-06-07 17:40 ` Pavel Roskin 0 siblings, 0 replies; 14+ messages in thread From: Pavel Roskin @ 2009-06-07 17:40 UTC (permalink / raw) To: The development of GRUB 2 On Sun, 2009-06-07 at 11:10 +0200, Vladimir 'phcoder' Serbinenko wrote: > On Sun, Jun 7, 2009 at 4:18 AM, Pavel Roskin<proski@gnu.org> wrote: > >> Improving > >> strcasecmp is possible and may even be compact. Even if unicode counts > >> a lot of alphabets only few are bicameral. AFAIK main ones are Latin, > >> Greek, Cyrillic and Armenian. I hope that in most cases the lowercase > >> conversion can be done with some simple arithmetic operations > > > > We are using grub_strcasecmp() on data in other encodings in some cases. > > I think short names on the FAT filesystem are never in UTF-8. > Normally not but fat code passes short filenames to grub without any > encoding conversion. In other words fat code assumes short filenames > are in utf8. A problem in both hfs and fat is that it uses local > encoding and at least in case of fat it's impossible to know which one > from filesystem alone. We would need something like mount option to > get this right. It could be sth like > hd0_0_codepage=437 Fortunately, it would only matter if the user tries to access a file with non-ASCII characters using a capitalization different from the one used by the filesystem. > > Also, > > case comparison depends on the locale. That's too much complexity for > > the code GRUB. I would prefer the case comparison for hfsplus to be in > > the hfsplus module if we decide to implement it correctly. > I agree with you. I see no reason for users to have unicode characters > in kernel names. And so getting it right is too much work for an > almost zero benefit I case of hfsplus, there is a chance to miss a file in the binary tree if there are non-ASCII files in the same directory. But I agree, it's not worth the trouble at this point. -- Regards, Pavel Roskin ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] support of hfsx ( case comparaison ) 2009-06-05 19:23 ` Michael Scherer 2009-06-05 19:46 ` Vladimir 'phcoder' Serbinenko @ 2009-06-05 21:08 ` Pavel Roskin 1 sibling, 0 replies; 14+ messages in thread From: Pavel Roskin @ 2009-06-05 21:08 UTC (permalink / raw) To: The development of GRUB 2 On Fri, 2009-06-05 at 21:23 +0200, Michael Scherer wrote: > Ok, I have taken in account your remark. Here is another patch. I've applied your patch with two fixes. Somehow you interchanged the values of GRUB_HFSPLUSX_BINARYCOMPARE and GRUB_HFSPLUSX_CASEFOLDING. GRUB_HFSPLUSX_BINARYCOMPARE is 0xBC and GRUB_HFSPLUSX_CASEFOLDING is 0xCF. The values are actually abbreviations. I wonder if you have tested your patch. I have tested the final patch on both case-sensitive and case-insensitive filesystems, and it appears to be OK. Also, it should not be needed to call an inline function from grub_hfsplus_iterate_dir(). It's easier to calculate it once for the filesystem at the mount time. -- Regards, Pavel Roskin ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2009-06-07 17:40 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-01-12 21:52 [PATCH] support of hfsx ( case comparaison ) Michael Scherer 2009-02-07 21:02 ` Robert Millan 2009-05-03 17:19 ` Michael Scherer 2009-06-01 10:11 ` Vladimir 'phcoder' Serbinenko 2009-06-03 7:56 ` Michael Scherer 2009-06-03 9:26 ` Vladimir 'phcoder' Serbinenko 2009-06-03 10:54 ` Michael Scherer 2009-06-03 21:23 ` Pavel Roskin 2009-06-05 19:23 ` Michael Scherer 2009-06-05 19:46 ` Vladimir 'phcoder' Serbinenko 2009-06-07 2:18 ` Pavel Roskin 2009-06-07 9:10 ` Vladimir 'phcoder' Serbinenko 2009-06-07 17:40 ` Pavel Roskin 2009-06-05 21:08 ` Pavel Roskin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.