diff mbox

[ovs-dev,RFC] ofctl: Fix nonstandard isatty on Windows

Message ID 20170608165120.GQ2820@ovn.org
State RFC
Headers show

Commit Message

Ben Pfaff June 8, 2017, 4:51 p.m. UTC 
On Thu, Jun 08, 2017 at 08:28:45AM +0000, Alin Serdean wrote:
> A lot of tests are failing, due to the open flow ports being outputted using
> names instead of numbers.
> i.e.: http://64.119.130.115/ovs/beb75a40fdc295bfd6521b0068b4cd12f6de507c/testsuite.dir/0464/testsuite.log.gz
> 
> The issues encountered above is because 'monitor' with 'detach' arguments are
> specified, that in turn will call 'close_standard_fds'
> (https://github.com/openvswitch/ovs/blob/master/lib/daemon-unix.c#L472)
> which will create a duplicate fd over '/dev/null' on Linux and 'nul' on Windows.
> 
> 'isatty' will be called on those FDs.
> What POSIX standard says:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/isatty.html
> 'The isatty() function shall test whether fildes, an open file descriptor,
> is associated with a terminal device.'
> What MSDN says:
> https://msdn.microsoft.com/en-us/library/f4s0ddew(VS.80).aspx
> 'The _isatty function determines whether fd is associated with a character
> device (a terminal, console, printer, or serial port).'
> 
> This patch adds another check using 'GetConsoleMode'
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms683167(v=vs.85).aspx
> which will fail if the handle pointing to the file descriptor is not associated
> to a console.
> 
> Signed-off-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>

Thanks for finding and fixing the problem.

This approach fixes a problem in one place only.  I see that there are
other calls to isatty() in the tree, so I would prefer to fix them all
in one place.  How about the following approach instead?  I have not
tested it.

Comments

Alin Serdean June 8, 2017, 5:23 p.m. UTC | #1 
> >
> > Signed-off-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
> 
> Thanks for finding and fixing the problem.
> 
> This approach fixes a problem in one place only.  I see that there are other
> calls to isatty() in the tree, so I would prefer to fix them all in one place.  How
> about the following approach instead?  I have not tested it.
[Alin Serdean] Thanks for the incremental. It makes more sense to just have it one place.
I think isatty might be already defined.
I will work around you incremental and add you as co-author.
Thanks for the quick review.
diff mbox

Patch

diff --git a/include/windows/unistd.h b/include/windows/unistd.h
index 2e9f0aef1647..00ca69762102 100644
--- a/include/windows/unistd.h
+++ b/include/windows/unistd.h
@@ -85,4 +85,19 @@  __inline long sysconf(int type)
     return value;
 }
 
+/* On Windows, a console is a specialized character device, and isatty() only
+ * reports whether a file description is a character device and thus reports
+ * that devices such as /dev/null are ttys.  This replacement avoids that
+ * problem. */
+#define isatty(fd) rpl_isatty(fd)
+static __inline int
+rpl_isatty(int fd)
+{
+    HANDLE h = (HANDLE) _get_osfhandle(fd);
+    DWORD st;
+    return (_isatty(STDOUT_FILENO)
+            && h != INVALID_HANDLE_VALUE
+            && GetConsoleMode(h, &st));
+}
+
 #endif /* unistd.h  */