Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

From: Marius Storm-Olsen <marius@trolltech.com>
Date: Sep 4, 2007, 11:21 UTC
Message-ID: <46DD3FC9.2020706@trolltech.com>
In-Reply-To: <46DD3943.8040403@eudaptics.com>

Johannes Sixt said the following on 04.09.2007 12:53:

Show 17 quoted lines

> Marius Storm-Olsen schrieb:
>> Johannes Sixt said the following on 04.09.2007 09:41:
>>> Thanks a lot! I've pushed it out in mingw.git's master.
>> Ops, already in master branch?
> 
> Yes, it looked so polished ;)
> 
>> http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f15974add93bdfa92775c77c00e7c65aefd42127 
> 
> Looks good, although you should now handle INVALID_HANDLE_VALUE at the 
> beginning of git_fstat() like this:
> 
> 	HANDLE fh = (HANDLE)_get_osfhandle(fd);
> 	if (fh == INVALID_HANDLE_VALUE)
> 		return -1;	/* errno has been set */
> 
> 	if (GetFileInformationByHandle(...

Actually, that's already handled. GetFileType will report FILE_TYPE_UNKNOWN (0), and GetLastError() will return a result != NO_ERROR (<-- so you can follow the codepath, but it actually returns ERROR_INVALID_HANDLE (6)) So, it will fall all the way through the function, and end up returning -1, with errno = EBADF.

(I use the
     case FILE_TYPE_UNKNOWN:
         if (GetLastError() != NO_ERROR)
             break;
construct, since the documentation says that an FILE_TYPE_UNKNOWN is 
still a _valid_ handle iff GetLastError() returns NO_ERROR. So, then 
we pass it on to the normal fstat function to let that figure out what 
we're dealing with)

Show 17 quoted lines

>> Ok, I can give it a performance test, but I tend to agree with David 
>> Kastrup there. It would be better if we rather fix the places where we 
>> check with the local timestamp instead; depending of course on how many 
>> places we actually do this.
>> We'll see how much the timezone conversion in the custom stat functions 
>> actually hurt us performance wise.
> 
> I'd make the decision on the grounds of a perfomance test. If it turns out 
> that the penalty is bearable, we should keep this stuff private to the MinGW 
> build. Otherwise, we would need MinGW specific code at the call sites 
> (unless we can hide the opposite conversion in some other wrapper function).
> 
> ... time passes ...
> 
> Ok, I just tested FileTimeToLocalFileTime() in a tight loop, and I can run 
> it 100,000,000 times per second. So I'm confident that there won't be any 
> noticable degradation with my proposed change.

Ok. I haven't done the performance test with Git yet, but we'll see. If it's not noticeable, I'll add it to all the timestamps we have.

-- 
.marius