rpmgit.h prototype clashes with OpenIndiana headers

[Tim Mooney]

Building rpm 5.4.12 on x86_64-pc-solaris2.11, OpenIndiana oi151a8. I'm using the Oracle Studio 12.3 compilers, but this issue would affect compilation with gcc too.

When building rpmgit.c, compilation fails with:

gmake[4]: Entering directory `/local/src/RPM/BUILD/rpm-5.4.12/rpmio'
source='rpmgit.c' object='rpmgit.lo' libtool=yes \
        DEPDIR=.deps depmode=none /bin/sh ../depcomp \
        /bin/sh ../libtool --tag=CC --mode=compile cc -m64 -DHAVE_CONFIG_H -I. -I.. -I. -I.. -I../build -I../lib -I../lib -I../rpmdb -I../rpmio -I../misc -I../beecrypt/include -I../beecrypt/include -I../beecrypt -I../beecrypt -I../popt -I../popt -D__FUNCTION__=__func__ -I/local/gnu/include -I/local/include -I/local/include/neon -I/local/include/beecrypt -I/local/openssl/include -DRPM_OS_SOLARIS=021100 -I/local/include -I/local/include/db60 -I/usr/include/pcre -xopenmp -Xa -xc99=all -g -mt -KPIC -D__FUNCTION__=__func__ -m64 -I/local/include -I/local/include/neon -I/local/include/beecrypt -I/local/openssl/include -D_GNU_SOURCE -D_REENTRANT -c -o rpmgit.lo rpmgit.c
libtool: compile: cc -m64 -DHAVE_CONFIG_H -I. -I.. -I. -I.. -I../build -I../lib -I../lib -I../rpmdb -I../rpmio -I../misc -I../beecrypt/include -I../beecrypt/include -I../beecrypt -I../beecrypt -I../popt -I../popt -D__FUNCTION__=__func__ -I/local/gnu/include -I/local/include -I/local/include/neon -I/local/include/beecrypt -I/local/openssl/include -DRPM_OS_SOLARIS=021100 -I/local/include -I/local/include/db60 -I/usr/include/pcre -xopenmp -Xa -xc99=all -g -mt -KPIC -D__FUNCTION__=__func__ -m64 -I/local/include -I/local/include/neon -I/local/include/beecrypt -I/local/openssl/include -D_GNU_SOURCE -D_REENTRANT -c rpmgit.c -KPIC -DPIC -o .libs/rpmgit.o
"./rpmgit.h", line 171: syntax error before or at: 0x00000008
"./rpmgit.h", line 171: warning: undefined or missing type for: void
"./rpmgit.h", line 180: syntax error before or at: 0x00000020
"./rpmgit.h", line 180: warning: undefined or missing type for: void

cc: acomp failed for rpmgit.c
gmake[4]: *** [rpmgit.lo] Error 1
gmake[4]: Leaving directory `/local/src/RPM/BUILD/rpm-5.4.12/rpmio'

The problem is that rpmgit.h uses very short names for some of the arguments in the function prototypes. In particular, _C and _R.

A couple of the headers on SVR4-derived systems have the bad habit of using #define with very short names, and in this case _C and _R from /usr/include/iso/ctype_iso.h has defines in place for _C and _R:

#define _U 0x00000001 /* Upper case */
#define _L 0x00000002 /* Lower case */
#define _N 0x00000004 /* Numeral (digit) */
#define _S 0x00000008 /* Spacing character */
#define _P 0x00000010 /* Punctuation */
#define _C 0x00000020 /* Control character */
#define _B 0x00000040 /* Blank */
#define _X 0x00000080 /* heXadecimal digit */

The easy fix is to alter the function prototypes to use longer argument names that do not conflict with any of the poorly-choosen pollution in the system headers.

A potential patch is attached. Only _C and _R need to change to avoid the class, but I changed all of them to be a little longer for consistency's sake.

[Jeff Johnson (n3npq)]

The patch looks fine for a rather mundane problem

Meanwhile, would using __C instead of _C (i.e. 2 underscores)
solve the problem? I'd like the prototypes and functions to
agree if at all possible.

Note that its not at all clear whether RPM+GIT using --with-libgit2
is maintainable/supportable. This is a build directly from a a git
checkout with no guarantee whatsoever of API/ABI compatibility.

rpm git.c has broken several times already due to changes in the libgit2 library.

[devzero2000 (pinto-elia)]

"In addition to the names documented in this manual, reserved names include all external identifiers (global functions and variables) that begin with an underscore (‘_’) and all identifiers regardless of use that begin with either two underscores or an underscore followed by a capital letter are reserved names. This is so that the library and header files can define functions, variables, and macros for internal purposes without risk of conflict with names in user programs."

http://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html

and according to the C Standard, section 7.1.3 [ISO/IEC 9899:2011]. See also https://www.securecoding.cert.org/confluence/display/seccode/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier

So neither _Commit nor __C are good choices, imho.

Of course this is a consideration very tedious and academic.

[devzero2000 (pinto-elia)]

Changed rpmgit.h and rpmgit.c :

from _R to Repo
from _C to Commit
from _H to Header.

I think this is ok (but check please).

*******************************

Index: rpmgit.c
===================================================================
RCS file: /v/rpm/cvs/rpm/rpmio/rpmgit.c,v
retrieving revision 2.1.2.32
diff -u -r2.1.2.32 rpmgit.c
--- rpmgit.c 15 Jul 2013 22:11:42 -0000 2.1.2.32
+++ rpmgit.c 30 Aug 2013 13:49:05 -0000
@@ -305,10 +305,10 @@
     }
 }

-void rpmgitPrintCommit(rpmgit git, void * _C, void * _fp)
+void rpmgitPrintCommit(rpmgit git, void * Commit, void * _fp)
 {
     FILE * fp = (FILE *) _fp;
- git_commit * C = _C;
+ git_commit * C = Commit;
     unsigned Pcnt;
     unsigned i;

@@ -378,10 +378,10 @@

 }

-void rpmgitPrintHead(rpmgit git, void * _H, void * _fp)
+void rpmgitPrintHead(rpmgit git, void * Header, void * _fp)
 {
     FILE * fp = (_fp ? _fp : stderr);
- git_reference * H = (_H ? _H : git->H);
+ git_reference * H = (Header ? Header : git->H);
     git_reference * Hresolved = NULL;
     int xx;

@@ -412,10 +412,10 @@

 }

-void rpmgitPrintRepo(rpmgit git, void * _R, void * _fp)
+void rpmgitPrintRepo(rpmgit git, void * Repo, void * _fp)
 {
     FILE * fp = (_fp ? _fp : stderr);
- git_repository * R = _R;
+ git_repository * R = Repo;
     const char * fn;

 if (_rpmgit_debug >= 0) return;
Index: rpmgit.h
===================================================================
RCS file: /v/rpm/cvs/rpm/rpmio/rpmgit.h,v
retrieving revision 2.1.2.17
diff -u -r2.1.2.17 rpmgit.h
--- rpmgit.h 8 Jul 2013 04:44:46 -0000 2.1.2.17
+++ rpmgit.h 30 Aug 2013 13:49:05 -0000
@@ -177,16 +177,16 @@
 void rpmgitPrintTree(void * _T, void * _fp)
        /*@*/;

-void rpmgitPrintCommit(rpmgit git, void * _C, void * _fp)
+void rpmgitPrintCommit(rpmgit git, void * Commit, void * _fp)
        /*@*/;

 void rpmgitPrintTag(rpmgit git, void * _tag, void * _fp)
        /*@*/;

-void rpmgitPrintHead(rpmgit git, void * _H, void * _fp)
+void rpmgitPrintHead(rpmgit git, void * Header, void * _fp)
        /*@*/;

-void rpmgitPrintRepo(rpmgit git, void * _R, void * _fp)
+void rpmgitPrintRepo(rpmgit git, void * Repo, void * _fp)
        /*@*/;

 int rpmgitInit(rpmgit git)

[Jeff Johnson (n3npq)]

There are 2 issues here:

1) an accidental name collision in OpenSolaris with some pre-existing symbols

2) a design issue for the rpmio/rpmgit.h API which is under active development

I used "void *_C" markers for arguments in prototypes to mark interfaces that
exposed arguments that either need to be exposed with an rpmio specific typedef
in functions that may need (otoh) to be made opaque/private by redesigning
the API (which I am actively working on).

The solution to the accidental name collision in 1) is trivial, the solution to 2) is
far more complicated. There is no useful RPM functionality building --with-libgit2 that
is either gained or lost atm. Meanwhile there is a great deal of confusion (for me) if
symbols start changing whilke the code is being developed when I do not yet
know what the proper API is going to be, with exposed additional RPM specific typedefs,
or removing the function entirely, particularly when there is no RPM functionality that
is useful or important yet.

[Tim Mooney (tim-mooney)]

Sorry I missed updating the prototypes in rpmgit.c; having the argument names match rpmgit.h makes perfect sense and I'll keep that in mind for any similar patches I might submit in the future. I see Pinto covered that commit already.

I think that using __C and __R would probably work, though I haven't tried it yet. If that's a preferable change to the current changes, let me know and I'll test it and report back.

[Jeff Johnson (n3npq)]

No worries: adding 3 underscores will "work" against all the known problems.

(aside)
I don't expect any serious interest in RPM+GIT as a "production" feature
until March 2015 based on hysterical experience with new RPM "features":
my rule of thumb for adding new "features" is ~3y and the time lime for
deployment is likely longer now because of declining participation in FL/OSS
development.

Its also quite risky compiling against active development: RPM+GIT breaks
somehow every other month or so.

[devzero2000 (pinto-elia)]

The patch in comment #4 changed.

The last is this

RPM Package Manager, CVS Repository
  http://rpm5.org/cvs/
  ____________________________________________________________________________

  Server: rpm5.org Name: Jeff Johnson
  Root: /v/rpm/cvs Email: <email address hidden>
  Module: rpm Date: 30-Aug-2013 19:43:07
  Branch: rpm-5_4 Handle: 2013083017430600

  Modified files: (Branch: rpm-5_4)
    rpm/rpmio rpmgit.c rpmgit.h

  Log:
    - fix: avoid symbol collisions on OopenSolaris AND be conformant with
    C 2011 AND mark tenative rpmgit symbols in a developing API
    consistently.

  Summary:
    Revision Changes Path
    2.1.2.34 +12 -12 rpm/rpmio/rpmgit.c
    2.1.2.19 +6 -6 rpm/rpmio/rpmgit.h
  ____________________________________________________________________________

  patch -p0 <<'@@ .'
  Index: rpm/rpmio/rpmgit.c
  ============================================================================
  $ cvs diff -u -r2.1.2.33 -r2.1.2.34 rpmgit.c
  --- rpm/rpmio/rpmgit.c 30 Aug 2013 14:34:42 -0000 2.1.2.33
  +++ rpm/rpmio/rpmgit.c 30 Aug 2013 17:43:06 -0000 2.1.2.34
  @@ -190,10 +190,10 @@
   fprintf(fp, " %s", _b);
   }

  -void rpmgitPrintSig(const char * msg, const void * _S, void * _fp)
  +void rpmgitPrintSig(const char * msg, const void * ___S, void * _fp)
   {
       FILE * fp = (_fp ? _fp : stderr);
  - const git_signature * S = _S;
  + const git_signature * S = ___S;
   assert(S != NULL);
   if (msg) fprintf(fp, "%s:", msg);
   fprintf(fp, " %s <%s>", S->name, S->email);
  @@ -206,10 +206,10 @@
   fprintf(fp, "\n");
   }

  -void rpmgitPrintIndex(void * _I, void * _fp)
  +void rpmgitPrintIndex(void * ___I, void * _fp)
   {
       FILE * fp = (FILE *) _fp;
  - git_index * I = (git_index *) _I;
  + git_index * I = (git_index *) ___I;
       unsigned Icnt;
       unsigned i;

  @@ -266,9 +266,9 @@
   }
   #endif

  -void rpmgitPrintTree(void * _T, void * _fp)
  +void rpmgitPrintTree(void * ___T, void * _fp)
   {
  - git_tree * T = (git_tree *) _T;
  + git_tree * T = (git_tree *) ___T;
       FILE * fp = (FILE *) _fp;
       unsigned Tcnt;
       unsigned i;
  @@ -305,10 +305,10 @@
       }
   }

  -void rpmgitPrintCommit(rpmgit git, void * Commit, void * _fp)
  +void rpmgitPrintCommit(rpmgit git, void * ___C, void * _fp)
   {
       FILE * fp = (FILE *) _fp;
  - git_commit * C = Commit;
  + git_commit * C = ___C;
       unsigned Pcnt;
       unsigned i;

  @@ -378,10 +378,10 @@

   }

  -void rpmgitPrintHead(rpmgit git, void * Header, void * _fp)
  +void rpmgitPrintHead(rpmgit git, void * ___H, void * _fp)
   {
       FILE * fp = (_fp ? _fp : stderr);
  - git_reference * H = (Header ? Header : git->H);
  + git_reference * H = (___H ? ___H : git->H);
       git_reference * Hresolved = NULL;
       int xx;

  @@ -412,10 +412,10 @@

   }

  -void rpmgitPrintRepo(rpmgit git, void * Repo, void * _fp)
  +void rpmgitPrintRepo(rpmgit git, void * ___R, void * _fp)
   {
       FILE * fp = (_fp ? _fp : stderr);
  - git_repository * R = Repo;
  ...