Bug#932438: RFS: irqbalance/1.6.0-1 [ITA] -- Daemon to balance interrupts for SMP systems

[Paride Legovini <pl@ninthfloor.org>]

Dmitry Bogatov wrote on 23/07/2019:
> [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.

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