Re: [PATCH] Fix git-pack-objects for 64-bit platforms

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Linus Torvalds <torvalds@xxxxxxxx> writes:

> On Thu, 11 May 2006, Dennis Stosberg wrote:
>> 
>> I am not sure whether an int cast or an int32_t cast is more
>> appropriate here.  An int is not guaranteed to be four bytes wide,
>> but I don't know of any modern platform where that's not the case.
>> On the other hand int32_t is not necessarily available before C99.
>> 
>> Any opinions?  I wonder why no one has hit this on x86_64...
>
> I think the "ntohl()" hides it. It loads a 64-bit value, but since x86-64 
> is little-endian, the low 32 bits are correct. The htonl() will then strip 
> the high bits and make it all be big-endian.

That sounds sensible.

Since I saw a patch that touches only one place, I thought I'd
better point this out...

There are a few more places that knows about this
((char*)base_pointer + (entry_count * 24)) magic in our code.

$ git grep -n -e '24  *\*' -e '\*  *24' master -- '*.c'

master:pack-objects.c:159:		long hl = *((long *)(index + 24 * i));

	This is yours.

master:sha1_file.c:447:	if (idx_size != 4*256 + nr * 24 + 20 + 20)
master:sha1_file.c:1148:	memcpy(sha1, (index + 24 * n + 4), 20);
master:sha1_file.c:1162:		int cmp = memcmp(index + 24 * mi + 4, sha1, 20);

	These three should be OK, I think.

master:sha1_file.c:1164:			e->offset = ntohl(*((int*)(index + 24 * mi)));

	This you might want to look at; I suspect it is what
	Linus suggests.

Also we _might_ have uglier magic that assumes the base_pointer to
be a pointer to a 4-byte integer and uses offset of multiple of
6 instead of 24, although I do not think it is likely.

I have to leave the keyboard in a few minutes so I cannot verify
nor fix them myself for the next 8 hours or so.  Sorry.

> And while I actually run a 64-bit big-endian machine myself (G5 ppc64), my 
> user space is all 32-bit by default, so it never showed up on linux-ppc64 
> either.
>
> Anyway, the correct type to use is "uint32_t" in this case. That's what 
> htonl() takes.

Is uint32_t guaranteed to be exactly 32-bit, or merely enough to
hold 32-bit?

-
: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]