Re: PATCH: dlls/cabinet/cabextract.c and off_t
On Thursday 26 December 2002 02:48 pm, Gerald Pfeifer wrote:
If we really want to use off_t in this file (instead of cab_off_t) we need to #include sys/types.h, or we'll break non-Linux systems.
Gerald
ChangeLog: #include <sys/types.h> to obtain off_t.
actually I don't think we really want to use off_t. The relationship to off_t is historical: cabextract (the unix executable, which I ported) used off_t because it used the fopen/fseek/fgetpos/etc family of functions. Wanting to depend only on kernel32, I ported these to use windowsy api's like CreateFile/SetFilePointer/etc, which take LONG arguments, IIRC. My memory on this is already a bit hazy, but I think I created cab_off_t to save myself keystrokes -- I basically was able to go through much of the code and just turn "off_t"'s into "cab_off_t"'s. Initially I tried to support huge (64 bit offset) files -- cab_off_t was going to be a struct with two LONGS -- but later I decided I was too lazy, since this would have required a substantial reworking of the logic, some of which I didn't even take the time to fully understand. There's a fair amount of math in there and in some places off_t's got passed around, converted to other types, etc.... so I decided I should bang out the port first and worry about the finer details later. The easy version of the right fix is probably just to rename "cab_off_t" to something less confusing (i.e., cab_file_offset). The fancy fix would be to support huge files, which might be a bit of a project. -- gmt "It does not take a majority to prevail ... but rather an irate, tireless minority, keen on setting brushfires of freedom in the minds of men." --Samuel Adams, Patriot
On Thu, 26 Dec 2002, Greg Turner wrote:
If we really want to use off_t in this file (instead of cab_off_t) we need to #include sys/types.h, or we'll break non-Linux systems. actually I don't think we really want to use off_t. [...] The easy version of the right fix is probably just to rename "cab_off_t" to something less confusing (i.e., cab_file_offset). The fancy fix would be to support huge files, which might be a bit of a project.
Ah, I see. Short to medium term, do you agree with my fix (#include <sys/types.h>) so that Wine also builds on non-Linux systems? Gerald -- Gerald "Jerry" pfeifer(a)dbai.tuwien.ac.at http://www.pfeifer.com/gerald/
On Friday 27 December 2002 04:35 am, Gerald Pfeifer wrote:
On Thu, 26 Dec 2002, Greg Turner wrote:
If we really want to use off_t in this file (instead of cab_off_t) we need to #include sys/types.h, or we'll break non-Linux systems.
actually I don't think we really want to use off_t. [...] The easy version of the right fix is probably just to rename "cab_off_t" to something less confusing (i.e., cab_file_offset). The fancy fix would be to support huge files, which might be a bit of a project.
Ah, I see.
Short to medium term, do you agree with my fix (#include <sys/types.h>) so that Wine also builds on non-Linux systems?
Gerald
Maybe. I don't object, since it should be harmless to add this, but I seem to be missing the point of why sys/types.h is needed at all. cab_off_t is just typedef'ed to UINT32, which should compile just fine with only winnt.h, I think. Is there a particular scenario you are encountering that fails? If so, what is the compile error? -- gmt "It does not take a majority to prevail ... but rather an irate, tireless minority, keen on setting brushfires of freedom in the minds of men." --Samuel Adams, Patriot
On Fri, 27 Dec 2002, Greg Turner wrote:
If we really want to use off_t in this file (instead of cab_off_t) we need to #include sys/types.h, or we'll break non-Linux systems. Short to medium term, do you agree with my fix (#include <sys/types.h>) so that Wine also builds on non-Linux systems? Maybe. I don't object, since it should be harmless to add this, but I seem to be missing the point of why sys/types.h is needed at all. cab_off_t is just typedef'ed to UINT32, which should compile just fine with only winnt.h, I think. Is there a particular scenario you are encountering that fails? If so, what is the compile error?
The following line is the problem: fol->offset[0] = base_offset + (off_t) EndGetI32(buf+cffold_DataOffset); ^^^^^ Here we use off_t (which requires sys/types.h), that's why I wrote "If we really want to use off_t in this file. Should this simply become cab_off_t? Gerald -- Gerald "Jerry" pfeifer(a)dbai.tuwien.ac.at http://www.pfeifer.com/gerald/
On Friday 27 December 2002 01:49 pm, Gerald Pfeifer wrote:
On Fri, 27 Dec 2002, Greg Turner wrote:
If we really want to use off_t in this file (instead of cab_off_t) we need to #include sys/types.h, or we'll break non-Linux systems.
Short to medium term, do you agree with my fix (#include <sys/types.h>) so that Wine also builds on non-Linux systems?
Maybe. I don't object, since it should be harmless to add this, but I seem to be missing the point of why sys/types.h is needed at all. cab_off_t is just typedef'ed to UINT32, which should compile just fine with only winnt.h, I think. Is there a particular scenario you are encountering that fails? If so, what is the compile error?
The following line is the problem:
fol->offset[0] = base_offset + (off_t) EndGetI32(buf+cffold_DataOffset); ^^^^^
Here we use off_t (which requires sys/types.h), that's why I wrote "If we really want to use off_t in this file. Should this simply become cab_off_t?
Bingo. Although I never formally grepped for it, I made every attempt to convert all off_t's to cab_off_t's or signed integers as appropriate. So, yes, this is most certainly an error on my part, and your fix is likely the correct one. Out of curiosity, what is your platform? I am slightly surprised that off_t compiles at all without sys/types.h, even on linux... Would you be so kind as to change that to cab_off_t, and then run the compile again to make sure that we've really eliminated all the off_t's in there? Let me know your results and I will submit a patch including this, and the renaming I discussed before. A random thought: It's notable, that cab_off_t is unsigned, whereas off_t is signed. I began, but did not finish, the process of pouring over the source to ensure this is a nonissue. Perhaps, I might sleep easier if I just turned it back to a signed INT32, although if I recall, I used to think I had a good reason not to do that. Before I submit I'll take a look at that once more, as well. Thanks for spotting this, -- gmt "It does not take a majority to prevail ... but rather an irate, tireless minority, keen on setting brushfires of freedom in the minds of men." --Samuel Adams, Patriot
On Fri, 27 Dec 2002, Greg Turner wrote:
Here we use off_t (which requires sys/types.h), that's why I wrote "If we really want to use off_t in this file. Should this simply become cab_off_t? Bingo. [...] So, yes, this is most certainly an error on my part, and your fix is likely the correct one. Out of curiosity, what is your platform? I am slightly surprised that off_t compiles at all without sys/types.h, even on linux...
I was seeing this on FreeBSD; on GNU/Linux you probably don't see that because sys/types.h and others are implicitly #included by lots of other headers (which is somewhat non-standard, though not forbidden).
Would you be so kind as to change that to cab_off_t, and then run the compile again to make sure that we've really eliminated all the off_t's in there? Let me know your results and I will submit a patch including this, and the renaming I discussed before.
It works fine. I am submitting this to wine-patches, so that Alexandre can unbreak the build as soon as possible, though of course of full fix like the one you mentioned would be better. ;-) Thanks for working on this with me! Gerald Index: cabextract.c =================================================================== RCS file: /home/wine/wine/dlls/cabinet/cabextract.c,v retrieving revision 1.1 diff -u -3 -p -r1.1 cabextract.c --- cabextract.c 19 Dec 2002 21:16:56 -0000 1.1 +++ cabextract.c 29 Dec 2002 22:52:16 -0000 @@ -472,7 +472,7 @@ BOOL cabinet_read_entries(struct cabinet } fol->cab[0] = cab; - fol->offset[0] = base_offset + (off_t) EndGetI32(buf+cffold_DataOffset); + fol->offset[0] = base_offset + (cab_off_t) EndGetI32(buf+cffold_DataOffset); fol->num_blocks = EndGetI16(buf+cffold_NumBlocks); fol->comp_type = EndGetI16(buf+cffold_CompType);
participants (2)
-
Gerald Pfeifer -
Greg Turner