zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (ZOOKEEPER-2841) ZooKeeper public include files leak porting changes
Date Fri, 07 Jul 2017 22:57:00 GMT

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

ASF GitHub Bot commented on ZOOKEEPER-2841:

GitHub user andschwa opened a pull request:


    ZOOKEEPER-2841: ZooKeeper public include files leak porting changes

    This PR includes the patches [ZOOKEEPER-2756](https://issues.apache.org/jira/browse/ZOOKEEPER-2756)
and [ZOOKEEPER-2841](https://issues.apache.org/jira/browse/ZOOKEEPER-2841), and can supplant
PR #255.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/andschwa/zookeeper ZOOKEEPER-2841

Alternatively you can review and apply these changes as the patch at:


To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #306
commit 187ce8acc1707a0dd20752b624a3fa5648706f97
Author: Andrew Schwartzmeyer <andrew@schwartzmeyer.com>
Date:   2017-04-10T23:12:40Z

    ZOOKEEPER-2756: Add CMake build system for better cross-platform support
    This notably lacks Solaris and libtool support.
    Almost everything else from Autotools has been ported,
    including header/function/library checks, and all targets
    (zookeeper, hashtable, cli, load_gen, and tests).
    Both Linux and Windows are supported.
    The primary work involved (other than the writing of `CMakeLists.txt`)
    was transitioning the hand-written `winconfig.h` to an
    auto-generated `config.h` file, and guarding code with `#ifdef
    HAVE_FEATURE`. The `cmake_config.h.in` template was modeled after
    the Autotools config file so that the feature guards share the same
    While `load_gen.c` looks at first glance as if it were ported to Windows,
    it never actually was, so the erroneous `#include "win32port.h"` was
    removed, and the target is not built on Windows.
    There are existent warnings which this patch did not attempt to fix.
    A guard was placed around `#define snprintf` in order to enable
    compiling with modern versions of MSVC.
    Fixed DLL_EXPORT and USE_STATIC_LIB redefinition.
    As this commit deprecates (and breaks) the previously existing Visual
    Studio solutions and projects, they've been removed.
    Building tests on Windows is still not supported.

commit 22a378ae2a7c30657326b6a602b62116ab7c050a
Author: Andrew Schwartzmeyer <andrew@schwartzmeyer.com>
Date:   2017-07-07T22:35:37Z

    ZOOKEEPER-2841: ZooKeeper public include files leak porting changes
    This patch refactors the Windows port of the C client. Notably, it moves
    as much porting code as possible out of the publicly included
    `winconfig.h` header and into the relevant source files. This also
    removes `winstdint.h` as it has been replaced by `<stdint.h>`. It fixes
    problems for upstream libraries by removing `#undef AF_INET6` and
    `#include <windows.h>`.
    Future work should resolve the `ACL` / `ZKACL` conflict, and potentially
    remove `include/winconfig.h` altogether.


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

View raw message