[Opendnssec-user] OpenBSD porting questions regarding 2.x

[Berry A.W. van Halderen]

On 09/14/2016 07:33 PM, Patrik Lundin wrote:
> On Tue, Sep 06, 2016 at 06:16:06PM +0200, Patrik Lundin wrote:
> Here is the error and warnings produced by the build and my attempts at
> fixing them.
>
> First the error and warnings from building:
> ===
> depbase=`echo ods-enforcerd.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`; cc
-std=gnu99 -DHAVE_CONFIG_H -I. -I../../common     -I../../common
-I../../common  -I./../../libhsm/src/lib  -I./../../libhsm/src/lib
-I/usr/local/include/libxml2 -I/usr/local/include  -I/usr/local/include
 -I/usr/include -O2 -pipe -pedantic -Wall -pthread -fno-strict-aliasing
-Wall -Wextra -Wwrite-strings -Wpointer-arith -Wno-unused-parameter
-Wno-missing-field-initializers -Wformat=2 -Wcast-align
-Wformat-security -Wstrict-aliasing -Wpacked -Winit-self
-Wmissing-include-dirs -Wreturn-type -Wno-format-nonliteral
-Wno-format-y2k -Wno-unused-function -Wno-unused-variable
-Wno-sign-compare -Wno-error=unused-parameter
-Wno-error=missing-field-initializers -Wno-error=format-nonliteral
-Wno-error=format-y2k -Wno-error=unused-function
-Wno-error=unused-variable -Wno-error=sign-compare -MT ods-enforcerd.o
-MD -MP -MF $depbase.Tpo -c -o ods-enforcerd.o ods-enforcerd.c && mv -f
$depbase.Tpo $depbase.Po
> In file included from ./daemon/cmdhandler.h:36,
>                  from daemon/engine.h:37,
>                  from ods-enforcerd.c:37:
>
> Fixed with the following include:
> ===
> --- enforcer/src/scheduler/schedule.h.orig      Sun Sep  4 16:11:36 2016
> +++ enforcer/src/scheduler/schedule.h   Sun Sep  4 16:11:55 2016
> @@ -36,6 +36,7 @@
>
>  #include <time.h>
>  #include <ldns/ldns.h>
> +#include <pthread.h>
>
>  #include "scheduler/task.h"
>  #include "status.h"
> ===

That's on the spot (ie. quite right change).

> Secondly some warnings thrown regarding unsupported push/pop GCC magic:
> ===
> hsmutil.c:107: warning: expected [error|warning|ignored] after
'#pragma GCC diagnostic'
> hsmutil.c:193: warning: expected [error|warning|ignored] after
'#pragma GCC diagnostic'
> ===
>
> These I am not sure how to deal with in an upstream-compatible way.
This seemed
> to be caused by the use of "#pragma GCC diagnostic push/pop" calls
which from what I
> can tell are not supported by GCC 4.2.1.
>
> Locally I can just remove all the #pragma GCC diagnostic stuff:
>
> I notice the build is done using "-Wno-format-nonliteral" which i guess
> disables this kind of warnings anyway, and is the only option I can
find traces
> of in the tar.gz.

Yes, it disables the warning in GCC. Need to look into it how the
pragma is to be left out for older GCC versions. Normally pragmas
ought to be ignored if the compiler does not understand it. Love
the exceptions...

> Next up:
> ===
> parser/addnsparser.c:381: warning: passing argument 2 of
'parse_addns_tsig_static' discards qualifiers from pointer target type
> ===
>
> This was a bit tricky. Reading the code it seemed the second argument to
> parse_addns_tsig_static() is declared as a "char*" and the argument being
> passed was cast to that: (char *)"//Adapter/DNS/TSIG".
>
> After fiddling around with the build flags I could narrow the warning
down to
> the combinariton of the flags "-O2 -Wwrite-strings". The warning
disappears by
> defining the argument as a const:
> ===
> --- signer/src/parser/addnsparser.c.orig        Sun Sep  4 21:49:08 2016
> +++ signer/src/parser/addnsparser.c     Sun Sep  4 21:49:39 2016
> @@ -231,7 +231,7 @@ parse_addns_acl(const char* filename,
>   */
>  static tsig_type*
>  parse_addns_tsig_static(const char* filename,
> -    char* expr)
> +    const char* expr)
>  {
>      tsig_type* tsig = NULL;
>      tsig_type* new_tsig = NULL;
> ===
>
> This feels a bit off, since the argument and function declaration now
> mismatch code-wise.
> Leaving it out there for upstream feedback.

I think this can indeed be handled better by properly using const
versus non-const in the right places.

> Next up, some time_t inconsistencies:
> ===
> wire/axfr.c:112: warning: format '%ld' expects type 'long int', but
argument 4 has type 'time_t'
> wire/axfr.c:112: warning: format '%ld' expects type 'long int', but
argument 5 has type 'time_t'
> ===
>
> On modern OpenBSD time_t is a "long long". I am not sure all target
platforms
> of opendnssec supports "long long", but if it does I guess one
solution is to
> just cast the value to "long long" instead since it should fit
anything that
> fits in a "long":
> ===
> --- signer/src/wire/axfr.c.orig Sun Sep  4 23:18:13 2016
> +++ signer/src/wire/axfr.c      Sun Sep  4 23:18:44 2016
> @@ -108,8 +108,8 @@ soa_request(query_type* q, engine_type* engine)
>          expire = q->zone->xfrd->serial_xfr_acquired;
>          expire += ldns_rdf2native_int32(ldns_rr_rdf(rr,
SE_SOA_RDATA_EXPIRE));
>          if (expire < time_now()) {
> -            ods_log_warning("[%s] zone %s expired at %ld, and it is
now %ld: "
> -                "not serving soa", axfr_str, q->zone->name, expire,
time_now());
> +            ods_log_warning("[%s] zone %s expired at %lld, and it is
now %lld: "
> +                "not serving soa", axfr_str, q->zone->name, (long
long)expire, (long long)time_now());
>              ldns_rr_free(rr);
>              buffer_pkt_set_rcode(q->buffer, LDNS_RCODE_SERVFAIL);
>              ods_fclose(fd);
> ===

Then again there might be platforms that no not have long-longs. This
needs some reviewing.

> Next up: another set of the #pragma warnings. See above.
> ===
> utils/kc_helper.c:57: warning: expected [error|warning|ignored] after
'#pragma GCC diagnostic'
> utils/kc_helper.c:85: warning: expected [error|warning|ignored] after
'#pragma GCC diagnostic'
> ===
>
> Patched:
> ===
> --- enforcer/src/utils/kc_helper.c.orig Sun Sep  4 22:55:07 2016
> +++ enforcer/src/utils/kc_helper.c      Sun Sep  4 22:55:55 2016
> @@ -54,8 +54,6 @@ void log_init(int facility, const char *program_name)
>  }
>
>  /* As far as possible we send messages both to syslog and STDOUT */
> -#pragma GCC diagnostic push
> -#pragma GCC diagnostic ignored "-Wformat-nonliteral"
>  void dual_log(const char *format, ...) {
>
>         /* If the variable arg list is bad then random errors can
occur */
> @@ -82,7 +80,6 @@ void dual_log(const char *format, ...) {
>         va_end(args);
>         va_end(args2);
>  }
> -#pragma GCC diagnostic pop
>
>  /* Check an XML file against its rng */
>  int check_rng(const char *filename, const char *rngfilename, int verbose)
> ===
>
> Then there is another time_t related warning:
> ===
> hsmkey/hsm_key_factory.c:195: warning: format '%lu' expects type 'long
unsigned int', but argument 4 has type 'time_t'
> ===
>
> Fixed like so:
> ===
> --- enforcer/src/hsmkey/hsm_key_factory.c.orig  Sun Sep  4 22:58:21 2016
> +++ enforcer/src/hsmkey/hsm_key_factory.c       Sun Sep  4 23:15:11 2016
> @@ -189,8 +189,8 @@ void hsm_key_factory_generate(engine_type* engine, con
>          ods_log_info("%lu zone(s) found on policy <unknown>", num_zones);
>      }
>      ods_log_info("[hsm_key_factory_generate] %lu keys needed for %lu "
> -        "zones covering %lu seconds, generating %lu keys for policy %s",
> -        generate_keys, num_zones, duration,
> +        "zones covering %lld seconds, generating %lu keys for policy %s",
> +        generate_keys, num_zones, (long long)duration,
>          (unsigned long)(generate_keys-num_keys), /* This is safe
because we checked num_keys < generate_keys */
>          policy_name(policy));
>      generate_keys -= num_keys;
> ===
>
> A warning regarding implicit declaration:
> ===
> keystate/keystate_ds_submit_task.c:51: warning: implicit declaration
of function 'flush_enforce_task'
> ===

The change is fine, but I'm much surprised it was in. We regulary
check that there are NO warnings.

> Fixed by adding an include:
> ===
> --- enforcer/src/keystate/keystate_ds_submit_task.c.orig        Sun
Sep  4 23:20:37 2016
> +++ enforcer/src/keystate/keystate_ds_submit_task.c     Sun Sep  4
23:23:47 2016
> @@ -33,6 +33,7 @@
>  #include "daemon/engine.h"
>  #include "duration.h"
>  #include "keystate/keystate_ds.h"
> +#include "enforcer/enforce_task.h"
>
>  #include "keystate/keystate_ds_submit_task.h"
>
> ===
>
> Then there are a bunch of warnings regarding const variables losing
the const
> when passed to a function:
> ===
> keystate/key_purge.c:166: warning: passing argument 1 of
'key_state_delete' discards qualifiers from pointer target type
> keystate/key_purge.c:167: warning: passing argument 1 of
'key_state_delete' discards qualifiers from pointer target type
> keystate/key_purge.c:168: warning: passing argument 1 of
'key_state_delete' discards qualifiers from pointer target type
> keystate/key_purge.c:169: warning: passing argument 1 of
'key_state_delete' discards qualifiers from pointer target type
> ===
>
> The solution might be to make the function take a const instead, but
not sure about this, feedback needed:

Yes, const needed in most places. There are some (more) problematic
items where passing it as const will cause another warning as it
is passed as a key-value into an ldns rbtree. Those functions do
not use const, but the value can be treated as such, hence an
explicit cast is needed then.

> ===
> --- enforcer/src/db/key_state.c.orig    Sun Sep  4 23:45:35 2016
> +++ enforcer/src/db/key_state.c Sun Sep  4 23:46:05 2016
> @@ -828,7 +828,7 @@ int key_state_update(key_state_t* key_state) {
>      return ret;
>  }
>
> -int key_state_delete(key_state_t* key_state) {
> +int key_state_delete(const key_state_t* key_state) {
>      db_clause_list_t* clause_list;
>      db_clause_t* clause;
>      int ret;
>
> --- enforcer/src/db/key_state.h.orig    Sun Sep  4 23:39:23 2016
> +++ enforcer/src/db/key_state.h Sun Sep  4 23:39:47 2016
> @@ -254,7 +254,7 @@ int key_state_update(key_state_t* key_state);
>   * \param[in] key_state a key_state_t pointer.
>   * \return DB_ERROR_* on failure, otherwise DB_OK.
>   */
> -int key_state_delete(key_state_t* key_state);
> +int key_state_delete(const key_state_t* key_state);
>
>  /**
>   * A list of key state objects.
> ===
>
>
> An initialization warning:
> ===
> keystate/key_purge.c:48: warning: 'state' may be used uninitialized in
this function
> ===
>
> Fixed by:
> ===
> --- enforcer/src/keystate/key_purge.c.orig      Sun Sep  4 23:47:05 2016
> +++ enforcer/src/keystate/key_purge.c   Sun Sep  4 23:47:21 2016
> @@ -45,7 +45,7 @@ int removeDeadKeysNow(int sockfd, db_connection_t *dbc
>         int key_purgable, cmp;
>         int zone_key_purgable;
>         unsigned int j;
> -       const key_state_t* state;
> +       const key_state_t* state = NULL;
>         key_data_list_t *key_list = NULL;
>         key_data_t** keylist = NULL;
>         key_dependency_list_t *deplist = NULL;
> ===

It is okay, but it is a false positive by the compiler. The compiler
improperly does not see that if a for loop goes from 0 to 4 and an
if handles all the cases, it will always have a value. The initializer
will actually hide a warning when the if would not handle all cases.
So here the initializer is actually working against us, and shutting
up the compiler is unfortunate. Nevertheless, the code could use
some cleaning up, so we'll can just put it in.

> Then there is another case of const-juggling:
> ===
> signconf/signconf.c:88: warning: passing argument 1 of 'policy_free'
discards qualifiers from pointer target type
> signconf/signconf.c:111: warning: passing argument 1 of 'policy_free'
discards qualifiers from pointer target type
> ===
>
> I believe I first tried making the function take a const which then
caused a
> slew of new warnings, so maby the right solution is to not declare the
variable
> as const:
> ===
> --- enforcer/src/signconf/signconf.c.orig       Mon Sep  5 00:09:06 2016
> +++ enforcer/src/signconf/signconf.c    Mon Sep  5 00:09:23 2016
> @@ -57,7 +57,7 @@ int signconf_export_all(int sockfd, const db_connectio
>      zone_list_t* zone_list;
>      zone_t* zone;
>      int ret;
> -    const policy_t* policy = NULL;
> +    policy_t* policy = NULL;
>      int cmp;
>      int change = 0;
>
> ===
>
> Then there is another uninitialized variable:
> ===
> enforcer/enforcer.c:2637: warning: 'state' may be used uninitialized
in this function
> ===
>
> Patch:
> ===
> --- enforcer/src/enforcer/enforcer.c.orig       Mon Sep  5 00:11:29 2016
> +++ enforcer/src/enforcer/enforcer.c    Mon Sep  5 00:11:51 2016
> @@ -2634,7 +2634,7 @@ removeDeadKeys(db_connection_t *dbconn,
key_data_t** k
>         size_t i, deplist2_size = 0;
>         int key_purgable, cmp;
>         unsigned int j;
> -       const key_state_t* state;
> +       const key_state_t* state = NULL;
>         key_dependency_t **deplist2 = NULL;
>
>         assert(keylist);
> ===
>
> Finally there are some warnings thrown when building with -pedantic,
these I am
> not sure how to properly deal with (the first two being a result of native
> calls to strlcat/strlcpy and the last one I am not sure how to properly
> decipher:
> ===
> strlcat.c:22: warning: ISO C forbids an empty source file
> strlcpy.c:22: warning: ISO C forbids an empty source file

The C files should be optionally included by automake/autoconf
I think, rather then leave them empty.

> libhsm.c:289: warning: ISO C forbids conversion of object pointer to
function pointer type

This one is known, hopefully we can lift this in future, but it requires
some work in code we'd rather replease. It is part of
how the PKCS#11 interface works and requires some special C construct
to get rid off. I'd rather live with the warning for now than
to use the construct which I need to check on all systems first.

> ===
>
> Next up is the result of me trying to run tests. It appears the file
> test.sqlite is missing, is this by design? It is present in the git repo.
> ===
> rm -f test.db
> sqlite3 test.db < ./test.sqlite
> /bin/sh: cannot open ./test.sqlite: No such file or directory
> *** Error 1 in enforcer/src/db/test (Makefile:786 'regress-db')
> *** Error 1 in enforcer/src (Makefile:1468 'check-recursive')
> *** Error 1 in enforcer (Makefile:491 'check-recursive')
> *** Error 1 in /home/pobj/opendnssec-2.0.1-sqlite3/opendnssec-2.0.1
(Makefile:539 'check-recursive')
> *** Error 1 in . (/usr/ports/infrastructure/mk/bsd.port.mk:2713
'/usr/ports/pobj/opendnssec-2.0.1-sqlite3/.test_done')
> *** Error 1 in /usr/ports/security/opendnssec
(/usr/ports/infrastructure/mk/bsd.port.mk:2397 'test')
> ===

Not be design, but I'd advice not to run these tests. They give a false
sense of correctness, as OpenDNSSEC is tested in a different
manner. This is just one source module being tested, and only in
parts. Not worth the effort.

>
> Some questions raised during runtime:
> ===
> Sep  4 11:07:09 obsd-amd64-t01 ods-enforcerd: [cmdhandler] unable to
create, bind() failed: No such file or directory
> Sep  4 11:07:09 obsd-amd64-t01 ods-enforcerd: [engine] create command
handler to /var/run/opendnssec/enforcer.sock failed
> Sep  4 11:07:09 obsd-amd64-t01 ods-enforcerd: setup failed: Command
handler error
> Sep  4 11:07:09 obsd-amd64-t01 ods-enforcerd: [engine] enforcer shutdown
> Sep  4 11:07:09 obsd-amd64-t01 ods-enforcerd: [engine] enforcerd (pid:
56729) stopped with exitcode 3
> ===
>
> In OpenDNSSEC 1.4.10 the pid directory is created if it is missing (see
> createPidDir() in enforcer/common/daemon_util.c. Is this supposed to
be handled
> by rc/init scripts in 2.x?

Yes.

> 1.4.10: enforcer/common/daemon_util.c: createPidDir()
> 2.0.1: enforcer/src/daemon/cmdhandler.c: cmdhandler_create() does not
create pid directory.
>
> I also notice the directories /var/opendnssec/enforcer and
> /var/opendnssec/signer are not created manually. Are these supposed to be
> created by package maintaners:
> ===
> Sep  4 11:30:47 obsd-amd64-t01 ods-enforcerd: [file] chown()
/var/opendnssec/enforcer failed: No such file or directory
> Sep  4 11:30:47 obsd-amd64-t01 ods-enforcerd: [engine] chdir to
/var/opendnssec/enforcer failed: No such file or directory
> Sep  4 11:30:47 obsd-amd64-t01 ods-enforcerd: setup failed: Change
directory failed
> Sep  4 11:30:47 obsd-amd64-t01 ods-enforcerd: [engine] enforcer shutdown
> Sep  4 11:30:47 obsd-amd64-t01 ods-enforcerd: [engine] enforcerd (pid:
94894) stopped with exitcode 3
> ===
>
> ... and:
>
> ===
> Sep  4 12:30:14 obsd-amd64-t01 ods-signerd: [file] chown()
/var/opendnssec/signer failed: No such file or directory
> Sep  4 12:30:14 obsd-amd64-t01 ods-signerd: [engine] setup: unable to
chdir to /var/opendnssec/signer (No such file or directory)
> Sep  4 12:30:14 obsd-amd64-t01 ods-signerd: [engine] setup failed:
Change directory failed
> ===
>
> Finally it was the question of the missing zones.xml file, but this
has been
> discussed with Berry earlier, so for now the right thing is to have
this error
> be a part of an initial setup. I was thinking maby this file could be
created
> by the init script if missing, but I am worried this might have
implications
> when the goal is the do a migration from 1.4.10.
>
> The migration as described in
enforcer/utils/1.4-2.0_db_convert/README.md uses
> the missing file as a "lock" which is then resolved by copying in the
> zonelist.xml. In this case creating an empty file might break the
migration?
> ===
> Sep  4 12:31:22 obsd-amd64-t01 ods-signerd: [file] unable to stat file
/var/opendnssec/enforcer/zones.xml: ods_fopen() failed
> Sep  4 12:31:22 obsd-amd64-t01 ods-signerd: [engine] signer started
(version 2.0.1), pid 28483
> ===
>
> Hope anything here is interesting to upstream. I can create pull requests
> against 2.0/develop for anything deemed suitable and welcome feedback on
> anything even if not suitable for upstream merging.
>

Yes, we're interested, of course...

With kind regards,
Berry van Halderen