Hi! Now I uploaded new version to mentors. On Friday 23 December 2016 10:18:52 Andreas Henriksson wrote: > Hello, > > Started looking at this bug report yesterday but got discracted... > > I spotted much the same issues as Chrisian so I'll instead just > echo what he's saying and add a few comments. > (I'll be able to sponsor you once the package is ready.) > > On Fri, Dec 23, 2016 at 12:12:17AM +0100, Christian Seiler wrote: > > Hi, > > > > as announced on IRC, I'm just doing a review, since I'm not a DD > > > > and can't sponsor: > > - packaging in a VCS would be nice to have (plus the appropriate > > > > Vcs-Browser / Vcs-... headers in d/control) > > > > - debian/copyright: > > * Tobias Klauser wasn't just active in 2016, the earliest > > > > copyright notice of his I could find in the package is > > from 2014; so s/2016/2014-2016/ there > > > > * missing mention of Copyright (C) 2012 Christoph Jaeger > > > > for pkt.h > > > > * missing mention of Copyright (C) 2009-2012 Daniel > > > > Borkmann for util.[ch] > > The debian/copyright issue is the only major reason I seen not to > upload right now, because this issue will possibly mean rejection > from NEW queue. Please carefully look at all source files > and document copyright holders (autogenerated build files can > be excluded). Should be fixed now. > > - debian/compat: why only 9? compat 10 is considered stable now > > > > and unless you have a good reason I would recommend that any new > > package should use compat 10. (please read the debhelper manual > > though for information on what changed between 9 and 10) > > (compat 10 also gives you a nice automatic dh-autoreconf and > dh-systemd. Don't forget both to bump debian/compat and the > debhelper build-dependency.) dh_make just generate template with debhelper 9. dh-autoreconf is useless here as this package does not use any autoconf or automake. There is just plain Makefile. Anyway, changed to debhelper 10. > > - init.d: this file name works with dh_installinit, but is not > > > > documented, so I'd recommend using llmnrd.init as the file name > > I see you're already credited by upstream so I assume you have > already established a good relationship with your upstream. > That's very good and very useful. Keep your upstream happy. > Upstreams like contributions. You have a golden opportunity > on upstream issue #2. Renamed to llmnrd.init. > > - init.d: any particular reason you don't use init-d-script? (See > > > > current /etc/init.d/skeleton for how this works; it will > > automatically source /etc/default/$scriptname and interpret the > > DAEMON_ARGS variable, so your init script could probably be just > > a couple of lines that set the name of the executable) > > I'd recommend *against* "init-d-script". It has several outstanding > issues, is unmaintained/orphaned/unproven and AIUI that also means > the init script becomes debian-only. Init script I used from old template provided by Debian and slightly modified it. As llmnrd does not generate pidfile, I used start-stop- daemon with -b and -m args. So I cannot use init-d-script easily. Maybe I can report feature request to upstream to include support for creating pid file. And together with support for forking, start-stop- daemon can be used without -b -m. > > - any reason you don't install the systemd service provided by > > > > upstream in addition to the init script? > > Please do. Please also consider improving the systemd service > shipped by upstream. (Another golden opportunity for upstream > contributions.) > Most importantly have a look at the User= directive as it seems > like running unprivilegied is preferred (see upstream issue #4). > See also the Restrict*= directives provided by systemd which > would also be nice to limit the potential attack surface. Reasons why I did not install systemd service file: 1) it is not installed by make install, so I forgot about it 2) it is not fully compatible with provided init.d script 3) I'm not familiar with systemd (not even big fan) Init.d script loads additional args from /etc/default/llmnrd DAEMON_OPTS and there is specified default argument -6 which enabled IPv6 support. I hope Debian is already IPv6 ready and enabling IPv6 support is useful in Debian. 1) is not a problem for me, but 3) discredit me to properly handle DAEMON_OPTS from /etc/default/llmnrd properly. Anyway, current init script is working fine with systemd, so missing systemd service file is not a problem for using llmnrd. I would rather have IPv6 support as systemd service file. If somebody want to improve systemd service file I will let this part... > > - debian/rules: nice and clean, I like it > > > > - upstream's build system does git id to get the git revision of > > > > the current source - but that will clash if you have the > > packaging in git (which can happen implicitly when someone > > checks out the package source via e.g. dgit) > > > > Minor cosmetic thing, but makes the package non-reproducible > > depending on whether you build from unpacked .dsc or from a git > > environment Makefile append -DGIT_VERSION flag from `git id` output. And it is used just only in help message to show not only version, but also git id. I added "export GIT_VERSION =" in debian/rules which should overwrite possible git id and so built should be reproducible also when building from git. > > - lintian warnings: > > W: llmnrd: binary-without-manpage usr/bin/llmnr-query > > W: llmnrd: binary-without-manpage usr/sbin/llmnrd Sorry, but with missing manpages I cannot do nothing. Just report bug to upstream. I'm not native English speaker so writing text for end users is not for me. > > - you should probably add a line "export Q =" to debian/rules to > > > > disable silent builds. While these look nicer, automated build > > log scanners such as blhc aren't able to catch problems. > > debhelper today automatically disables silent rules when building > on buildds. Using Q environment variables isn't the normal thing > though. Even better than to explicitly disable silent build would be > to hook up Q to the automatic debhelper version (V=1?). I added export Q =. Looks like it is working with current Makefile. > > - Building in sbuild appears to work fine. > > > > - Package appears to work fine (though I don't have any llmnr > > > > device running at the moment, so I could only test name > > resolution of my own system) > > > > Regards, > > Christian > > Regards, > Andreas Henriksson -- Pali Rohár pali.rohar@gmail.com
Attachment:
signature.asc
Description: This is a digitally signed message part.