Hi Michael

Michael Tremer schreef op di 18-05-2021 om 16:09 [+0100]:
> 
> 
> > On 14 May 2021, at 21:07, Robin Roevens <robin.roevens(a)disroot.org>
> > wrote:
> > 
> > Hi
> > 
> > Michael Tremer schreef op vr 14-05-2021 om 13:24 [+0100]:
> > > Hello Jonatan,
> > > 
> > > Yes I get your point. Just because a package has more than one
> > > initscript does not mean they both should be listed in
> > > services.cgi.
> > > 
> > > It is a weird corner case, but how do we want to handle this?
> > > 
> > > -Michael
> > 
> > Maybe a slight renaming of my proposed new variable INITSCRIPTS to
> > SERVICES in the LFS files would make it a bit more clear that this
> > concerns only service-initscripts?
> > 
> > The INSTALL_INITSCRIPTS macro could then be renamed to
> > INSTALL_SERVICE_INITSCRIPTS.
> > 
> > If we then also keep the original INSTALL_INITSCRIPT macro, that
> > macro
> > can  then be used to install non-service initscripts if required.
> > Or
> > even to install all required initscripts in the occasion that you
> > want
> > to skip some service-initscripts defined in SERVICES because they
> > are
> > included in the source, like the situation Jonatan pointed out (in
> > this
> > case the SERVICES variable is filled with the sole purpose to serve
> > as
> > metadata).
> > 
> > So this would then result in:
> > ---
> > define INSTALL_INITSCRIPT
> >   install -m 754 -v $(DIR_SRC)/src/initscripts/packages/$(1) 
> > /etc/rc.d/init.d/$(1)
> > endef
> > 
> > define INSTALL_SERVICE_INITSCRIPTS
> >   for service in $(SERVICES); do \
> >      $(call INSTALL_INITSCRIPT,$$service) || exit 1; \
> >   done
> > endef
> > ---
> > 
> > Optionally we could also add again a variable INITSCRIPTS with the
> > purpose to list non-service initscripts. so
> > SERVICES - for service-initscripts
> > INITSCRIPTS - for non-service-initscripts
> > 
> > and in the meta-* files:
> > ---
> > Name: NAME
> > Summary: SUMMARY
> > ProgVersion: VER
> > Release: RELEASE
> > Size: SIZE
> > Dependencies: DEPS
> > File: NAME-VER-RELEASE.ipfire
> > Services: SERVICES
> > InitScripts: INITSCRIPTS
> > ---
> > 
> > Maybe that info could also come in handy sooner or later ?
> 
> This sounds rather complicated to me.
> 
> What are our objectives here? Does Zabbix want/need to start/stop
> services? And if so, why?

I was not directly thinking of Zabbix specifically here. The objective
is to have a central 'database' of metadata for the pak's that can come
in handy for current and possibly future functionality of IPFire. And a
single way of retrieving this information (both by using "pakfire info"
or directly accessing the pakfire library, in my proposal). 

Knowing which pak's provide which services can be used in service.cgi
(which currently does give users the possibility to start/stop services
indeed) and can be used for monitoring agents like Zabbix, but also
Nagios or possibly others to know which services to monitor.

This instead of my earlier proposal of re-using parts of the
services.cgi code in a separate script for zabbix_agentd, which you
pointed out that should be done cleaner using some central way of
storing/retrieving that information.

Zabbix does have functionality to implement starting/stopping those
services using a context menu in the Zabbix web GUI; and a user can
easily implement that on it's own (as this should be done mainly on
server-side anyway) if he wants to and has the information of which
services should run. This could be done by zabbix server logging into
the IPFire machine using ssh and using addonctrl or by the agent, which
would then need permission to addonctrl start/stop. But I think that is
something for the user to decide on his own as you pointed out
previously, giving zabbix agent root permission on addonctrl start/stop
is a possible extra security risk. So I don't think we should provide
that out of the box. (Maybe it could be a configurable 'setting' in the
IPFire webgui some time in the future, but not something to focus on
currently)

But with that said, I don't really understand your remark here as I
only propose a way for the LFS files to populate the "Services" meta-
data and I only threw in "Initscripts" for non-service initscripts,
just for completeness of the meta-data.
And to prevent duplication of the list of initscripts, I added a macro
to install those in one go instead of again 'defining' this list by
needing to call INSTALL_INITSCRIPT individually for each initscript
while we now have a list of them in a variable. And due to the remark
of Jonatan I now also kept the original INSTALL_INITSCRIPT for the pak
maintainer to use in corner cases.

But this has nothing to do with actually stopping/starting services
other that providing information about which services are installed
(and maybe could be stopped/started if wanted).

If it would make things more clear, I could work my last mail out in an
actual patch ?

Regards
Robin

> 
> > In this case the INSTALL_SERVICE_INITSCRIPTS could again be renamed
> > back to INSTALL_INITSCRIPTS, now accepting a parameter which is a
> > list
> > of initscripts to install, so one could call:
> > ---
> > $(call INSTALL_INITSCRIPTS,$(SERVICES))
> > $(call INSTALL_INITSCRIPTS,$(INITSCRIPTS))
> > ---
> > to install all initscripts.
> > 
> > Or even provide again an additional macro INSTALL_ALL_INITSCRIPTS
> > which
> > would then install both SERVICES and INITSCRIPTS (or even replace
> > the
> > above suggested INSTALL_SERVICE_INITSCRIPTS, so resulting in a
> > macro to
> > install all initscripts (SERVICES and INITSCRIPTS) and a macro to
> > install a single initscript in case some of the ones listed in
> > SERVICES
> > and/or INITSCRIPTS need to be skipped..
> > 
> > But I think the INSTALL_INITSCRIPTS which takes a list of services
> > to
> > install as parameter is probably the most generic one, giving you
> > the
> > possibility to install all SERVICES in one go, and then
> > individually
> > install only those INITSCRIPTS not included in the source if
> > INITSCRIPTS contains such initscript(s) or vice versa.
> > 
> > Robin
> > 
> > > 
> > > > On 12 May 2021, at 19:49, Jonatan Schlag <
> > > > jonatan.schlag(a)ipfire.org>
> > > > wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > > Am 23.04.2021 um 18:16 schrieb Robin Roevens <
> > > > > robin.roevens(a)disroot.org>:
> > > > > 
> > > > > * Introduce SUMMARY and INITSCRIPTS macro's in LFS-
> > > > > makefiles.
> > > > > * Add a Summary and InitScripts field to the meta-* addon
> > > > > files
> > > > > containing the value's of those macro's.
> > > > > * Replace the INSTALL_INITSCRIPT makefile macro by a new
> > > > > INSTALL_INITSCRIPTS macro/method that will install all
> > > > > initscripts
> > > > > defined in the new INITSCRIPTS macro.
> > > > > * Adapt libvirt and zabbix_agentd pak as examples of how to
> > > > > use
> > > > > this.
> > > > > 
> > > > > Signed-off-by: Robin Roevens <robin.roevens(a)disroot.org>
> > > > > ---
> > > > > lfs/Config        | 8 ++++++--
> > > > > lfs/libvirt       | 6 ++++--
> > > > > lfs/zabbix_agentd | 5 ++++-
> > > > > src/pakfire/meta  | 2 ++
> > > > > 4 files changed, 16 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/lfs/Config b/lfs/Config
> > > > > index eadbbc408..61d6f0c82 100644
> > > > > --- a/lfs/Config
> > > > > +++ b/lfs/Config
> > > > > @@ -299,15 +299,19 @@ define PAK
> > > > >   # Create meta file
> > > > >   sed \
> > > > >       -e "s/NAME/$(PROG)/g" \
> > > > > +        -e "s/SUMMARY/$(SUMMARY)/g" \
> > > > >       -e "s/VER/$(VER)/g" \
> > > > >       -e "s/RELEASE/$(PAK_VER)/g" \
> > > > >       -e "s/DEPS/$(DEPS)/g" \
> > > > >       -e "s/SIZE/$$(stat --format=%s
> > > > > /install/packages/$(PROG)-
> > > > > $(VER)-$(PAK_VER).ipfire)/g" \
> > > > > +        -e "s/INITSCRIPTS/$(INITSCRIPTS)/g" \
> > > > >     < /usr/src/src/pakfire/meta > /install/packages/meta-
> > > > > $(PROG)
> > > > > endef
> > > > > 
> > > > > -define INSTALL_INITSCRIPT
> > > > > -    install -m 754 -v
> > > > > $(DIR_SRC)/src/initscripts/packages/$(1) 
> > > > > /etc/rc.d/init.d/$(1)
> > > > > +define INSTALL_INITSCRIPTS
> > > > > +    for initscript in $(INITSCRIPTS); do \
> > > > > +        install -m 754 -v
> > > > > $(DIR_SRC)/src/initscripts/packages/$$initscript 
> > > > > /etc/rc.d/init.d/$$initscript; \
> > > > > +    done
> > > > > endef
> > > > > 
> > > > > ifeq "$(BUILD_ARCH)" "$(filter $(BUILD_ARCH),aarch64
> > > > > riscv64)"
> > > > > diff --git a/lfs/libvirt b/lfs/libvirt
> > > > > index 28a95d317..be5d3db3a 100644
> > > > > --- a/lfs/libvirt
> > > > > +++ b/lfs/libvirt
> > > > > @@ -25,6 +25,7 @@
> > > > > include Config
> > > > > 
> > > > > VER        = 6.5.0
> > > > > +SUMMARY       = Server side daemon and supporting files for
> > > > > libvirt
> > > > > 
> > > > > THISAPP    = libvirt-$(VER)
> > > > > DL_FILE    = $(THISAPP).tar.xz
> > > > > @@ -37,6 +38,8 @@ PAK_VER    = 25
> > > > > 
> > > > > DEPS       = ebtables libpciaccess libtirpc libyajl ncat qemu
> > > > > 
> > > > > +INITSCRIPTS= libvirtd virtlogd
> > > > > +
> > > > > #############################################################
> > > > > ######
> > > > > ############
> > > > > # Top-level Rules
> > > > > #############################################################
> > > > > ######
> > > > > ############
> > > > > @@ -121,8 +124,7 @@ $(TARGET) : $(patsubst
> > > > > %,$(DIR_DL)/%,$(objects))
> > > > >   cd $(DIR_APP)/build_libvirt && make install
> > > > > 
> > > > >   #install initscripts
> > > > > -    $(call INSTALL_INITSCRIPT,libvirtd)
> > > > > -    $(call INSTALL_INITSCRIPT,virtlogd)
> > > > > +    @$(INSTALL_INITSCRIPTS)
> > > > >   mv /usr/libexec/libvirt-guests.sh /etc/rc.d/init.d/libvirt-
> > > > > guests
> > > > And here my approach maybe breaks :-). How could we handle
> > > > this? It
> > > > is not a daemon, more something like a startup/shutdown scripts
> > > > for
> > > > machines... (And should not appear in the service.cgi). So
> > > > parsing
> > > > the root file may yield wrong results and your way would be the
> > > > better one. 
> > > > 
> > > > Jonatan
> > > > > 
> > > > >   # Backup
> > > > > diff --git a/lfs/zabbix_agentd b/lfs/zabbix_agentd
> > > > > index c69643a54..a72fe024b 100644
> > > > > --- a/lfs/zabbix_agentd
> > > > > +++ b/lfs/zabbix_agentd
> > > > > @@ -25,6 +25,7 @@
> > > > > include Config
> > > > > 
> > > > > VER        = 4.2.6
> > > > > +SUMMARY    = Zabbix Agent
> > > > > 
> > > > > THISAPP    = zabbix-$(VER)
> > > > > DL_FILE    = $(THISAPP).tar.gz
> > > > > @@ -35,6 +36,8 @@ PROG       = zabbix_agentd
> > > > > PAK_VER    = 4
> > > > > DEPS       =
> > > > > 
> > > > > +INITSCRIPTS= zabbix_agentd
> > > > > +
> > > > > #############################################################
> > > > > ######
> > > > > ############
> > > > > # Top-level Rules
> > > > > #############################################################
> > > > > ######
> > > > > ############
> > > > > @@ -106,7 +109,7 @@ $(TARGET) : $(patsubst
> > > > > %,$(DIR_DL)/%,$(objects))
> > > > >   chown zabbix.zabbix /var/run/zabbix
> > > > > 
> > > > >   # Install initscripts
> > > > > -    $(call INSTALL_INITSCRIPT,zabbix_agentd)
> > > > > +    @$(INSTALL_INITSCRIPTS)
> > > > > 
> > > > >   # Install sudoers include file
> > > > >   install -v -m 644 $(DIR_SRC)/config/zabbix_agentd/sudoers \
> > > > > diff --git a/src/pakfire/meta b/src/pakfire/meta
> > > > > index d97b2a0fa..849b9cd6c 100644
> > > > > --- a/src/pakfire/meta
> > > > > +++ b/src/pakfire/meta
> > > > > @@ -1,6 +1,8 @@
> > > > > Name: NAME
> > > > > +Summary: SUMMARY
> > > > > ProgVersion: VER
> > > > > Release: RELEASE
> > > > > Size: SIZE
> > > > > Dependencies: DEPS
> > > > > File: NAME-VER-RELEASE.ipfire
> > > > > +InitScripts: INITSCRIPTS
> > > > > -- 
> > > > > 2.31.1
> > > > > 
> > > > > 
> > > > > -- 
> > > > > Dit bericht is gescanned op virussen en andere gevaarlijke
> > > > > inhoud door MailScanner en lijkt schoon te zijn.
> > > 
> > > 
> > 
> > 
> > -- 
> > Dit bericht is gescanned op virussen en andere gevaarlijke
> > inhoud door MailScanner en lijkt schoon te zijn.
> 
> 


-- 
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.