Re: [PATCH 1/3] buildprocess: Add extra metadata to meta-* files

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 12568 bytes --]

Hello,

> On 24 May 2021, at 21:23, Robin Roevens <robin.roevens(a)disroot.org> wrote:
> 
> 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). 

I understood that, but my question remains. Why does Zabbix need to start/stop individual services on the firewall?

I just want to understand the use-case so I can help to build a better solution.

> 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).

Noted.

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

To remind me again, what does that entail?

-Michael

> 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.
>