Hi Lukas,
2017-06-28 20:41 GMT+02:00 Lukas Schwaighofer <lukas at schwaighofer.name>:
> Hi Stéphane,
>
> On Wed, 28 Jun 2017 14:05:57 +0200
> Stéphane Neveu <stefneveu at gmail.com> wrote:
>
>> 2017-06-27 23:00 GMT+02:00 Lukas Schwaighofer
>> <lukas at schwaighofer.name>:
>> > In the postinst script you tried the following:
>> >
>> > DNSPUBKEY=`echo $OUTPUT | awk '/DNS public key:/ {print $4}'`
>> > HEXPUBKEY=`echo $OUTPUT | awk '/Hex public key:/ {print $4}'`
>> > HEXSECRETKEY=`echo $OUTPUT | awk '/Hex secret key:/ {print $4}'`
>> >
>> > This is a good way of extracting the information (much better than
>> > what you did afterwards). The reason why it didn't work is because
>> > you need to quote "$OUTPUT", otherwise the shell performs what is
>> > called word splitting.
>> >
>> > Please change the lines again to be similar to:
>> > DNSPUBKEY=`echo "$OUTPUT" | awk '/DNS public key:/ {print $4}'`
>> >
>>
>> Corrected, may I ask you to explain me why it is a better solution ?
>
> It's better because it's more explicit (much easier for someone reading
> the script to see you are indeed extracting the correct information).
> Also it's less likely to break if the output of curvedns-keygen changes
> slightly (e.g. keys returned in a different order).
>
OK thank you
>
>> > [regarding the init script]
>> > The environment should work fine. Have a look at the DAEMON_ARGS…
>> >
>>
>> Yes the environment is well sourced but actually "$@" is empty ($*
>> also empty) in the wrapper-script, whereas echoing every variables
>> works well...
>
> The init script is, in this case, quite difficult to get right. To
> save some time I've made the required adjustments in git, I'll explain
> each of my changes here:
>
> * I've added the path to the wrapper script into a separate variable
> including a comment why we've made such a wrapper script
> * lsb-base >= 3.2-14 is even part of oldoldstable, so I've dropped the
> version comment.
> * We need to source the defaults file (`. /etc/default/$NAME`) so we can
> pass the command line arguments to the daemon later. (Not needed
> when you do that in the wrapper, see below.)
> * do_start: I've adjusted the command to start the daemon:
> - `--exec` needs to be passed the path to the actual daemon
> executable (the executable of the long-running process); that's
> because --exec is part of the "matching options" to look if the
> process already exists (and the wrapper will have long died)
> - added `--startas $DAEMON_WRAPPER`, which is what start-stop-daemon
> will actually execute
> - curvedns doesn't fork and doesn't create a pidfile, so we let
> start-stop-daemon do that for us (by adding `--make-pidfile` and
> `--background`); this will also take care of the Ctrl+C problem you
> had
> - pass the correct arguments (ip port remoteip remoteport) to the
> wrapper script (this is what "$@" in the wrapper will expand to,
> so they are then passed to the real daemon in the same way)
> * do_stop cleanup: curvedns does not fork or create child processes,
> removing unnecessary comments and the second call to start-stop-daemon
> * curvedns has no reload functionality, thus we completely remove the
> do_reload shell function
> * pass the pidfile to status_of_proc now that we have it :)
> * some more cleanup towards the end of the script
>
> I have also reverted your change to the wrapper script and added "$@"
> again for the commandline arguments. I think it's better to do as much
> as possible in the init script itself (like passing the correct
> arguments) and only do what we cannot do (setting up the environment)
> in the wrapper script.
>
>
> Please check each of the changes and get back to me if there's
> something you do not understand.
>
Great ! thank you. This is clear enough for me to understand.
>
> I have a few more TODOs for you:
> * debian/curvedns.postinst cleanup:
> - remove the `chmod` of the wrapper script
> - drop `set +x` once you are done debugging
> * debian/curvedns.dirs: drop `/usr/lib/curvedns` (created by dh_install
> when it installs the wrapper script)
> * debian/watch: "Debian" needs to be lower case (otherwise this is
> interpreted as a version and `uscan -v` reports a warning)
> * enable pristine-tar in your debian/gbp.conf, download the original
> upstream tarball and commit (+push) the delta files into the
> pristine-tar branch
> * debian/control: drop the version in the lsb-base dependency (even
> oldoldstable is recent enough as noted above)
> * debian/copyright:
> - The curvedns software is licensed as BSD-2-clause (which you also
> specified). However, as license text for the BSD-2-clause license,
> you have given the text of the MIT license (!).
> - I would make the Copyright line as follows:
> Copyright: 2010, CurveDNS Project.
> There are no other mentions of names/entities directly listed as
> copyright holders in any of the files…
> - The nacl software is public domain, you should add a separate
> section for nacl/* in the debian/copyright file (you can probably
> copy the license text for that from the nacl source package)
> * consistent order of sourcing environment files: both curvedns.service
> and the wrapper script should take the environment files in the same
> order so we have consistent behavior between the different service
> managers. I would suggest the following order:
> 1. /var/lib/curvedns/numeric_uid_gid
> 2. /etc/default/curvedns
> 3. /etc/curvedns/curvedns_private_key.hex
> That allows overriding the UID/GID in /etc/default/curvedns if
> someone desires (but do.es not encourage them to write the secret key
> into this world readable file).
>
All done I hope but I did not found any license text for public
domain. This is almost like having no copyright to me
>
>
> General feedback: Reviewing your work gets easier if you do not push
> your trial-and-error steps as commits. You can do that by either
> always amending the topmost commit until you are satisfied (use
> `git commit --amend`) or to squash the try-and-error commits together
> just before pushing.
>
> However, be aware that you should never amend commits or squash
> something that was already pushed to alioth.
>
Sorry about that.
> Regards
> Lukas
Good evening
Stephane