Bug#848993: RFS: llmnrd/0.2-1 [ITP]
On Saturday 24 December 2016 11:38:01 Andreas Henriksson wrote: > On Fri, Dec 23, 2016 at 09:41:29PM +0100, Pali Rohár wrote: > > On Friday 23 December 2016 14:46:53 Andreas Henriksson wrote: > > > You use DAEMON_OPTS in the default file, while sysvinit seems to > > > be standardizing on DAEMON_ARGS to avoid the messy work of > > > later migration of conffile settings you might want to consider > > > switching to DAEMON_ARGS now before the first version has been > > > uploaded. You decide. > > > > Hm... Another option would be to use LLMNRD_OPTS. Looks like other > > daemons in Debian (e.g. ntpd or rsync) use env variable _OPTS > > in /etc/default/. > > > > What do you think about it? > > The reason to use DAEMON_ARGS is that if you ever where to switch > to init-d-script in the future it would "just work" without > needing messy conversion handling. Your current init scripts seems > to already support DAEMON_ARGS already so it would just be a > case of changing the default file to use DAEMON_ARGS. Hm... Ok make sense. I removed DAEMON_OPTS from init.d script and added DAEMON_ARGS into default file. > Giving it an even more specialized name lite LLMNRD_OPTS seems > like you're expecting environment variable leakage between > different services and that should in my opinion be handled > by fixing the leakage instead. > > In the end this is all your choice. I've reviewed your existing > package and is ready to upload as soon as you've decided on > this matter. Poke me asap and there might still be a theoretical > chance of making into stretch (although I basically assume it's > already missed since we need to clear both the NEW queue + > a 10 day migration time before 5 jan - and I don't expect to see > 0-day NEW queue handling on christmas). Half hour ago I uploaded new package to mentors. But it is still not there... https://mentors.debian.net/package/llmnrd -- Pali Rohár pali.ro...@gmail.com signature.asc Description: This is a digitally signed message part.
Bug#848993: RFS: llmnrd/0.2-1 [ITP]
On Fri, Dec 23, 2016 at 09:41:29PM +0100, Pali Rohár wrote: > On Friday 23 December 2016 14:46:53 Andreas Henriksson wrote: > > You use DAEMON_OPTS in the default file, while sysvinit seems to be > > standardizing on DAEMON_ARGS to avoid the messy work of later > > migration of conffile settings you might want to consider switching > > to DAEMON_ARGS now before the first version has been uploaded. You > > decide. > > Hm... Another option would be to use LLMNRD_OPTS. Looks like other > daemons in Debian (e.g. ntpd or rsync) use env variable _OPTS in > /etc/default/. > > What do you think about it? The reason to use DAEMON_ARGS is that if you ever where to switch to init-d-script in the future it would "just work" without needing messy conversion handling. Your current init scripts seems to already support DAEMON_ARGS already so it would just be a case of changing the default file to use DAEMON_ARGS. Giving it an even more specialized name lite LLMNRD_OPTS seems like you're expecting environment variable leakage between different services and that should in my opinion be handled by fixing the leakage instead. In the end this is all your choice. I've reviewed your existing package and is ready to upload as soon as you've decided on this matter. Poke me asap and there might still be a theoretical chance of making into stretch (although I basically assume it's already missed since we need to clear both the NEW queue + a 10 day migration time before 5 jan - and I don't expect to see 0-day NEW queue handling on christmas). Regards, Andreas Henriksson
Bug#848993: RFS: llmnrd/0.2-1 [ITP]
On Friday 23 December 2016 14:46:53 Andreas Henriksson wrote: > Hi again Pali Rohár, > > On Fri, Dec 23, 2016 at 11:29:59AM +0100, Pali Rohár wrote: > > Now lintian on mentors shows warning: > > > > package-uses-experimental-debhelper-compat-version > > 10 > > Yes, lintian is simply wrong/outdated here. It's just a tool to help > you find issues, don't blindly follow lintian like if it was > religion or policy. Normally you'd consider a lintian override in > cases where you have confirmed lintian is wrong, but in this case > the warning will likely go away by itself if you just give it some > time for new lintian releases to appear. > > More importantly I wanted to mention a detail which might be useful > to consider before uploading your package: > > You use DAEMON_OPTS in the default file, while sysvinit seems to be > standardizing on DAEMON_ARGS to avoid the messy work of later > migration of conffile settings you might want to consider switching > to DAEMON_ARGS now before the first version has been uploaded. You > decide. Hm... Another option would be to use LLMNRD_OPTS. Looks like other daemons in Debian (e.g. ntpd or rsync) use env variable _OPTS in /etc/default/. What do you think about it? > FYI, I also filed issues upstream for potential systemd service > improvements. #15, #16. > Shipping the file is as simple as running > "echo etc/llmnrd.service >> debian/llmnrd.install" Yes, I know. But first we need fully working service file. > as dh compat 10 takes care of the rest for you. > You might want to consider dropping the attached patch in > debian/patches/ and adding debian/patches/series containing it's > name to preserve default file configuration under both init systems. > > Regards, > Andreas Henriksson -- Pali Rohár pali.ro...@gmail.com signature.asc Description: This is a digitally signed message part.
Bug#848993: RFS: llmnrd/0.2-1 [ITP]
Hi again Pali Rohár, On Fri, Dec 23, 2016 at 11:29:59AM +0100, Pali Rohár wrote: > Now lintian on mentors shows warning: > > package-uses-experimental-debhelper-compat-version > 10 Yes, lintian is simply wrong/outdated here. It's just a tool to help you find issues, don't blindly follow lintian like if it was religion or policy. Normally you'd consider a lintian override in cases where you have confirmed lintian is wrong, but in this case the warning will likely go away by itself if you just give it some time for new lintian releases to appear. More importantly I wanted to mention a detail which might be useful to consider before uploading your package: You use DAEMON_OPTS in the default file, while sysvinit seems to be standardizing on DAEMON_ARGS to avoid the messy work of later migration of conffile settings you might want to consider switching to DAEMON_ARGS now before the first version has been uploaded. You decide. FYI, I also filed issues upstream for potential systemd service improvements. #15, #16. Shipping the file is as simple as running "echo etc/llmnrd.service >> debian/llmnrd.install" as dh compat 10 takes care of the rest for you. You might want to consider dropping the attached patch in debian/patches/ and adding debian/patches/series containing it's name to preserve default file configuration under both init systems. Regards, Andreas Henriksson --- a/etc/llmnrd.service +++ b/etc/llmnrd.service @@ -4,7 +4,8 @@ [Service] Type=simple -ExecStart=/usr/sbin/llmnrd +EnvironmentFile=-/etc/default/llmnrd +ExecStart=/usr/sbin/llmnrd $DAEMON_OPTS $DAEMON_ARGS Restart=on-failure User=nobody Group=nogroup
Bug#848993: RFS: llmnrd/0.2-1 [ITP]
On Fri, Dec 23, 2016 at 11:28:39AM +0100, Pali Rohár wrote: > Hi! Now I uploaded new version to mentors. > [...] > So I cannot use init-d-script easily. [...] Thanks for following up with a good explanation for the choices you make. [...] > Reasons why I did not install systemd service file: [...] You're ofcourse free to decide not to spend time on specific things, but at the same time by integrating your package in a less than optimal way with Debian you'll likely not attract as much interest from potential sponsors. Also, any network facing daemon really need to think about security implications. Just running as root and not caring is in my opinion not a good option. Other options than using the systemd feature would be: * help upstream implement the privilegies dropping feature. * use start-stop-daemon to start under a less privilegied user. Either way you'll need to implement the integration in your package to create the less privilegied user. See adduser. Not sure I'll consider this a blocker, but it's borderline for me. Will review your new revision when I find time. Regards, Andreas Henriksson
Bug#848993: RFS: llmnrd/0.2-1 [ITP]
Hello, On Fri, Dec 23, 2016 at 11:18:03AM +0100, Christian Seiler wrote: > Hi there, > > sorry for the formatting, writing this on my phone. > > > Am 23. Dezember 2016 10:18:52 MEZ, schrieb Andreas Henriksson > : [...] > >on upstream issue #2. > > I'm not sure that'll work. In contrast to systemd services init scripts are > necessarily very distro-dependent. You can hack together something that's > cross-distro, but that's really ugly. [...] If only looking at major distributions Debian is likely the only one still using init scripts. OTOH apparently upstream thinks catering for the niche distros is important enough to file a bug report about it against his own software. Making the debian init script more portable could just be seen as a future improvement IMO. :P [...] > IMHO init scripts are distro-dependent anyway (see above). I didn't know > about the issues in init-d-script and since I use that in my own packages, > I'll look into that. Any pointers? [...] See existing bug reports, many contain init-d-script in title at: https://bugs.debian.org/cgi-bin/pkgreport.cgi?src=sysvinit My personal opinion is that for example breaking the LSB hooks that redirects direct /etc/init.d/foo invokations to using systemctl (which you really, really want to do when) under running systemd is quite unfortunate (#826214, #826215). I also find it very unfortunate that minor bugs that goes unfixed gets worked around depending on implementation-specific ways which means that the more people who use init-d-script the hard it will become to ever fix it up without breaking all users which then relies on the exact current/previous implementation. I've already asked about rolling back to the old skeleton but since noone is caring for sysvinit that has just ended up in void. When this issue was brought up with dh-make maintainers they instead decided to just completely drop sysvinit example. :/ If needed, we should probably discuss this further elsewhere as this is getting off-topic for the llmnrd sponsorship bug report. Regards, Andreas Henriksson
Bug#848993: RFS: llmnrd/0.2-1 [ITP]
On Friday 23 December 2016 11:28:39 Pali Rohár wrote: > > > - 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. Now lintian on mentors shows warning: package-uses-experimental-debhelper-compat-version 10 -- Pali Rohár pali.ro...@gmail.com signature.asc Description: This is a digitally signed message part.
Bug#848993: RFS: llmnrd/0.2-1 [ITP]
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
Bug#848993: RFS: llmnrd/0.2-1 [ITP]
Hi there, sorry for the formatting, writing this on my phone. Am 23. Dezember 2016 10:18:52 MEZ, schrieb Andreas Henriksson : >On Fri, Dec 23, 2016 at 12:12:17AM +0100, Christian Seiler wrote: >> - 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. I'm not sure that'll work. In contrast to systemd services init scripts are necessarily very distro-dependent. You can hack together something that's cross-distro, but that's really ugly. Also, Debian (+ derivatives) is just about the only major distro that still supports traditional init scripts, except for maybe Slackware: Gentoo always had their own thing that wasn't compatible. RH had /etc/sysconfig instead of /etc/default and had different includes for helper functions, just to give an idea what differences there are. SuSE hat yet another include library. RH didn't support LSB headers but had similar headers based on chkconfig to express dependencies. >> - 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. IMHO init scripts are distro-dependent anyway (see above). I didn't know about the issues in init-d-script and since I use that in my own packages, I'll look into that. Any pointers? >> - 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. Ack. >> - 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?). Yeah, probably do something like ifneq($(V),1) Q?=@ endif instead of just Q?=@ in upstream's Makefile. That said: I concur that these are all minor issues that can be fixed later and that d/copyright is the only blocker for an upload. And if this is to go into Stretch, the upload needs to happen today. Since Andreas is willing to sponsor I'd recommend fixing that issue immediately and after Jan. 5th when it is in Stretch to fix the rest. Regards, Christian
Bug#848993: RFS: llmnrd/0.2-1 [ITP]
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). > > - 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.) > > - 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. > > - 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. > > - 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. > > - 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 > > - lintian warnings: >W: llmnrd: binary-without-manpage usr/bin/llmnr-query >W: llmnrd: binary-without-manpage usr/sbin/llmnrd > > > - 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?). > > - 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
Bug#848993: RFS: llmnrd/0.2-1 [ITP]
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] - 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) - init.d: this file name works with dh_installinit, but is not documented, so I'd recommend using llmnrd.init as the file name - 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) - any reason you don't install the systemd service provided by upstream in addition to the init script? - 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 - lintian warnings: W: llmnrd: binary-without-manpage usr/bin/llmnr-query W: llmnrd: binary-without-manpage usr/sbin/llmnrd - 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. - 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
Bug#848993: RFS: llmnrd/0.2-1 [ITP]
Updated to version 0.2.1-1 https://mentors.debian.net/debian/pool/main/l/llmnrd/llmnrd_0.2.1-1.dsc -- Pali Rohár pali.ro...@gmail.com signature.asc Description: This is a digitally signed message part.
Bug#848993: RFS: llmnrd/0.2-1 [ITP]
Package: sponsorship-requests Severity: wishlist Dear mentors, I am looking for a sponsor for my package "llmnrd" * Package name: llmnrd Version : 0.2-1 Upstream Author : Tobias Klauser * URL : https://github.com/tklauser/llmnrd * License : GPL-2 Section : net It builds those binary packages: llmnrd - Link-Local Multicast Resolution (LLMNR) Daemon for Linux To access further information about this package, please visit the following URL: https://mentors.debian.net/package/llmnrd Alternatively, one can download the package with dget using this command: dget -x https://mentors.debian.net/debian/pool/main/l/llmnrd/llmnrd_0.2-1.dsc More information about hello can be llmnrd from https://github.com/tklauser/llmnrd. Changes since the last upload: * Initial release (Closes: #848880) Regards, Pali Rohár signature.asc Description: This is a digitally signed message part.
