Mailing-List: contact dev-help@zookeeper.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@zookeeper.apache.org
Date: Sat, 8 Jul 2017 22:11:00 +0000 (UTC)
From: "ASF GitHub Bot (JIRA)" <jira@apache.org>
To: dev@zookeeper.apache.org
Message-ID: <JIRA.13085619.1499467710000.200930.1499551860053@Atlassian.JIRA>
In-Reply-To: <JIRA.13085619.1499467710000@Atlassian.JIRA>
References: <JIRA.13085619.1499467710000@Atlassian.JIRA> <JIRA.13085619.1499467710989@jira-lw-us.apache.org>
Subject: [jira] [Commented] (ZOOKEEPER-2841) ZooKeeper public include files
 leak porting changes
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
archived-at: Sat, 08 Jul 2017 22:11:07 -0000


    [ https://issues.apache.org/jira/browse/ZOOKEEPER-2841?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16079342#comment-16079342 ] 

ASF GitHub Bot commented on ZOOKEEPER-2841:
-------------------------------------------

Github user andschwa commented on a diff in the pull request:

    https://github.com/apache/zookeeper/pull/306#discussion_r126291131
  
    --- Diff: src/c/include/winconfig.h ---
    @@ -1,196 +1,15 @@
    -/* Define to 1 if you have the <arpa/inet.h> header file. */
    -#undef HAVE_ARPA_INET_H
    -
    -/* Define to 1 if you have the <dlfcn.h> header file. */
    -#undef HAVE_DLFCN_H
    -
    -/* Define to 1 if you have the <fcntl.h> header file. */
    -#undef HAVE_FCNTL_H
    -
    -/* Define to 1 if you have the file `generated/zookeeper.jute.c'. */
    -#define HAVE_GENERATED_ZOOKEEPER_JUTE_C 1
    -
    -/* Define to 1 if you have the file `generated/zookeeper.jute.h'. */
    -#define HAVE_GENERATED_ZOOKEEPER_JUTE_H 1
    -
    -/* Define to 1 if you have the `getcwd' function. */
    -#undef HAVE_GETCWD
    -
    -/* Define to 1 if you have the `gethostbyname' function. */
    -#undef HAVE_GETHOSTBYNAME
    -
    -/* Define to 1 if you have the `gethostname' function. */
    -#define HAVE_GETHOSTNAME 1
    -
    -/* Define to 1 if you have the `getlogin' function. */
    -#undef HAVE_GETLOGIN
    -
    -/* Define to 1 if you have the `getpwuid_r' function. */
    -#undef HAVE_GETPWUID_R
    -
    -/* Define to 1 if you have the `gettimeofday' function. */
    -#undef HAVE_GETTIMEOFDAY
    -
    -/* Define to 1 if you have the `getuid' function. */
    -#undef HAVE_GETUID
    -
    -/* Define to 1 if you have the <inttypes.h> header file. */
    -#undef HAVE_INTTYPES_H
    -
    -/* Define to 1 if you have the `memmove' function. */
    -#undef HAVE_MEMMOVE
    -
    -/* Define to 1 if you have the <memory.h> header file. */
    -#undef HAVE_MEMORY_H
    -
    -/* Define to 1 if you have the `memset' function. */
    -#undef HAVE_MEMSET
    -
    -/* Define to 1 if you have the <netdb.h> header file. */
    -#undef HAVE_NETDB_H
    -
    -/* Define to 1 if you have the <netinet/in.h> header file. */
    -#undef HAVE_NETINET_IN_H
    -
    -/* Define to 1 if you have the `poll' function. */
    -#undef HAVE_POLL
    -
    -/* Define to 1 if you have the `socket' function. */
    -#undef HAVE_SOCKET
    -
    -/* Define to 1 if you have the <stdint.h> header file. */
    -#undef HAVE_STDINT_H
    -
    -/* Define to 1 if you have the <stdlib.h> header file. */
    -#define HAVE_STDLIB_H 1
    -
    -/* Define to 1 if you have the `strchr' function. */
    -#define HAVE_STRCHR 1
    -
    -/* Define to 1 if you have the `strdup' function. */
    -#define HAVE_STRDUP 1
    -
    -/* Define to 1 if you have the `strerror' function. */
    -#define HAVE_STRERROR 1
    -
    -/* Define to 1 if you have the <strings.h> header file. */
    -#undef HAVE_STRINGS_H
    -
    -/* Define to 1 if you have the <string.h> header file. */
    -#undef HAVE_STRING_H
    -
    -/* Define to 1 if you have the `strtol' function. */
    -#undef HAVE_STRTOL
    -
    -/* Define to 1 if you have the <sys/socket.h> header file. */
    -#undef HAVE_SYS_SOCKET_H
    -
    -/* Define to 1 if you have the <sys/stat.h> header file. */
    -#undef HAVE_SYS_STAT_H
    -
    -/* Define to 1 if you have the <sys/time.h> header file. */
    -#undef HAVE_SYS_TIME_H
    -
    -/* Define to 1 if you have the <sys/types.h> header file. */
    -#undef HAVE_SYS_TYPES_H
    -
    -/* Define to 1 if you have the <sys/utsname.h> header file. */
    -#undef HAVE_SYS_UTSNAME_H
    -
    -/* Define to 1 if you have the <unistd.h> header file. */
    -#undef HAVE_UNISTD_H
    -
    -/* Define to the sub-directory in which libtool stores uninstalled libraries.
    -   */
    -#define LT_OBJDIR
    -
    -/* Define to 1 if your C compiler doesn't accept -c and -o together. */
    -/* #undef NO_MINUS_C_MINUS_O */
    -
    -/* Name of package */
    -#define PACKAGE "c-client-src"
    -
    -/* Define to the address where bug reports for this package should be sent. */
    -#define PACKAGE_BUGREPORT "user@zookeeper.apache.org"
    -
    -/* Define to the full name of this package. */
    -#define PACKAGE_NAME "zookeeper C client"
    -
    -/* Define to the full name and version of this package. */
    -#define PACKAGE_STRING "zookeeper C client 3.5.0 win32"
    -
    -/* Define to the one symbol short name of this package. */
    -#define PACKAGE_TARNAME "c-client-src"
    -
    -/* Define to the home page for this package. */
    -#define PACKAGE_URL ""
    -
    -/* Define to the version of this package. */
    -#define PACKAGE_VERSION "3.5.0"
    --- End diff --
    
    It should be freshly rebased as of yesterday, but yeah I didn't catch the version update.


> ZooKeeper public include files leak porting changes
> ---------------------------------------------------
>
>                 Key: ZOOKEEPER-2841
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2841
>             Project: ZooKeeper
>          Issue Type: Bug
>          Components: c client
>         Environment: Windows 10 with Visual Studio 2017
>            Reporter: Andrew Schwartzmeyer
>            Assignee: Andrew Schwartzmeyer
>              Labels: windows
>
> The fundamental problem is that the port of the C client to Windows is now close to six years old, with very few updates. This port leaks a lot of changes that should be internal to ZooKeeper, and many of those changes are simply no longer relevant. The correct thing to do is attempt to refactor the Windows port for new versions of ZooKeeper, removing dead/unneeded porting code, and moving dangerous porting code to C files instead of public headers.
> Two primary examples of this problem are [ZOOKEEPER-2491|https://issues.apache.org/jira/browse/ZOOKEEPER-2491] and [MESOS-7541|https://issues.apache.org/jira/browse/MESOS-7541].
> The first issue stems from this ancient porting code:
> {noformat}
> #define snprintf _snprintf
> {noformat}
>  in [winconfig.h|https://github.com/apache/zookeeper/blob/ddf0364903bf7ac7cd25b2e1927f0d9d3c7203c4/src/c/include/winconfig.h#L179]. Newer versions of Windows C libraries define {{snprintf}} as a function, and so it cannot be redefined.
> The second issue comes from this undocumented change:
> {noformat}
> #undef AF_INET6
> {noformat}
> again in [winconfig.h|https://github.com/apache/zookeeper/blob/ddf0364903bf7ac7cd25b2e1927f0d9d3c7203c4/src/c/include/winconfig.h#L169] which breaks any library that uses IPv6 and {{winsock2.h}}.
> Furthermore, the inclusion of the following defines and headers causes terrible problems for consuming libraries, as they leak into ZooKeeper's public headers:
> {noformat}
> #define _CRT_SECURE_NO_WARNINGS
> #define WIN32_LEAN_AND_MEAN
> #include <Windows.h>
> #include <Winsock2.h>
> #include <winstdint.h>
> #include <process.h>
> #include <ws2tcpip.h>
> {noformat}
> Depending on the order that a project includes or compiles files, this may or may not cause {{WIN32_LEAN_AND_MEAN}} to become unexpectedly defined, and {{windows.h}} to be unexpectedly included. This problem is exacberated by the fact that the {{winsock2.h}} and {{windows.h}} headers are order-dependent (if you read up on this, you'll see that defining {{WIN32_LEAN_AND_MEAN}} was meant to work-around this).
> Going forward, porting changes should live next to where they are used, preferably in source files, not header files, so they remain contained.


--
This message was sent by Atlassian JIRA
(v6.4.14#64029)