[v3,0/5] Fix Bug#12935 + cosmetic changes/enhancements

  Hi all

After carefully reviewing and adopting all comments Michael had about
the code in previous patchset, here is again a new version of it.

Only PATCH 1 was changed since v2:
- propper initialization of variables where required preventing possible
  undefined behaviour
- more reliable error checking: all functions now return a meaningfull
  integer one way or another to indicate if and what kind of error
  happend. No need anymore to set errno to 0 manually as it no longer
  depended on.
- No more checking against NULL making many comparisions easier for the
  eye
- fix some possible memory leaks
- fix a possible unallocation of not yet allocated memory
- in case of a system error, a descriptive error is shown instead of
  the number, using the %m directive.

Hoping to pass the required strict evaluation this time :-).

For reference, the content of the summary mail that was sent with v1 of
the patch:
---
This patchset fixes Bug#12935
(https://bugzilla.ipfire.org/show_bug.cgi?id=12935)

Summary:
Addons where the initscript does not match the addon-name and addons with
multiple initscripts are now listed on services.cgi since CU170.
But addonctrl still expected addon name to be equal to
initscript name; Hence starting/stopping/enabling/disabling of such
addons was not possible.
This has always been like that, but that problem was hidden as
services.cgi also did not display those addon services.

After discussing this with Adolf on the Bug report, we concluded that we
should adapt addonctrl to work with the new addon metadata
Services-field instead.

I basically rewrote addonctrl to not only use the new services metadata
but also to have better errorchecking and added the posibility to check
if a service is currently enabled or disabled.
As a result services.cgi no longer has to go checking the precense of
runlevel initscripts, but can just ask addonctrl.
I also added a warning to services.cgi if a runlevel initscript does not
exists, to prevent the user from wondering why he can't enable a
specific service. (Adolf pointed out some services don't install
runlevel initscripts by default)

More details in the bugreport and in the commit-messages of the patches.

Regards
Robin
  

Bump...nudge nudge .

Robin Roevens <robin.roevens@disroot.org> schreef op 12 oktober 2022 00:01:52 CEST:
>Hi all
>
>After carefully reviewing and adopting all comments Michael had about
>the code in previous patchset, here is again a new version of it.
>
>Only PATCH 1 was changed since v2:
>- propper initialization of variables where required preventing possible
>  undefined behaviour
>- more reliable error checking: all functions now return a meaningfull
>  integer one way or another to indicate if and what kind of error
>  happend. No need anymore to set errno to 0 manually as it no longer
>  depended on.
>- No more checking against NULL making many comparisions easier for the
>  eye
>- fix some possible memory leaks
>- fix a possible unallocation of not yet allocated memory
>- in case of a system error, a descriptive error is shown instead of
>  the number, using the %m directive.
>
>Hoping to pass the required strict evaluation this time :-).
>
>For reference, the content of the summary mail that was sent with v1 of
>the patch:
>---
>This patchset fixes Bug#12935
>(https://bugzilla.ipfire.org/show_bug.cgi?id=12935)
>
>Summary:
>Addons where the initscript does not match the addon-name and addons with
>multiple initscripts are now listed on services.cgi since CU170.
>But addonctrl still expected addon name to be equal to
>initscript name; Hence starting/stopping/enabling/disabling of such
>addons was not possible.
>This has always been like that, but that problem was hidden as
>services.cgi also did not display those addon services.
>
>After discussing this with Adolf on the Bug report, we concluded that we
>should adapt addonctrl to work with the new addon metadata
>Services-field instead.
>
>I basically rewrote addonctrl to not only use the new services metadata
>but also to have better errorchecking and added the posibility to check
>if a service is currently enabled or disabled.
>As a result services.cgi no longer has to go checking the precense of
>runlevel initscripts, but can just ask addonctrl.
>I also added a warning to services.cgi if a runlevel initscript does not
>exists, to prevent the user from wondering why he can't enable a
>specific service. (Adolf pointed out some services don't install
>runlevel initscripts by default)
>
>More details in the bugreport and in the commit-messages of the patches.
>
>Regards
>Robin
>
>
>
>-- 
>Dit bericht is gescanned op virussen en andere gevaarlijke
>inhoud door MailScanner en lijkt schoon te zijn.
>
  

Hello Robin,

> On 11 Oct 2022, at 23:01, Robin Roevens <robin.roevens@disroot.org> wrote:
> 
> Hi all
> 
> After carefully reviewing and adopting all comments Michael had about
> the code in previous patchset, here is again a new version of it.
> 
> Only PATCH 1 was changed since v2:
> - propper initialization of variables where required preventing possible
>  undefined behaviour
> - more reliable error checking: all functions now return a meaningfull
>  integer one way or another to indicate if and what kind of error
>  happend. No need anymore to set errno to 0 manually as it no longer
>  depended on.
> - No more checking against NULL making many comparisions easier for the
>  eye
> - fix some possible memory leaks
> - fix a possible unallocation of not yet allocated memory
> - in case of a system error, a descriptive error is shown instead of
>  the number, using the %m directive.
> 
> Hoping to pass the required strict evaluation this time :-).

Yes, I think so.

It is a huge patchset, so hard to review, but I am happy with it and happy to put my stamp on it :)

Thank you very much for your very hard work on this and all those hours that you put in.

What is coming next? :)

-Michael

> For reference, the content of the summary mail that was sent with v1 of
> the patch:
> ---
> This patchset fixes Bug#12935
> (https://bugzilla.ipfire.org/show_bug.cgi?id=12935)
> 
> Summary:
> Addons where the initscript does not match the addon-name and addons with
> multiple initscripts are now listed on services.cgi since CU170.
> But addonctrl still expected addon name to be equal to
> initscript name; Hence starting/stopping/enabling/disabling of such
> addons was not possible.
> This has always been like that, but that problem was hidden as
> services.cgi also did not display those addon services.
> 
> After discussing this with Adolf on the Bug report, we concluded that we
> should adapt addonctrl to work with the new addon metadata
> Services-field instead.
> 
> I basically rewrote addonctrl to not only use the new services metadata
> but also to have better errorchecking and added the posibility to check
> if a service is currently enabled or disabled.
> As a result services.cgi no longer has to go checking the precense of
> runlevel initscripts, but can just ask addonctrl.
> I also added a warning to services.cgi if a runlevel initscript does not
> exists, to prevent the user from wondering why he can't enable a
> specific service. (Adolf pointed out some services don't install
> runlevel initscripts by default)
> 
> More details in the bugreport and in the commit-messages of the patches.
> 
> Regards
> Robin
> 
> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
>
  

Hi Michael

Michael Tremer schreef op wo 26-10-2022 om 15:36 [+0100]:
> Hello Robin,
> 
> > On 11 Oct 2022, at 23:01, Robin Roevens <robin.roevens@disroot.org>
> > wrote:
> > 
> > Hi all
> > 
> > After carefully reviewing and adopting all comments Michael had
> > about
> > the code in previous patchset, here is again a new version of it.
> > 
> > Only PATCH 1 was changed since v2:
> > - propper initialization of variables where required preventing
> > possible
> >  undefined behaviour
> > - more reliable error checking: all functions now return a
> > meaningfull
> >  integer one way or another to indicate if and what kind of error
> >  happend. No need anymore to set errno to 0 manually as it no
> > longer
> >  depended on.
> > - No more checking against NULL making many comparisions easier for
> > the
> >  eye
> > - fix some possible memory leaks
> > - fix a possible unallocation of not yet allocated memory
> > - in case of a system error, a descriptive error is shown instead
> > of
> >  the number, using the %m directive.
> > 
> > Hoping to pass the required strict evaluation this time :-).
> 
> Yes, I think so.
> 
> It is a huge patchset, so hard to review, but I am happy with it and
> happy to put my stamp on it :)
> 
> Thank you very much for your very hard work on this and all those
> hours that you put in.

Done with pleasure. Happy to be able to dust off my programming skills
:-) And happy that my work is appreciated.

> 
> What is coming next? :)

I was thinking maybe integrating AI enhanced bitcoin trading into
pakfire ? :-p
Joking aside, things on my wishlist for now are 
- better integration of the zabbix addon by adding UI zabbix-agent log
viewing and a UI configuration page for it.
- revisit a few pakfire enhancements which got rejected due to
limitations I didn't count on back then. (like presenting an upgrade
overview before actually installing the upgrade and required space
calculation)..
- a redesign of the pakfire UI page integrating the summary meta-data

So you'll probably hear me again soon with some difficult to review
things and/or discussions with decisions to make :-)

Robin
> 
> -Michael
> 
> > For reference, the content of the summary mail that was sent with
> > v1 of
> > the patch:
> > ---
> > This patchset fixes Bug#12935
> > (https://bugzilla.ipfire.org/show_bug.cgi?id=12935)
> > 
> > Summary:
> > Addons where the initscript does not match the addon-name and
> > addons with
> > multiple initscripts are now listed on services.cgi since CU170.
> > But addonctrl still expected addon name to be equal to
> > initscript name; Hence starting/stopping/enabling/disabling of such
> > addons was not possible.
> > This has always been like that, but that problem was hidden as
> > services.cgi also did not display those addon services.
> > 
> > After discussing this with Adolf on the Bug report, we concluded
> > that we
> > should adapt addonctrl to work with the new addon metadata
> > Services-field instead.
> > 
> > I basically rewrote addonctrl to not only use the new services
> > metadata
> > but also to have better errorchecking and added the posibility to
> > check
> > if a service is currently enabled or disabled.
> > As a result services.cgi no longer has to go checking the precense
> > of
> > runlevel initscripts, but can just ask addonctrl.
> > I also added a warning to services.cgi if a runlevel initscript
> > does not
> > exists, to prevent the user from wondering why he can't enable a
> > specific service. (Adolf pointed out some services don't install
> > runlevel initscripts by default)
> > 
> > More details in the bugreport and in the commit-messages of the
> > patches.
> > 
> > Regards
> > Robin
> > 
> > 
> > 
> > -- 
> > Dit bericht is gescanned op virussen en andere gevaarlijke
> > inhoud door MailScanner en lijkt schoon te zijn.
> > 
> 
>