Quantcast

(fixup) [PATCH] forkables: use dynloaded dll's IndexNumber as dirname

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

(fixup) [PATCH] forkables: use dynloaded dll's IndexNumber as dirname

Michael Haubenwallner-2
Hi Corinna,

as realized during write of tl;dr draft for the topic/forkables branch,
>
> <damn-wrong reason="original directory may not exist any more">
>  * The temporary subdirectory name for a dynamically loaded dll is formed
>    using the original directory's NTFS-IndexNumber.
> </damn-wrong>
> https://cygwin.com/ml/cygwin-developers/2017-01/msg00000.html

here's the patch, intended as fixup for
>
> [PATCH 3/6] forkables: Create forkable hardlinks, yet unused.
> https://cygwin.com/ml/cygwin-developers/2016-12/msg00006.html

Thanks!
/haubi/

0001-forkables-use-dynloaded-dll-s-IndexNumber-as-dirname.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: (fixup) [PATCH] forkables: use dynloaded dll's IndexNumber as dirname

Corinna Vinschen-2
On Feb 10 15:08, Michael Haubenwallner wrote:

> Hi Corinna,
>
> as realized during write of tl;dr draft for the topic/forkables branch,
> >
> > <damn-wrong reason="original directory may not exist any more">
> >  * The temporary subdirectory name for a dynamically loaded dll is formed
> >    using the original directory's NTFS-IndexNumber.
> > </damn-wrong>
> > https://cygwin.com/ml/cygwin-developers/2017-01/msg00000.html
>
> here's the patch, intended as fixup for
> >
> > [PATCH 3/6] forkables: Create forkable hardlinks, yet unused.
> > https://cygwin.com/ml/cygwin-developers/2016-12/msg00006.html
Applied to the (rebased) topic/forkables branch.


Thanks,
Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: (fixup) [PATCH] forkables: use dynloaded dll's IndexNumber as dirname

Corinna Vinschen-2
In reply to this post by Michael Haubenwallner-2
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.

- 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.

  Consequentially, forkables_needs should only reflect the per-process
  states

    "needless"
    "needed"
    "created"

- Shouldn't forkables_needs be renamed to forkables_needed?

That's all.  There are a few minor formatting issues, but they are
negligible.


Thanks,
Corinna

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: (fixup) [PATCH] forkables: use dynloaded dll's IndexNumber as dirname

Michael Haubenwallner-2
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'?

> - 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?

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?

>   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.

> 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?

Thanks a lot!
/haubi/

0001-forkables-simplify-disabling-via-shm.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: (fixup) [PATCH] forkables: use dynloaded dll's IndexNumber as dirname

Corinna Vinschen-2
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 (836 bytes) Download Attachment
Loading...