Hi Michael, On Mar 1 20:18, Michael Haubenwallner wrote: > Hi Corinna, > > On 02/23/2017 03:03 PM, Corinna Vinschen wrote: > > Hi Michael, > > > > I'm inclined to promote the "forkables" code to master. I just have a > > few points before we do that: > > > > - Revert bumping of CYGWIN_VERSION_SHARED_DATA. We only have to do that > > if the shared region changes in an incompatible way. But this is just > > adding a member to the end. > > Ok. > As long as properly aligned, even int-access should be atomic: > Is it ok to add the new member as 'char' rather than 'int'?
What about using a LONG? It allows atomic access and is the right type, should we find that we have to use Interlocked access at one point. > > - I'm looking a bit cross-eyed on the usage of forkables_needs and > > cygwin_shared->prefer_forkable_hardlinks. It seems to me as if the > > split between those two isn't quite right and the fact that both > > share information seems error prone. > > > > IMHO prefer_forkable_hardlinks should actually be the single marker > > for the per-installation state. After startup of the first process it > > should be "unknown" (0) by default. At startup it's set to one of > > > > "disabled" (-1) no NTFS or dir is missing > > "enabled" (+1) NTFS and dir exists > > > > That sets the state once and for all by the first Cygwin process in > > the system. > > The initial check now is moved to dll_list::forkable_ntnamesize(), > which is called by dll_list::alloc(). > > What about the renaming cygwin_shared->prefer_forkable_hardlinks > to cygwin_shared->forkable_hardlink_support? Good idea. > The new dll_list::forkables_supported() replaces the "unknown","impossible", > "disabled" values. I've thought about inlining into dll_init.h, but that > would require to include "shared_info.h": Is there a specific reason behind > dll_init.h not having any #include right now? History. At one point there was a rule set to reduce include-creep in header files and to move the includes to the .c and .cc files as much as possible. I think the driving factor at the time was compile time, which is pretty moot these days. I think it would not hurt to include obvious local dependencies directly from the affected header, but you could also just include shared_info.h prior ro dll_init.h in affected sources. AFAICS, 3 of 5 already inlcude shared_inof.h anyway. > > Consequentially, forkables_needs should only reflect the per-process > > states > > > > "needless" > > "needed" > > "created" > > > > - Shouldn't forkables_needs be renamed to forkables_needed? > > I've further simplified and replaced "enum forkables_needs" with > "bool forkables_created", because the "needless" value now is > implemented as "first fork tries without hardlinks". So as soon as > request_forkables() is entered, hardlinks aren't "needless" any more. ACK > > That's all. There are a few minor formatting issues, but they are > > negligible. > > Do you prefer another patch series with this patch applied as fixups? No, just send patches. I think keeping the history of changes is helpful in the long run. I'm going to apply the attached patch as is, and you just follow up with a char -> LONG patch, ok? Thanks, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat
signature.asc
Description: PGP signature