Bug#932438: RFS: irqbalance/1.6.0-1 [ITA] -- Daemon to balance interrupts for SMP systems
[2019-08-15 17:59] Paride Legovini > Dmitry Bogatov wrote on 06/08/2019: > > So, please: > > > > * drop --as-needed > > * fix spelling > > * finalize changelog (dch -r) > > Done. Uploaded. Tagged. Permissions granted. -- Note, that I send and fetch email in batch, once in a few days. Please, mention in body of your reply when you add or remove recepients.
Bug#932438: RFS: irqbalance/1.6.0-1 [ITA] -- Daemon to balance interrupts for SMP systems
Dmitry Bogatov wrote on 06/08/2019: > So, please: > > * drop --as-needed > * fix spelling > * finalize changelog (dch -r) Done. Paride
Bug#932438: RFS: irqbalance/1.6.0-1 [ITA] -- Daemon to balance interrupts for SMP systems
Dmitry Bogatov wrote on 06/08/2019:
>
> [2019-08-05 22:33] Paride Legovini
>> Dmitry Bogatov wrote on 23/07/2019:
>>> [2019-07-21 11:36] Paride Legovini
> 1. In [942ed5e] you added this line:
>
> export DEB_LDFLAGS_MAINT_APPEND = -Wl,--as-needed
>> [...]
>> That's my understanding too. In this case I'm pretty sure the binaries
>> are not getting linked against extra libraries. Maybe in larger project
>> it could be helpful to have a final prune of eventual extra libs.
>>
>> Do you suggest dropping those extra flags?
>
> Yes.
>
> 3. What is "irqbalance-ui"? It has no manpage, not referenced in
>irqbalance(1) and even after source diving I can't understand how to
>use it.
I'll probably add a stub manpage for it.
>>>
>>> Good.
>>
>> Done, and I upstreamed it:
>>
>> https://github.com/Irqbalance/irqbalance/commit/28575ddb46773a44
>
> I: irqbalance: spelling-error-in-manpage usr/share/man/man1/irqbalance.1.gz
> similiar similar
> I: irqbalance: spelling-error-in-manpage usr/share/man/man1/irqbalance.1.gz
> occured occurred
>
> 5. Do we really need debconf to configure oneshot feature? Debconf
> question block installation process, so they are not to be used
> lightly, imho. Even ssh server does not use debconf to make me
> review its config, which is of much more importance.
I am more than happy with dropping it entirely.
>>>
>>> Good.
>>
>> Done; I hope the note in d/NEWS is clear enough.
>
> Yes, it is fine.
>
> So, please:
>
> * drop --as-needed
> * fix spelling
> * finalize changelog (dch -r)
ACK, but I'll be back home in ~10 days.
> By the way, why
>
> [ -z "${IRQBALANCE_ONESHOT+x} ]
>
> instead of plain
>
> [ -z "${IRQBALANCE_ONESHOT}" ]
Because it would match even if the variable is set but empty, while we
want the condition to be met iif $IRQBALANCE_ONESHOT is unset.
With the "+x", if the variable is set (even to the empty string) the
expression is expanded to "x" and the condition is not met. Irqbalance
checks only if the variable is set.
This is my reference table for those parameter expansions is:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02
Paride
Bug#932438: RFS: irqbalance/1.6.0-1 [ITA] -- Daemon to balance interrupts for SMP systems
[2019-08-05 22:33] Paride Legovini
> Dmitry Bogatov wrote on 23/07/2019:
> > [2019-07-21 11:36] Paride Legovini
> >>> 1. In [942ed5e] you added this line:
> >>>
> >>> export DEB_LDFLAGS_MAINT_APPEND = -Wl,--as-needed
> [...]
> That's my understanding too. In this case I'm pretty sure the binaries
> are not getting linked against extra libraries. Maybe in larger project
> it could be helpful to have a final prune of eventual extra libs.
>
> Do you suggest dropping those extra flags?
Yes.
> >>> 3. What is "irqbalance-ui"? It has no manpage, not referenced in
> >>>irqbalance(1) and even after source diving I can't understand how to
> >>>use it.
> >>
> >> I'll probably add a stub manpage for it.
> >
> > Good.
>
> Done, and I upstreamed it:
>
> https://github.com/Irqbalance/irqbalance/commit/28575ddb46773a44
I: irqbalance: spelling-error-in-manpage usr/share/man/man1/irqbalance.1.gz
similiar similar
I: irqbalance: spelling-error-in-manpage usr/share/man/man1/irqbalance.1.gz
occured occurred
> >>> 5. Do we really need debconf to configure oneshot feature? Debconf
> >>> question block installation process, so they are not to be used
> >>> lightly, imho. Even ssh server does not use debconf to make me
> >>> review its config, which is of much more importance.
> >>
> >> I am more than happy with dropping it entirely.
> >
> > Good.
>
> Done; I hope the note in d/NEWS is clear enough.
Yes, it is fine.
So, please:
* drop --as-needed
* fix spelling
* finalize changelog (dch -r)
By the way, why
[ -z "${IRQBALANCE_ONESHOT+x} ]
instead of plain
[ -z "${IRQBALANCE_ONESHOT}" ]
?
--
Note, that I send and fetch email in batch, once in a few days.
Please, mention in body of your reply when you add or remove recepients.
Bug#932438: RFS: irqbalance/1.6.0-1 [ITA] -- Daemon to balance interrupts for SMP systems
Dmitry Bogatov wrote on 23/07/2019:
> [2019-07-21 11:36] Paride Legovini
>>> 1. In [942ed5e] you added this line:
>>>
>>> export DEB_LDFLAGS_MAINT_APPEND = -Wl,--as-needed
>>>
>>>Why is it needed? Maybe, these flags should be provided by
>>>dpkg-buildflags(1)?
>>
>> dpkg-buildflags does not currently provide the --as-needed flag to the
>> linker, see:
>>
>> https://wiki.debian.org/HardeningWalkthrough
>>
>> or just try. The flags are not needed but I think using them when
>> possible is good practice. For what --as-needed does see ld(1).
>
> As I understand, this with this flag linker does not make dynamically
> linked binary depend on library, that was passed on command line, but
> none of its symbols is used.
>
> If this is correct, question is why build system system links unused
> library in first place.
That's my understanding too. In this case I'm pretty sure the binaries
are not getting linked against extra libraries. Maybe in larger project
it could be helpful to have a final prune of eventual extra libs.
Do you suggest dropping those extra flags?
>>> 3. What is "irqbalance-ui"? It has no manpage, not referenced in
>>>irqbalance(1) and even after source diving I can't understand how to
>>>use it.
>>
>> I'll probably add a stub manpage for it.
>
> Good.
Done, and I upstreamed it:
https://github.com/Irqbalance/irqbalance/commit/28575ddb46773a44
>>> 4. Situation with "oneshot" option is quite... inconvenient.
>>>
>>>* If you have ONESHOT option set, `/etc/init.d/irqbalance status`
>>> will report failure. It can be fixed by checking for ONESHOT
>>> variable in status) clause;
This is fixed now.
>>>* current runscript does not respect ONESHOT option. It can be fixed
>>> with something like
>>>
>>>#!/bin/sh
>>>. /etc/default/irqbalance
>>>if [ -n "${ONESHOT:-}" ] ; then
>>>irqbalance --oneshot
>>>sv down irqbalance
>>>else
>>>exec irqbalance --foreground
>>>fi
>>>
>>> but it would be quite unnatural. I think proper solution would be
>>> separation of /etc/init.d/irqbalance and
>>> /etc/init.d/irqbalance-oneshot.
>>
>> I agree calling `sv down` from the runscript is ugly, on the other side
>> I don't really like the idea of having two sets of init scripts for
>> three init systems. Moreover while dh_installsystemd and dh_installinit
>> have a --no-enable option, dh_unit does not, requiring some manual
>> (hacky) handling.
>
> Runit does not have support for "one-shot" invocations by design: it is
> process supervisor, after all. But I agree, doubling number of initfiles
> for three init systems is considerable burden.
>
> So please check for $ONESHOT in init.d script, I will provide patch for
> runscript after.
OK, thanks!
>>> 5. Do we really need debconf to configure oneshot feature? Debconf
>>> question block installation process, so they are not to be used
>>> lightly, imho. Even ssh server does not use debconf to make me
>>> review its config, which is of much more importance.
>>
>> I am more than happy with dropping it entirely.
>
> Good.
Done; I hope the note in d/NEWS is clear enough.
The new commits are out for review:
https://salsa.debian.org/paride-guest/irqbalance
Thanks,
Paride
Bug#932438: RFS: irqbalance/1.6.0-1 [ITA] -- Daemon to balance interrupts for SMP systems
control: clone 932438 -2
control: reassign -2 irqbalance
control: retitle -2 add support for oneshot invocation with runit
control: owner -2 kact...@debian.org
[2019-07-21 11:36] Paride Legovini
> > 1. In [942ed5e] you added this line:
> >
> > export DEB_LDFLAGS_MAINT_APPEND = -Wl,--as-needed
> >
> >Why is it needed? Maybe, these flags should be provided by
> >dpkg-buildflags(1)?
>
> dpkg-buildflags does not currently provide the --as-needed flag to the
> linker, see:
>
> https://wiki.debian.org/HardeningWalkthrough
>
> or just try. The flags are not needed but I think using them when
> possible is good practice. For what --as-needed does see ld(1).
As I understand, this with this flag linker does not make dynamically
linked binary depend on library, that was passed on command line, but
none of its symbols is used.
If this is correct, question is why build system system links unused
library in first place.
> > 2. I think manpage in wrong direction should be patched, not overrided.
> >
> > On package in general (does not block upload). These issues were present
> > before your changes, but it would be nice to solve them.
>
> I was expecting this comment :-)
>
> I don't really oppose patching the manpage section, and I'm willing to
> do so should you suggest it again in the next review round, but I
> personally don't like the idea very much.
> [...]
Convincing. Let there be override.
> > 3. What is "irqbalance-ui"? It has no manpage, not referenced in
> >irqbalance(1) and even after source diving I can't understand how to
> >use it.
>
> I'll probably add a stub manpage for it.
Good.
> > 4. Situation with "oneshot" option is quite... inconvenient.
> >
> >* If you have ONESHOT option set, `/etc/init.d/irqbalance status`
> > will report failure. It can be fixed by checking for ONESHOT
> > variable in status) clause;
>
> Looks reasonable.
>
> >* current runscript does not respect ONESHOT option. It can be fixed
> > with something like
> >
> >#!/bin/sh
> >. /etc/default/irqbalance
> >if [ -n "${ONESHOT:-}" ] ; then
> >irqbalance --oneshot
> >sv down irqbalance
> >else
> >exec irqbalance --foreground
> >fi
> >
> > but it would be quite unnatural. I think proper solution would be
> > separation of /etc/init.d/irqbalance and
> > /etc/init.d/irqbalance-oneshot.
>
> I agree calling `sv down` from the runscript is ugly, on the other side
> I don't really like the idea of having two sets of init scripts for
> three init systems. Moreover while dh_installsystemd and dh_installinit
> have a --no-enable option, dh_unit does not, requiring some manual
> (hacky) handling.
Runit does not have support for "one-shot" invocations by design: it is
process supervisor, after all. But I agree, doubling number of initfiles
for three init systems is considerable burden.
So please check for $ONESHOT in init.d script, I will provide patch for
runscript after.
> > 5. Do we really need debconf to configure oneshot feature? Debconf
> > question block installation process, so they are not to be used
> > lightly, imho. Even ssh server does not use debconf to make me
> > review its config, which is of much more importance.
>
> I am more than happy with dropping it entirely.
Good.
> I'll ping you again when the updated branch is ready to be reviewed
> again. Thanks again.
Sure.
--
Note, that I send and fetch email in batch, once in a few days.
Please, mention in body of your reply when you add or remove recepients.
Bug#932438: RFS: irqbalance/1.6.0-1 [ITA] -- Daemon to balance interrupts for SMP systems
Paride Legovini wrote on 21/07/2019:
> Dmitry Bogatov wrote on 20/07/2019:
>>* current runscript does not respect ONESHOT option. It can be fixed
>> with something like
>>
>>#!/bin/sh
>>. /etc/default/irqbalance
>>if [ -n "${ONESHOT:-}" ] ; then
>>irqbalance --oneshot
>>sv down irqbalance
>>else
>>exec irqbalance --foreground
>>fi
>>
>> but it would be quite unnatural. I think proper solution would be
>> separation of /etc/init.d/irqbalance and /etc/init.d/irqbalance-oneshot.
>
> I agree calling `sv down` from the runscript is ugly, on the other side
> I don't really like the idea of having two sets of init scripts for
> three init systems. Moreover while dh_installsystemd and dh_installinit
> have a --no-enable option, dh_unit does not, requiring some manual
> (hacky) handling.
Looks like "sv once" is not really meant for "oneshot" services, and
runit seems to be lacking a proper "oneshot", see:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=787363
(a bug you know). What about explicitly not supporting the "oneshot"
mode in runit, documenting the lack of support?
Paride
Bug#932438: RFS: irqbalance/1.6.0-1 [ITA] -- Daemon to balance interrupts for SMP systems
Hi Dmitry, thanks for your review.
Dmitry Bogatov wrote on 20/07/2019:
> [2019-07-19 12:57] Paride Legovini
>> Package: sponsorship-requests
>> Severity: normal
>>
>> Please review my packaging branch for irqbalance/1.6.0-1 at:
>>
>> https://salsa.debian.org/paride-guest/irqbalance
>> [...]
>
> On your changes:
>
> 1. In [942ed5e] you added this line:
>
> export DEB_LDFLAGS_MAINT_APPEND = -Wl,--as-needed
>
>Why is it needed? Maybe, these flags should be provided by
>dpkg-buildflags(1)?
dpkg-buildflags does not currently provide the --as-needed flag to the
linker, see:
https://wiki.debian.org/HardeningWalkthrough
or just try. The flags are not needed but I think using them when
possible is good practice. For what --as-needed does see ld(1).
> 2. I think manpage in wrong direction should be patched, not overrided.
>
> On package in general (does not block upload). These issues were present
> before your changes, but it would be nice to solve them.
I was expecting this comment :-)
I don't really oppose patching the manpage section, and I'm willing to
do so should you suggest it again in the next review round, but I
personally don't like the idea very much.
It's nice to have the manpage in the "right" section, but I also value
consistency across distributions, operating systems, and with upstream,
when it makes sense. Irqbalance has its manpage in section 1 in every
supported operating system; is fixing this small annoyance worth
deviating? What if some README file, code comment, or the manpage itself
references to the manpage as irqbalance(1)? We should patch all the
references too, and double-check every future upstream release. Is the
maintenance burden justified? In 5 years from now will we end up with a
Debian package sometimes referencing to section 1, sometimes to section
8, with no apparent reason?
Now, irqbalance is a relatively simple package, it would be easy to keep
the patch up to date, but you see my general point.
I *did* want to get it fixed, and even submitted a PR for it:
https://github.com/Irqbalance/irqbalance/pull/110
but I think upstream's objection is valid.
> 3. What is "irqbalance-ui"? It has no manpage, not referenced in
>irqbalance(1) and even after source diving I can't understand how to
>use it.
I'll probably add a stub manpage for it.
> 4. Situation with "oneshot" option is quite... inconvenient.
>
>* If you have ONESHOT option set, `/etc/init.d/irqbalance status`
> will report failure. It can be fixed by checking for ONESHOT
> variable in status) clause;
Looks reasonable.
>* current runscript does not respect ONESHOT option. It can be fixed
> with something like
>
>#!/bin/sh
>. /etc/default/irqbalance
>if [ -n "${ONESHOT:-}" ] ; then
>irqbalance --oneshot
>sv down irqbalance
>else
>exec irqbalance --foreground
>fi
>
> but it would be quite unnatural. I think proper solution would be
> separation of /etc/init.d/irqbalance and /etc/init.d/irqbalance-oneshot.
I agree calling `sv down` from the runscript is ugly, on the other side
I don't really like the idea of having two sets of init scripts for
three init systems. Moreover while dh_installsystemd and dh_installinit
have a --no-enable option, dh_unit does not, requiring some manual
(hacky) handling.
I found an answer of stackoverflow suggesting to run "sv once" in the
runscript to set the expected mode. What do you think of this solution?
[0]
https://stackoverflow.com/questions/17035341/use-runit-to-only-boot-up-the-service-and-not-supervise
> 5. Do we really need debconf to configure oneshot feature? Debconf
> question block installation process, so they are not to be used
> lightly, imho. Even ssh server does not use debconf to make me
> review its config, which is of much more importance.
I am more than happy with dropping it entirely.
I'll ping you again when the updated branch is ready to be reviewed
again. Thanks again.
Paride
Bug#932438: RFS: irqbalance/1.6.0-1 [ITA] -- Daemon to balance interrupts for SMP systems
[2019-07-19 12:57] Paride Legovini
> Package: sponsorship-requests
> Severity: normal
>
> Please review my packaging branch for irqbalance/1.6.0-1 at:
>
> https://salsa.debian.org/paride-guest/irqbalance
> [...]
On your changes:
1. In [942ed5e] you added this line:
export DEB_LDFLAGS_MAINT_APPEND = -Wl,--as-needed
Why is it needed? Maybe, these flags should be provided by
dpkg-buildflags(1)?
2. I think manpage in wrong direction should be patched, not overrided.
On package in general (does not block upload). These issues were present
before your changes, but it would be nice to solve them.
3. What is "irqbalance-ui"? It has no manpage, not referenced in
irqbalance(1) and even after source diving I can't understand how to
use it.
4. Situation with "oneshot" option is quite... inconvenient.
* If you have ONESHOT option set, `/etc/init.d/irqbalance status`
will report failure. It can be fixed by checking for ONESHOT
variable in status) clause;
* current runscript does not respect ONESHOT option. It can be fixed
with something like
#!/bin/sh
. /etc/default/irqbalance
if [ -n "${ONESHOT:-}" ] ; then
irqbalance --oneshot
sv down irqbalance
else
exec irqbalance --foreground
fi
but it would be quite unnatural. I think proper solution would be
separation of /etc/init.d/irqbalance and /etc/init.d/irqbalance-oneshot.
5. Do we really need debconf to configure oneshot feature? Debconf
question block installation process, so they are not to be used
lightly, imho. Even ssh server does not use debconf to make me
review its config, which is of much more importance.
--
Note, that I send and fetch email in batch, once in a few days.
Please, mention in body of your reply when you add or remove recepients.
Bug#932438: RFS: irqbalance/1.6.0-1 [ITA] -- Daemon to balance interrupts for SMP systems
I fixed the spelling errors lintian complains about upstream: https://github.com/Irqbalance/irqbalance/commit/c30406b2b6f0e9c23b7 Paride
Bug#932438: RFS: irqbalance/1.6.0-1 [ITA] -- Daemon to balance interrupts for SMP systems
Package: sponsorship-requests
Severity: normal
Please review my packaging branch for irqbalance/1.6.0-1 at:
https://salsa.debian.org/paride-guest/irqbalance
In the beginning I thought this was going to be an easy one, it turned
out not to be the case. This means that even the review won't be
trivial. I implemented some opinionated choices while refactoring the
package, so I expect that some discussion will take place, so don't be
afraid of asking for changes or clarifications. Given that the package
is widely used I'd have asked for a review even as a DD.
I dropped most of the maintainer's scripts which are shipped with the
irqbalance package currently in the archive. The service enablement via
debconf is IMHO wrong, and indeed required overriding an Important
lintian tag.
The existing "oneshot" setting mechanism is buggy (try installing
irqbalance 1.5.0-4 from the archive and answer Yes to the "balance all
IRQs once" queston, then check /etc/default/irqbalance: the setting
won't be there) and overengineered. I simplified it a lot, dropping the
mechanism to inherit the "oneshot" setting from the package version in
Squeeze. I think this simplification is worth the very little lack in
support in an unlikely (and discouraged) upgrade scenario. The
configuration file is also handled in a nicer way, making clear what
debconf touched.
Attached is the output of `git request-pull`.
Thank you,
Paride
-- System Information:
Debian Release: bullseye/sid
APT prefers unstable-debug
APT policy: (500, 'unstable-debug'), (500, 'unstable'), (1, 'experimental')
Architecture: amd64 (x86_64)
Kernel: Linux 4.19.0-5-amd64 (SMP w/4 CPU cores)
Kernel taint flags: TAINT_OOT_MODULE, TAINT_UNSIGNED_MODULE
Locale: LANG=en_IE.UTF-8, LC_CTYPE=en_IE.UTF-8 (charmap=UTF-8),
LANGUAGE=en_IE.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled
The following changes since commit f3e6f4cb574fa495a92ff03a761ab44f2bf9e846:
Open new entry in changelog (2019-06-08 10:46:37 +)
are available in the Git repository at:
https://salsa.debian.org/paride-guest/irqbalance.git
for you to fetch changes up to ba4a460c0328ac667a2890970ca41af7ec0e0c0b:
Update d/changelog (2019-07-18 23:02:14 +0200)
Paride Legovini (18):
New upstream version 1.6.0
Drop fix-permissions-for-unix-socket.patch (unnecessary in v1.6.0)
Bump compat level to 12
Hardened build (hardening=+all)
Remove postinst support for ancient version 1.0.5-1
Add dep3 headers to fix-ftbs-on-hppa.patch
Add Pre-Depends: ${misc:Pre-Depends}
Convert d/copyright to DEP5
Add the upstream metadata file
d/rules: drop override_dh_clean
Bump Standards-Version to 4.4.0 (no changes needed)
Drop debian-systemd-service-adaptions.patch
Refactor the maintainer scripts
Update the debconf PO files
Drop stale variable from irqbalance.init
Set myself (Paride Legovini) as Maintainer
lintian-overrides: command-in-sbin-has-manpage-in-incorrect-section
Update d/changelog
configure.ac | 2 +-
cputree.c | 22 ++-
debian/NEWS| 15 +
debian/changelog | 23 +--
debian/control | 7 ++-
debian/copyright | 36 +++
debian/irqbalance.config | 71 --
debian/irqbalance.init | 1 -
debian/irqbalance.lintian-overrides| 14 +++--
debian/irqbalance.postinst | 57 +++--
debian/irqbalance.postrm | 11
debian/irqbalance.preinst | 32 --
debian/irqbalance.templates| 7 ---
.../patches/debian-systemd-service-adaptions.patch | 34 ---
debian/patches/fix-ftbs-on-hppa.patch | 16 +
.../patches/fix-permissions-for-unix-socket.patch | 40
debian/patches/series | 2 -
debian/po/cs.po| 30 -
debian/po/da.po| 30 -
debian/po/de.po| 30 -
debian/po/es.po| 30 -
debian/po/eu.po| 30 -
debian/po/fi.po| 30 -
debian/po/fr.po| 32 --
debian/po/gl.po| 30 -
debian/po/it.po| 30 -
debian/po/ja.po| 30 -
debian/po/nl.po
