(fixup) [PATCH] forkables: use dynloaded dll's IndexNumber as dirname
Corinna Vinschen
corinna-cygwin@cygwin.com
Thu Mar 2 10:35:00 GMT 2017
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-patches/attachments/20170302/ae8bda17/attachment.sig>
More information about the Cygwin-patches
mailing list