Bug 11033 - The macro DEBUG is exposed if header file ndr.h is included.
Summary: The macro DEBUG is exposed if header file ndr.h is included.
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: 4.2.0
Hardware: All All
: P5 major (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 10077
  Show dependency treegraph
 
Reported: 2015-01-06 14:10 UTC by Lukas Slebodnik
Modified: 2015-04-21 19:32 UTC (History)
6 users (show)

See Also:


Attachments
Test patch (651 bytes, patch)
2015-01-07 01:18 UTC, Jeremy Allison
no flags Details
v4-2-test patch (1.65 KB, patch)
2015-01-09 07:42 UTC, Andreas Schneider
asn: review? (ddiss)
martins: review+
gd: review+
Details
Extra patch for 4.2.next. (3.46 KB, patch)
2015-04-16 19:14 UTC, Jeremy Allison
asn: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Lukas Slebodnik 2015-01-06 14:10:47 UTC
It is common to use the name "DEBUG" for debug macros/functions in c/c++ projects. With the recent changes in samba 4.2, macro "DEBUG" is defined if public header file "ndr.h" is included in c/c++ module

Here is a part of build log from sssd:
In file included from /usr/include/samba-4.0/util/fault.h:27:0,
                 from /usr/include/samba-4.0/samba_util.h:62,
                 from /usr/include/samba-4.0/ndr.h:30,
                 from src/providers/ad/ad_gpo.c:52:
/usr/include/samba-4.0/util/debug.h:182:0: warning: "DEBUG" redefined
 #define DEBUG( level, body ) \
 ^
In file included from src/providers/ad/ad_gpo.c:38:0:
./src/util/util.h:126:0: note: this is the location of the previous definition
 #define DEBUG(level, format, ...) do { \
 ^
src/providers/ad/ad_gpo.c: In function 'ad_gpo_parse_map_option_helper':
src/providers/ad/ad_gpo.c:265:38: error: macro "DEBUG" passed 3 arguments, but takes just 2
               hash_error_string(hret));

Full log file:
https://kojipkgs.fedoraproject.org//work/tasks/9191/8539191/build.log
Comment 1 Lukas Slebodnik 2015-01-06 14:17:43 UTC
The change can be caused by
commit 8dac190ee1bc0e7f6d17eeca097f027fcaf584ed
Author: Martin Schwenke <martin@meltin.net>
Date:   Mon Sep 22 19:43:27 2014 +1000

    lib/util: Clean up includes for fault.c


The commit added public header file "lib/util/fault.h", which includes
'#include "debug.h"'

There isn't such problem with samba-devel-4.1.14
Comment 2 Adam Williamson 2015-01-06 21:46:37 UTC
Note, this is rather badly breaking Fedora Rawhide at present; it's stopping sssd from building, but sssd needs rebuilding against an updated library. This means you can't currently do a network install of most Fedora package sets, and nightly image composes are building. So, Samba devs who care about Fedora, please help :)
Comment 3 Jeremy Allison 2015-01-07 01:15:24 UTC
fault.c already includes debug.h

Does the build still work if

#include "debug.h"

is removed from fault.h ?
Comment 4 Jeremy Allison 2015-01-07 01:18:10 UTC
Created attachment 10586 [details]
Test patch

Can you check if this works for Fedora ?
Comment 5 Andreas Schneider 2015-01-07 07:48:46 UTC
Jeremy, If you look at the lines below where you removed debug.h you will notice that the SMB_ASSERT macro is defined at this position using the DEBUG macro. I guess it will not compile ...
Comment 6 Andreas Schneider 2015-01-07 08:49:08 UTC
I think the right approach would be to protect the macros ...

#ifndef DEBUG
#define DEBUG(level, body) ...
#endif

and prefix the function with samba_ and the enums etc. too.
Comment 7 Jeremy Allison 2015-01-07 16:47:30 UTC
(In reply to Andreas Schneider from comment #5)

Oh that's strange, 'cos I just recompiled it successfully :-). Must have missed something.
Comment 8 Jakub Hrozek 2015-01-08 15:05:41 UTC
(In reply to Andreas Schneider from comment #6)

Why are the DEBUG macros public at all? Are they considered public Samba API?

Wouldn't it be better to prefix anything that's exported in public headers with a samba-specific prefix?
Comment 9 Martin Schwenke 2015-01-08 18:26:11 UTC
Patch for master has been pushed to autobuild.  Will cherry-pick into 4.2 when it lands in master.
Comment 10 Andreas Schneider 2015-01-09 07:42:43 UTC
Created attachment 10603 [details]
v4-2-test patch
Comment 11 Martin Schwenke 2015-01-09 08:16:10 UTC
To avoid confusion, Jeremy's patch should be dropped, right?

Also, is just one review in addition to Andreas' sign-off sufficient to reassign to Karolin?
Comment 12 Andreas Schneider 2015-01-09 12:52:42 UTC
Karolin, please add the patch to 4.2.0. Thanks
Comment 13 Karolin Seeger 2015-01-14 21:00:43 UTC
(In reply to Andreas Schneider from comment #12)
Pushed to autobuild-v4-2-test.
Comment 14 Karolin Seeger 2015-01-16 20:10:43 UTC
Pushed to v4-2-test.
Closing out bug report.

Thanks!
Comment 15 Lukas Slebodnik 2015-03-31 20:30:17 UTC
The additional patch for this ticket was pushed to master few weeks ago.
Would it be possible to back port it to samba v4-2?

commit 9643a4b1ef2ada764f454ecc82aa6936217967fc
Author:     Lukas Slebodnik <lslebodn@redhat.com>
AuthorDate: Thu Mar 5 11:26:46 2015 +0100
Commit:     Jeremy Allison <jra@samba.org>
CommitDate: Wed Mar 11 18:47:22 2015 +0100

    lib/util: Include DEBUG macro in internal header files before samba_util.h
Comment 16 Karolin Seeger 2015-04-08 19:23:19 UTC
Re-assigning to Jeremy.
Comment 17 Jeremy Allison 2015-04-16 19:14:48 UTC
Created attachment 10959 [details]
Extra patch for 4.2.next.

If asn +1's it we'll get this into 4.2.2.
Comment 18 Andreas Schneider 2015-04-20 09:36:17 UTC
Comment on attachment 10959 [details]
Extra patch for 4.2.next.

LGTM
Comment 19 Andreas Schneider 2015-04-20 09:39:00 UTC
Karolin, please add the extra patch to the 4.2 branch. Thanks.
Comment 20 Karolin Seeger 2015-04-20 19:43:19 UTC
(In reply to Andreas Schneider from comment #19)
Pushed extra patch to autobuild-v4-2-test.
Comment 21 Lukas Slebodnik 2015-04-21 05:41:46 UTC
Thank you very much
Comment 22 Karolin Seeger 2015-04-21 19:01:11 UTC
(In reply to Karolin Seeger from comment #20)
Pushed to v4-2-test.
Closing out bug report.

Thanks!
Comment 23 Lukas Slebodnik 2015-04-21 19:32:12 UTC
Thank you very much