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

[Patrik Lundin]

On Thu, Sep 15, 2016 at 09:55:16AM +0200, Berry A.W. van Halderen wrote:
> > --- 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).
> 

https://github.com/opendnssec/opendnssec/pull/526

> > 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'
> > ===
> >
[...]
> 
> 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...
> 

Guess I will just handle the pragma removal locally for now. If the
push/pop pragma was silently ignored the side effects would be wider
applications of the actually supported pragmas which the push/pop is
supposed to contain. This would probably be even less expected.

> > Next up:
> > ===
> > parser/addnsparser.c:381: warning: passing argument 2 of
> 'parse_addns_tsig_static' discards qualifiers from pointer target type
> > ===
> >
[...]
> 
> I think this can indeed be handled better by properly using const
> versus non-const in the right places.
> 

Have any input on where the "right places" are? If this is updated
upstream I could mirror those changes in the port as well. This is true
for all the other "const" things I have reported. It is hard to know
what directions are correct in the several places I have seen this far.

> > Next up, some time_t inconsistencies:
> > ===
[...]
> 
> Then again there might be platforms that no not have long-longs. This
> needs some reviewing.
> 

https://github.com/opendnssec/opendnssec/pull/527

> >
> > 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.
> 

https://github.com/opendnssec/opendnssec/pull/528

> > 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.
> 

This is the same as the previous "const" question. I am not sure I am
placing "const" in the right places currently, given I do not know the
grand design goals of these interfaces.

> > An initialization warning:
> > ===
> > keystate/key_purge.c:48: warning: 'state' may be used uninitialized in
> this function
> > ===
> >
[...]
> 
> 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.
> 

https://github.com/opendnssec/opendnssec/pull/529

> > 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;
> >
> > ===
> >

You could say these 'const' questions are a constant hassle ;).

> > Then there is another uninitialized variable:
> > ===
> > enforcer/enforcer.c:2637: warning: 'state' may be used uninitialized
> in this function
> > ===
> >

https://github.com/opendnssec/opendnssec/pull/530

> >
> > 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.
> 

This sounds reasonable, do you have a specific diff in mind?

> > 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.
> 

This is fine with me. As long as the warning is known and acknowledged I
am happy.

> > ===
> >
> > 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.
> > ===
> 
> 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.
> 

I'll skip the tests.

> >
> > 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.
> 

I take it the "Yes" refers to all the missing directories I mentioned.
Thanks for the info :).

> >
> > 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?

Never got a response regarding this, but maby it was a silent
acknowledgement to just let it be missing :).

-- 
Patrik Lundin