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 KAction@debian.org
[2019-07-21 11:36] Paride Legovini <pl@ninthfloor.org>
> > 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.
Reply to: