Post by Patrik LundinThanks for the info. At this point I have a bunch of diffs against
2.0.1 (as well as some runtime observations). I feel it would be
good to discuss them prior to creating PRs since some may be
interesting for merging upstream while others wont. Is there a
suitable way for doing this? I could post on this this list but I am
afraid it would cause a bunch of uninteresting noice for other members.
Given the lack of response I'll just use this list for the followup.
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:
./scheduler/schedule.h:50: error: expected specifier-qualifier-list before 'pthread_cond_t'
In file included from daemon/engine.h:37,
from ods-enforcerd.c:37:
./daemon/cmdhandler.h:49: error: expected specifier-qualifier-list before 'pthread_t'
In file included from daemon/engine.h:38,
from ods-enforcerd.c:37:
./daemon/worker.h:45: error: expected specifier-qualifier-list before 'pthread_t'
In file included from ods-enforcerd.c:37:
daemon/engine.h:69: error: expected specifier-qualifier-list before 'pthread_cond_t'
*** Error 1 in enforcer/src (Makefile:1368 'ods-enforcerd.o')
*** Error 1 in enforcer/src (Makefile:1468 'all-recursive')
*** Error 1 in enforcer (Makefile:491 'all-recursive')
*** Error 1 in /home/pobj/opendnssec-2.0.1-sqlite3/opendnssec-2.0.1 (Makefile:539 'all-recursive')
*** Error 1 in . (/usr/ports/infrastructure/mk/bsd.port.mk:2679 '/usr/ports/pobj/opendnssec-2.0.1-sqlite3/.build_done')
*** Error 1 in /usr/ports/security/opendnssec (/usr/ports/infrastructure/mk/bsd.port.mk:2397 'build')
===
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"
===
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:
===
--- libhsm/src/bin/hsmutil.c.orig Sun Sep 4 16:30:20 2016
+++ libhsm/src/bin/hsmutil.c Sun Sep 4 16:31:34 2016
@@ -104,8 +104,6 @@ cmd_logout ()
return 0;
}
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wformat-nonliteral"
static int
cmd_list (int argc, char *argv[])
{
@@ -190,7 +188,6 @@ cmd_list (int argc, char *argv[])
return 0;
}
-#pragma GCC diagnostic pop
static int
cmd_generate (int argc, char *argv[])
===
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.
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.
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);
===
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'
===
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:
===
--- 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;
===
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
libhsm.c:289: warning: ISO C forbids conversion of object pointer to function pointer type
===
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')
===
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?
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.
--
Patrik Lundin