|
From: Philipp T. <pt...@su...> - 2003-03-25 17:21:30
|
The latest NTFS code in 2.5.65 still has this code in
super.c(parse_ntfs_boot_sector):
/*
* Get the size of the volume in clusters and check for 64-bit-ness.
* Windows currently only uses 32 bits to save the clusters so we do
* the same as it is much faster on 32-bit CPUs.
*/
if ((u64)ll >= 1ULL << (sizeof(unsigned long) * 8)) {
ntfs_error(vol->sb, "Cannot handle %i-bit clusters. Sorry.",
sizeof(unsigned long) * 4);
return FALSE;
}
Now on AMD64 (aka x86-64, Hammer), long is 64 bit wide, so the test resolves to:
(u64 >= 1ULL << 64)
which effectively is
(u64 >= 0)
So, as expected, mounting a NTFS partiton fails.
Looks like this should use sizeof(u32) instead of sizeof(unsigned long).
With the diff below, I'm able to mount a NTFS partition and all seems
well.
BTW, in the light of x86-64 Hardware slated for 1Q 2003, shouldn't the
driver be changed to use 64 bits on platforms where it's the native size,
i.e. BITS_PER_LONG == 64? With this platform it will get fairly common to
have 64 bit machines that also have NTFS partitions.
Philipp
2003-03-25 Philipp Thomas <pt...@su...>
* super.c (parse_ntfs_boot_sector): Use u32 not unsigned long
to check for the use of 64 bits.
--- fs/ntfs.old/super.c 2003-03-17 22:44:20.000000000 +0100
+++ fs/ntfs/super.c 2003-03-25 18:17:00.000000000 +0100
@@ -619,9 +619,9 @@
* the same as it is much faster on 32-bit CPUs.
*/
ll = sle64_to_cpu(b->number_of_sectors) >> sectors_per_cluster_bits;
- if ((u64)ll >= 1ULL << (sizeof(unsigned long) * 8)) {
+ if ((u64)ll >= 1ULL << (sizeof(u32) * 8)) {
ntfs_error(vol->sb, "Cannot handle %i-bit clusters. Sorry.",
- sizeof(unsigned long) * 4);
+ sizeof(u32) * 4);
return FALSE;
}
vol->nr_clusters = ll;
--
Philipp Thomas <pt...@su...>
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nuremberg, Germany
|
|
From: Anton A. <ai...@ca...> - 2003-03-27 15:36:05
|
Hi,
Sorry for the delay in replying.
On Tue, 25 Mar 2003, Philipp Thomas wrote:
> The latest NTFS code in 2.5.65 still has this code in
> super.c(parse_ntfs_boot_sector):
>
> /*
> * Get the size of the volume in clusters and check for 64-bit-ness.
> * Windows currently only uses 32 bits to save the clusters so we do
> * the same as it is much faster on 32-bit CPUs.
> */
> if ((u64)ll >= 1ULL << (sizeof(unsigned long) * 8)) {
> ntfs_error(vol->sb, "Cannot handle %i-bit clusters. Sorry.",
> sizeof(unsigned long) * 4);
> return FALSE;
> }
>
> Now on AMD64 (aka x86-64, Hammer), long is 64 bit wide, so the test resolves to:
>
> (u64 >= 1ULL << 64)
>
> which effectively is
>
> (u64 >= 0)
>
> So, as expected, mounting a NTFS partiton fails.
Grrr. Silly 64 bit architectures. The above test was designed with ULL =
128 bit in mind and not ULL = UL which the x86-64 seems to have. The test
really should only be:
if ((u64)ll >= (1ULL << 32)) {
ntfs_error(vol->sb, "Cannot handle 64-bit clusters. Sorry.");
return FALSE;
}
Best regards,
Anton
--
Anton Altaparmakov <aia21 at cantab.net> (replace at with @)
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/
|
|
From: Philipp T. <pt...@su...> - 2003-03-27 15:49:25
|
* Anton Altaparmakov (ai...@ca...) [20030327 16:35]:
> Sorry for the delay in replying.
No problem, as long as I do get a reply :)
> if ((u64)ll >= (1ULL << 32)) {
> ntfs_error(vol->sb, "Cannot handle 64-bit clusters. Sorry.");
> return FALSE;
> }
OK. As I'll be sending a patch to remove the use of unnamed typedef'd
structures in layout.h (see my other mail) anyways, I'll also send a patch
to fix this.
cheers
Philipp
--
Philipp Thomas <pt...@su...>
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nuremberg, Germany
|
|
From: Anton A. <ai...@ca...> - 2003-03-27 15:51:42
|
On Thu, 27 Mar 2003, Philipp Thomas wrote:
> * Anton Altaparmakov (ai...@ca...) [20030327 16:35]:
>
> > Sorry for the delay in replying.
>
> No problem, as long as I do get a reply :)
>
> > if ((u64)ll >= (1ULL << 32)) {
> > ntfs_error(vol->sb, "Cannot handle 64-bit clusters. Sorry.");
> > return FALSE;
> > }
>
> OK. As I'll be sending a patch to remove the use of unnamed typedef'd
> structures in layout.h (see my other mail) anyways,
Why would you want to do that? I like them just as they are...
> I'll also send a patch to fix this.
Cool, thanks.
Anton
--
Anton Altaparmakov <aia21 at cantab.net> (replace at with @)
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/
|
|
From: Philipp T. <pt...@su...> - 2003-03-27 16:12:13
|
* Anton Altaparmakov (ai...@ca...) [20030327 16:50]: > > OK. As I'll be sending a patch to remove the use of unnamed typedef'd > > structures in layout.h (see my other mail) anyways, > > Why would you want to do that? I like them just as they are... Mainly because it's a bug that gcc 3.2 accepts it without an additional command line switch! Secondly because I agree with Richard Henderson that this is gross. gcc 3.3 (used in SuSE Linux 8.2) gave a warning so I asked on the gcc mailinglist and also filed a bug report: http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gcc&pr=10225 This was closed by Richard Henderson with the following comment: See Synopsis: [3.3/3.4 regression] Unnamed structs aren't handled correctly It was a bug that 3.2 accepted this. For 3.3 and beyond this is accepted with the -fms-extensions switch, but not otherwise. I'll admit the documentation needs clarification, but it's also true that you'll not find this "feature" documented anywhere in the 3.2 tree. In the discussion on the GCC ml rth also wrote this (quoting my mail): Please smack the ntfs folk for me please. This is gross. You can view the whole discussion starting at http://gcc.gnu.org/ml/gcc/2003-03/msg01542.html Philipp -- Philipp Thomas <pt...@su...> SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nuremberg, Germany |
|
From: Anton A. <ai...@ca...> - 2003-03-27 16:21:29
|
On Thu, 27 Mar 2003, Philipp Thomas wrote: > * Anton Altaparmakov (ai...@ca...) [20030327 16:50]: > > > > OK. As I'll be sending a patch to remove the use of unnamed typedef'd > > > structures in layout.h (see my other mail) anyways, > > > > Why would you want to do that? I like them just as they are... > > Mainly because it's a bug that gcc 3.2 accepts it without an additional > command line switch! Secondly because I agree with Richard Henderson that > this is gross. I disagree that it is gross. Microsoft compilers work with it just fine. gcc should do the same in it certainly did so fine in the past (gcc-2.96). It was only broken later on and now apparently requires a compile switch. This is annoying but we can just add the switch to the ntfs makefile depending on the gcc version used by the user. Shouldn't be too hard to do this. (Maybe we can just always add it if older gcc allow for the switch, too.) I like unnamed structs and unions a lot as I think they make for a very nice abstraction and a huge shorthand when writing code. I absolutely *despise* code like this: ATTR_RECORD *a; if (a->non_resident) a->n.data_size = 0; else a->r.value_length = 0; *much* nicer to have: ATTR_RECORD *a; if (a->non_resident) a->data_size = 0; else a->value_length = 0; I think the first case is gross where one has to invent stupid single letter names just because the compiler is too stupid to understand which field we want to use. It is damn obvious to the human eye and it should be damn obvious to the compiler! And if not I consider it a shortcomming of the compiler. Best regards, Anton > gcc 3.3 (used in SuSE Linux 8.2) gave a warning so I asked on the gcc > mailinglist and also filed a bug report: > > http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gcc&pr=10225 > > This was closed by Richard Henderson with the following comment: > > See Synopsis: [3.3/3.4 regression] Unnamed structs aren't handled correctly > > It was a bug that 3.2 accepted this. For 3.3 and beyond this > is accepted with the -fms-extensions switch, but not otherwise. > > I'll admit the documentation needs clarification, but it's > also true that you'll not find this "feature" documented > anywhere in the 3.2 tree. > > In the discussion on the GCC ml rth also wrote this (quoting my mail): > > Please smack the ntfs folk for me please. This is gross. > > You can view the whole discussion starting at > > http://gcc.gnu.org/ml/gcc/2003-03/msg01542.html > > Philipp > > -- > Philipp Thomas <pt...@su...> > SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nuremberg, Germany -- Anton Altaparmakov <aia21 at cantab.net> (replace at with @) Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/ |
|
From: Szakacsits S. <sz...@si...> - 2003-03-27 16:33:32
|
On Thu, 27 Mar 2003, Anton Altaparmakov wrote: > I like unnamed structs and unions a lot as I think they make for a very > nice abstraction and a huge shorthand when writing code. I absolutely > *despise* code like this: > > ATTR_RECORD *a; > > if (a->non_resident) > a->n.data_size = 0; > else > a->r.value_length = 0; > > *much* nicer to have: > > ATTR_RECORD *a; > > if (a->non_resident) > a->data_size = 0; > else > a->value_length = 0; Happy you mentioned :) Please add to these the endianess mess what the ntfs code has and look some other code how these are solved elegantly. Szaka |
|
From: Anton A. <ai...@ca...> - 2003-03-27 16:44:47
|
On Thu, 27 Mar 2003, Szakacsits Szabolcs wrote: > Happy you mentioned :) Please add to these the endianess mess what the ntfs > code has and look some other code how these are solved elegantly. What "endianess" mess? The way ntfs does it is AFAIK how all of the kernel does it. I can't see anything messy about it. If there is a better way, please tell... What code solves it more elegantly? You can't expect me to guess where to look! Best regards, Anton -- Anton Altaparmakov <aia21 at cantab.net> (replace at with @) Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/ |
|
From: Philipp T. <pt...@su...> - 2003-03-27 17:13:26
Attachments:
ntfs_64bit.diff
ntfs_makefile.diff
|
* Anton Altaparmakov (ai...@ca...) [20030327 17:20]:
> I disagree that it is gross. Microsoft compilers work with it just fine.
We don't disagree that much :) I agree that anonymous structs and unions are
indeed a usefull abstraction. It's the use of anonymous typedef'd
structs/unions I find gross.
And I'll bet you this will *never* make it into an official C standard. This
was only implemented in gcc in order to allow the mingw port to use MS
headers. I'm quite sure that if it wasn't for mingw, this would *never* have
been accepted for gcc.
Quoting from gcc's c-decl.c(grokfield):
/* This is an unnamed decl.
If we have something of the form "union { list } ;" then this
is the anonymous union extension. Similarly for struct.
If this is something of the form "struct foo;", then
If MS extensions are enabled, this is handled as an
anonymous struct.
Otherwise this is a forward declaration of a structure tag.
If this is something of the form "foo;" and foo is a TYPE_DECL, then
If MS extensions are enabled and foo names a structure, then
again this is an anonymous struct.
Otherwise this is an error.
Oh what a horrid tangled web we weave. I wonder if MS consciously
took this from Plan 9 or if it was an accident of implementation
that took root before someone noticed the bug... */
The main part for me is that "struct foo;" has a defined meaning in ISO C99
and this meaning is suddenly changed when -fms-extensions is used.
But as you're the Maintainer, it's your decision to make. Find attached the
patches for super.c and for ntfs/Makefile to make it use
-fms-extensions.
2003-27.03.2003 Philipp Thomas <pt...@su...>
* Makefile: Use -fms-extensions if gcc groks it.
* super.c(parse_ntfs_boot_sector): Correct the check for 64-bit
clusters.
--
Philipp Thomas <pt...@su...>
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nuremberg, Germany
|
|
From: Anton A. <ai...@ca...> - 2003-03-28 10:47:26
|
Hi, On Thu, 27 Mar 2003, Philipp Thomas wrote: > * Anton Altaparmakov (ai...@ca...) [20030327 17:20]: > > I disagree that it is gross. Microsoft compilers work with it just fine. > We don't disagree that much :) I agree that anonymous structs and unions are > indeed a usefull abstraction. It's the use of anonymous typedef'd > structs/unions I find gross. Ah, ok. I just do typedefs because I am lazy. (-; I know Linus doesn't like typedefs much either so if you wanted to submit a patch killing the typedefs and using "struct MFT_RECORD" instead of just "MFT_RECORD" then I would accept that. But I doubt this would solve the gcc problem. We would still need the -fms-extensions, wouldn't we? > 2003-27.03.2003 Philipp Thomas <pt...@su...> > * Makefile: Use -fms-extensions if gcc groks it. I will hold on for your reply to above question and apply if still necessary. > * super.c(parse_ntfs_boot_sector): Correct the check for 64-bit > clusters. Applied. Thanks for your work! Best regards, Anton -- Anton Altaparmakov <aia21 at cantab.net> (replace at with @) Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/ |
|
From: Philipp T. <pt...@su...> - 2003-03-28 11:31:53
|
* Anton Altaparmakov (ai...@ca...) [20030328 11:46]: > Ah, ok. I just do typedefs because I am lazy. (-; I know Linus doesn't > like typedefs much either so if you wanted to submit a patch killing the > typedefs and using "struct MFT_RECORD" instead of just "MFT_RECORD" then I > would accept that. > > But I doubt this would solve the gcc problem. We would still need the > -fms-extensions, wouldn't we? Yes, and rightly so. Both 'struct foo;' and 'foo;' as unnamed members are rejected without -fms-extensions and indeed to me they smell too much of MS kludging around bugs they themselves introduced. So I would *really* like to change the code such that it doesn't need the -fms-extensions by using named structs where necessary, thus only making use of *documented* gcc extensions. > > 2003-27.03.2003 Philipp Thomas <pt...@su...> > > * Makefile: Use -fms-extensions if gcc groks it. > > I will hold on for your reply to above question and apply if still > necessary. I'd rather see you agreeing on the above. > > * super.c(parse_ntfs_boot_sector): Correct the check for 64-bit > > clusters. > > Applied. > > Thanks for your work! Well, it's in my own interest to have the ntfs driver working on AMD64 :) Philipp -- Philipp Thomas <pt...@su...> SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nuremberg, Germany |
|
From: Anton A. <ai...@ca...> - 2003-03-28 11:38:36
|
On Fri, 28 Mar 2003, Philipp Thomas wrote: > Yes, and rightly so. Both 'struct foo;' and 'foo;' as unnamed members are > rejected without -fms-extensions and indeed to me they smell too much of MS > kludging around bugs they themselves introduced. So I would *really* like to > change the code such that it doesn't need the -fms-extensions by using named > structs where necessary, thus only making use of *documented* gcc > extensions. > > > > 2003-27.03.2003 Philipp Thomas <pt...@su...> > > > * Makefile: Use -fms-extensions if gcc groks it. > > > > I will hold on for your reply to above question and apply if still > > necessary. > > I'd rather see you agreeing on the above. How about you show me a snippet of what you propose it to look like, i.e. send me a patch that changes layout.h to your proposal (no need to change the rest of the driver yet). I am willing to have a look but I am not promising to like or accept it. If I find it sufficiently non-revolting I will let you know and I would then accept a patch that switches the whole driver but no promises! Best regards, Anton -- Anton Altaparmakov <aia21 at cantab.net> (replace at with @) Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/ |
|
From: Philipp T. <pt...@su...> - 2003-03-28 17:50:38
Attachments:
ntfs_layout.diff
|
* Anton Altaparmakov (ai...@ca...) [20030328 12:44]:
> How about you show me a snippet of what you propose it to look like, i.e.
> send me a patch that changes layout.h to your proposal (no need to change
> the rest of the driver yet).
Here's the patch for layout.h. With this, the driver is compilable without
any further changes. I also tested the 2.95.3 path by changing the test
in types.h to a simple '#if 1'. BTW, if you don't like
@@ -2037,7 +2037,7 @@ typedef struct {
/* sizeof() = 16 bytes */
} __attribute__ ((__packed__)) INDEX_ENTRY_HEADER;
-#define _IIF(X) SC(ieh.iif,X)
+#define _IIF(X) _IEH(SC(iif,X))
#define _IEV(X) SC(iif.iev,X)
We'd need something like
#if __GNUC__ < 3
# define SC2(P1,P2,N) P1.P2.N
#else
# define SC2(P1,P2,N) P1.N
#endif
#define _IIF(X) SC2(ieh,iif,X)
Though looking at the macro mess needed to support gcc < 3.0 I'd rather drop
the use of unnamed structs/unions until the kernel drops support for gcc <
3.0, as something like ie->_IIF(index_length) is really hard to read and not
exactly what I'd call intuitive.
Philipp
2003-03-28 Philipp Thomas <pt...@su...>
* layout.h: Remove the use of unnamed typedef'd structs and
adapt the accessor macros accordingly.
--
Philipp Thomas <pt...@su...>
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nuremberg, Germany
|
|
From: Szakacsits S. <sz...@si...> - 2003-03-28 18:48:41
|
On Fri, 28 Mar 2003, Philipp Thomas wrote: > 3.0, as something like ie->_IIF(index_length) is really hard to read and not What you really mean is le32_to_cpu(ie->_IIF(index_length)) Yes, it's not very nice. How about this? ntfs_get_index_length(ie) Szaka |
|
From: Philipp T. <pt...@su...> - 2003-03-28 18:58:32
|
* Szakacsits Szabolcs (sz...@si...) [20030328 19:48]: > What you really mean is > > le32_to_cpu(ie->_IIF(index_length)) > > Yes, it's not very nice. How about this? > > ntfs_get_index_length(ie) IMO that's a much better :) That way you encapsulate the hairy details and make the code much more readable. Philipp -- Philipp Thomas <pt...@su...> SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nuremberg, Germany |
|
From: Szakacsits S. <sz...@si...> - 2003-04-23 09:56:05
|
On Fri, 28 Mar 2003, Philipp Thomas wrote: > * Szakacsits Szabolcs (sz...@si...) [20030328 19:48]: > > > What you really mean is > > > > le32_to_cpu(ie->_IIF(index_length)) > > > > Yes, it's not very nice. How about this? > > > > ntfs_get_index_length(ie) > > IMO that's a much better :) That way you encapsulate the hairy details and > make the code much more readable. Yes, _moreover_ it would make - compilation possible with older compilers - compilation possible with broken compilers - Richard Henderson happy - usage of unusual/unused/unreliable "ms-extension" option unneeded There is also a new bug submitted to the ODSL Kernel Bug Tracker, NTFS driver can't be compiled on the newest SuSE 8.2 with gcc 3.3-pre [default compiler?]. I didn't check it out but apparently it's also due to this never ending issue ... http://bugme.osdl.org/show_bug.cgi?id=607 Szaka |
|
From: Philipp T. <pt...@su...> - 2003-04-23 10:28:10
|
* Szakacsits Szabolcs (sz...@si...) [20030423 11:55]: > Yes, _moreover_ it would make > > - compilation possible with older compilers > - compilation possible with broken compilers > - Richard Henderson happy > - usage of unusual/unused/unreliable "ms-extension" option unneeded Yepp, all of them :) I'd recommend strongly recommend getting rid of the need to use the truly horrible ms-extensions and encapsulate the access to certain fields in either macros or inline functions, like you proposed for the index length. > There is also a new bug submitted to the ODSL Kernel Bug Tracker, NTFS > driver can't be compiled on the newest SuSE 8.2 with gcc 3.3-pre [default > compiler?]. Yes, 3.3-pre is the SuSE 8.2 default compiler. I didn't check it out but apparently it's also due to this > never ending issue ... > > http://bugme.osdl.org/show_bug.cgi?id=607 In part yes. I guess it just needs the patch to layout.h I proposed back then applied. I'll attach that patch to the osdl bug, thanks for pointing it out. Philipp -- Philipp Thomas <pt...@su...> SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nuremberg, Germany |
|
From: Anton A. <ai...@ca...> - 2003-04-23 15:00:16
|
Ok, I am truly fed up with wasting time over this so I give up! *sigh* Watch the BK ntfs-2.4 repository the first (probably easiest chunk) was just pushed there by me. Time permitting I will slowly do the rest in the same way. Best regards, Anton -- Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @) Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/ |
|
From: Szakacsits S. <sz...@si...> - 2003-03-27 16:27:06
|
On Thu, 27 Mar 2003, Philipp Thomas wrote: > Please smack the ntfs folk for me please. This is gross. Just a quick note, I've never liked it either. It's also a showstopper for some people and distribution ... Szaka |
