Uploaded image for project: 'ZooKeeper'
  1. ZooKeeper
  2. ZOOKEEPER-2905

Don't include `config.h` in `zookeeper.h`

VotersWatch issueWatchersCreate sub-taskLinkCloneUpdate Comment AuthorReplace String in CommentUpdate Comment VisibilityDelete Comments
    XMLWordPrintableJSON

Details

    • Bug
    • Status: Resolved
    • Major
    • Resolution: Fixed
    • None
    • 3.4.11, 3.5.4, 3.6.0
    • None
    • None
    • Linux-ish environments.

    Description

      In ZOOKEEPER-2841 I fixed the inclusion of project-specific porting changes that were included in the public headers, which then broke upstream projects (in my case, Mesos).

      Unfortunately, I inadvertently created the exact same problem for Linux (or really any system that uses Autotools), and it wasn't evident until the build was coupled with another project with the same problem. More specifically, when including ZooKeeper (with my changes) in Mesos, and including Google's Glog in Mesos, and building both with Autotools (which we also support), both packages define the pre-processor macro PACKAGE_VERSION, and so so publicly. This is defined in config.h by Autotools, and is not a problem unless included publicly.

      When refactoring, I saw two includes in zookeeper.h that instead of being guarded by e.g. #ifdef HAVE_SYS_SOCKET_H were guarded by #ifndef WIN32. Without realizing that I would create the exact same problem I was elsewhere fixing, I erroneously added #include "config.h" and guarded the includes "properly." But there is very good reasons not to do this (explained above).

      The patch to fix this is simple:

      diff --git a/src/c/include/zookeeper.h b/src/c/include/zookeeper.h
      index d20e70af4..b0bb09e3f 100644
      --- a/src/c/include/zookeeper.h
      +++ b/src/c/include/zookeeper.h
      @@ -21,13 +21,9 @@
      
       #include <stdlib.h>
      
      -#include "config.h"
      -
      -#ifdef HAVE_SYS_SOCKET_H
      +/* we must not include config.h as a public header */
      +#ifndef WIN32
       #include <sys/socket.h>
      -#endif
      -
      -#ifdef HAVE_SYS_TIME_H
       #include <sys/time.h>
       #endif
      
      diff --git a/src/c/src/zookeeper.c b/src/c/src/zookeeper.c
      index 220c57dc4..9b837f227 100644
      --- a/src/c/src/zookeeper.c
      +++ b/src/c/src/zookeeper.c
      @@ -24,6 +24,7 @@
       #define USE_IPV6
       #endif
      
      +#include "config.h"
       #include <zookeeper.h>
       #include <zookeeper.jute.h>
       #include <proto.h>
      

      I am opening pull requests in a few minutes to have this applied to branch 3.4 and 3.5.

      I'm sorry!

      Attachments

        Activity

          This comment will be Viewable by All Users Viewable by All Users
          Cancel

          People

            andschwa Andrew Schwartzmeyer
            andschwa Andrew Schwartzmeyer
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Slack

                Issue deployment