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 _______________________________________________ Opendnssec-user mailing list [email protected] https://lists.opendnssec.org/mailman/listinfo/opendnssec-user
