[PATCH] Cygwin: make path_conv::isdevice() return false on socket files

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Cygwin: make path_conv::isdevice() return false on socket files

Ken Brown-6
As a result, socket files are no longer treated as lnk special files.
This prevents rename() from adding ".lnk" when renaming a socket file.

Remove a redundant !pc.issocket() from fhandler_disk_file::link().
---
 winsup/cygwin/fhandler_disk_file.cc | 4 ++--
 winsup/cygwin/path.h                | 2 +-
 winsup/cygwin/release/3.0.8         | 3 +++
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/fhandler_disk_file.cc b/winsup/cygwin/fhandler_disk_file.cc
index 193192762..fe4ee6971 100644
--- a/winsup/cygwin/fhandler_disk_file.cc
+++ b/winsup/cygwin/fhandler_disk_file.cc
@@ -1190,10 +1190,10 @@ fhandler_disk_file::link (const char *newpath)
   char new_buf[nlen + 5];
   if (!newpc.error)
     {
-      /* If the original file is a lnk special file (except for sockets),
+      /* If the original file is a lnk special file,
  and if the original file has a .lnk suffix, add one to the hardlink
  as well. */
-      if (pc.is_lnk_special () && !pc.issocket ()
+      if (pc.is_lnk_special ()
   && RtlEqualUnicodePathSuffix (pc.get_nt_native_path (),
  &ro_u_lnk, TRUE))
  {
diff --git a/winsup/cygwin/path.h b/winsup/cygwin/path.h
index 0c94c6152..5571218bd 100644
--- a/winsup/cygwin/path.h
+++ b/winsup/cygwin/path.h
@@ -179,7 +179,7 @@ class path_conv
   int issymlink () const {return path_flags & PATH_SYMLINK;}
   int is_lnk_symlink () const {return path_flags & PATH_LNK;}
   int is_known_reparse_point () const {return path_flags & PATH_REP;}
-  int isdevice () const {return dev.not_device (FH_FS) && dev.not_device (FH_FIFO);}
+  int isdevice () const {return dev.not_device (FH_FS) && dev.not_device (FH_FIFO) && !issocket ();}
   int isfifo () const {return dev.is_device (FH_FIFO);}
   int isspecial () const {return dev.not_device (FH_FS);}
   int iscygdrive () const {return dev.is_device (FH_CYGDRIVE);}
diff --git a/winsup/cygwin/release/3.0.8 b/winsup/cygwin/release/3.0.8
index e3734c9b7..11d11db6f 100644
--- a/winsup/cygwin/release/3.0.8
+++ b/winsup/cygwin/release/3.0.8
@@ -11,3 +11,6 @@ Bug Fixes
 
 - Fix a hang when opening a FIFO with O_PATH.
   Addresses: https://cygwin.com/ml/cygwin-developers/2019-06/msg00001.html
+
+- Don't append ".lnk" when renaming a socket file.
+  Addresses: https://cygwin.com/ml/cygwin/2019-07/msg00139.html
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: make path_conv::isdevice() return false on socket files

Corinna Vinschen-2
Hi Ken,

On Jul 18 20:02, Ken Brown wrote:
> As a result, socket files are no longer treated as lnk special files.
> This prevents rename() from adding ".lnk" when renaming a socket file.
>
> Remove a redundant !pc.issocket() from fhandler_disk_file::link().

I see what you're doing here, but it's totally non-obvious from the
commit message why this fixes the problem and doesn't introduce weird
side-effects.  Changing isdevice() also changes the definition of
is_auto_device(), which is used in symlink_worker().  

To ease the pain during later bisecting session, it would be kind to
explain detailed why the problem occurs and why your patch is the right
thing to do.

An editorial note: While looking into your patch it occured to me that
it would be about time to go over all the is***device() methods and
clean up the mess.  E.g., is_fs_device() is used by is_lnk_special()
only, is_auto_device() doesn't have much meaning, some funcs have
underscores, some don't.


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: make path_conv::isdevice() return false on socket files

Ken Brown-6
On 7/19/2019 4:28 AM, Corinna Vinschen wrote:
> I see what you're doing here, but it's totally non-obvious from the
> commit message why this fixes the problem and doesn't introduce weird
> side-effects.

Thanks.  I was pretty careless with this patch.  There's a new patch series on
the way that (I hope) does it right.

> An editorial note: While looking into your patch it occured to me that
> it would be about time to go over all the is***device() methods and
> clean up the mess.  E.g., is_fs_device() is used by is_lnk_special()
> only, is_auto_device() doesn't have much meaning,

I've removed is_fs_device() and is_auto_device()

> some funcs have
> underscores, some don't.

The convention seems to be that is<something> uses underscores if and only if
"something" is a single word.  The only exception I saw is isctty_capable.  I
didn't bother changing this, but I could if you want me to.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: make path_conv::isdevice() return false on socket files

Ken Brown-6
On 7/20/2019 6:53 PM, Ken Brown wrote:

> On 7/19/2019 4:28 AM, Corinna Vinschen wrote:
>> I see what you're doing here, but it's totally non-obvious from the
>> commit message why this fixes the problem and doesn't introduce weird
>> side-effects.
>
> Thanks.  I was pretty careless with this patch.  There's a new patch series on
> the way that (I hope) does it right.
>
>> An editorial note: While looking into your patch it occured to me that
>> it would be about time to go over all the is***device() methods and
>> clean up the mess.  E.g., is_fs_device() is used by is_lnk_special()
>> only, is_auto_device() doesn't have much meaning,
>
> I've removed is_fs_device() and is_auto_device()
>
>> some funcs have
>> underscores, some don't.
>
> The convention seems to be that is<something> uses underscores if and only if
> "something" is a single word.
                  ^
                 not

>  The only exception I saw is isctty_capable.  I
> didn't bother changing this, but I could if you want me to.
>
> Ken
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: make path_conv::isdevice() return false on socket files

Brian Inglis
On 2019-07-20 19:46, Ken Brown wrote:

> On 7/20/2019 6:53 PM, Ken Brown wrote:
>> On 7/19/2019 4:28 AM, Corinna Vinschen wrote:
>>> I see what you're doing here, but it's totally non-obvious from the
>>> commit message why this fixes the problem and doesn't introduce weird
>>> side-effects.
>> Thanks.  I was pretty careless with this patch.
>> There's a new patch series on the way that (I hope) does it right.
>>> An editorial note: While looking into your patch it occured to me that
>>> it would be about time to go over all the is***device() methods and
>>> clean up the mess.  E.g., is_fs_device() is used by is_lnk_special()
>>> only, is_auto_device() doesn't have much meaning,
>> I've removed is_fs_device() and is_auto_device()
>>> some funcs have underscores, some don't.
>> The convention seems to be that is<something> uses underscores if and only if
>> "something" is a single word.
>                ^
>               not
>> The only exception I saw is isctty_capable.
>> I didn't bother changing this, but I could if you want me to.

Anything beginning is or to followed by a lower case letter may be used by the
(library) implementation and may be considered reserved: best to interpose an
underscore as systems with better language support inc. BSDs are adding classes.

--
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: make path_conv::isdevice() return false on socket files

Ken Brown-6
On 7/21/2019 3:15 AM, Brian Inglis wrote:
> Anything beginning is or to followed by a lower case letter may be used by the
> (library) implementation and may be considered reserved: best to interpose an
> underscore as systems with better language support inc. BSDs are adding classes.

I assume you're referring to the POSIX name space rules, as in Section 2.2.2 of
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html.  I
don't see how that's related to the present discussion (identifiers used in
classes internal to Cygwin).

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: make path_conv::isdevice() return false on socket files

Brian Inglis
On 2019-07-21 08:25, Ken Brown wrote:
> On 7/21/2019 3:15 AM, Brian Inglis wrote:
>> Anything beginning is or to followed by a lower case letter may be used by the
>> (library) implementation and may be considered reserved: best to interpose an
>> underscore as systems with better language support inc. BSDs are adding classes.
>
> I assume you're referring to the POSIX name space rules, as in Section 2.2.2 of
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html.  I
> don't see how that's related to the present discussion (identifiers used in
> classes internal to Cygwin).

Software hygiene for 9899 (see 7.1.4 Future Library Directions), POSIX, and C++
(anything in the std namespace) conformance: 9899 and others are normative
references in both POSIX and C++.

"Also, POSIX.1-2017 defines symbols that are not permitted by other standards to
appear in those headers without some control on the visibility of those symbols."

Normative References -
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap01.html#tag_01_03 -
includes FORTRAN, 646 ASCII and 10646 UTF8 BMP, 8601 dates/times, and 4217
currencies/funds!

So we can't use anything that is currently or might be used in the future by any
compiler or library.

Builds break unexpectedly, and sometimes more widely than you might think, when
a header included perhaps indirectly, declares or defines something you have
improperly used.

BTDT Got The T-Shirt - structure member name used in a number of structures in
app headers included in and used in almost all sources, toasted by a macro
definition in a std header required in almost all those sources - as a core dev
I got to do the cleanup! ;^>

--
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.