Re: Make the git codebase thread-safe

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

 



Even if we make that change to use TLS for this case, the
implementation of pread() will still change in such a way that the
semantics of pread() are different on Windows.  Is that ok?

Just to summarize, here's the viable approaches I've seen discussed so far:

1) Use _WINVER at compile time to select either a thread-safe or
non-thread-safe implementation of pread.  This is the easiest possible
code change, but would necessitate 2 binary distributions of git for
windows.
2) Use TLS as you suggest and have one fd per pack thread.  Probably
the most complicated code change (at least for me, being a first-time
contributor)
3) Use Karsten's suggested implementation from earlier in the thread.
Seems to work, but it's a little confusing from a readability
standpoint since the implementation is not-thread safe except in this
specific usage context.

On Fri, Feb 14, 2014 at 4:56 PM, Duy Nguyen <pclouds@xxxxxxxxx> wrote:
> On Sat, Feb 15, 2014 at 7:50 AM, Stefan Zager <szager@xxxxxxxxxx> wrote:
>> On Fri, Feb 14, 2014 at 4:45 PM, Duy Nguyen <pclouds@xxxxxxxxx> wrote:
>>> On Sat, Feb 15, 2014 at 2:16 AM, Zachary Turner <zturner@xxxxxxxxxxxx> wrote:
>>>> (Gah, sorry if you're receiving multiple emails to your personal
>>>> addresses, I need to get used to manually setting Plain-text mode
>>>> every time I send a message).
>>>>
>>>> For the mixed read, we wouldn't be looking for another caller of
>>>> pread() (since it doesn't care what the file pointer is), but instead
>>>> a caller of read() or lseek() (since those do depend on the current
>>>> file pointer).  In index-pack.c, I see two possible culprits:
>>>>
>>>> 1) A call to xread() from inside fill()
>>>> 2) A call to lseek in parse_pack_objects()
>>>>
>>>> Do you think these could be related?  If so, maybe that opens up some
>>>> other solutions?
>>>
>>> For index-pack alone, what's wrong with open one file handle per thread?
>>
>> Nothing wrong with that, except that it would mean either using
>> thread-local storage (which the code doesn't currently use); or
>> plumbing pack_fd through the call stack, which doesn't sound very fun.
>
> Current code does use thread-local storage (struct thread_local and
> get_thread_data). Adding a new file handle when NO_THREAD_SAFE_PREAD
> is defined is simpler imo.
> --
> Duy
--
To unsubscribe from this list: 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]