[prev in list] [next in list] [prev in thread] [next in thread]
List: ipfire-development
Subject: Re: [PATCH v3 0/5] Fix Bug#12935 + cosmetic changes/enhancements
From: Robin Roevens <robin.roevens () disroot ! org>
Date: 2022-10-28 10:26:33
Message-ID: a65bd3c961a82ba045f7a85ff361d1ace5bdf204.camel () sicho ! home
[Download RAW message or body]
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.
> >
>
>
--
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.
[Attachment #3 (text/html)]
<html><head></head><body><div>Hi \
Michael</div><div><span></span></div><div><br></div><div>Michael Tremer schreef op wo \
26-10-2022 om 15:36 [+0100]:</div><blockquote type="cite" style="margin:0 0 0 .8ex; \
border-left:2px #729fcf solid;padding-left:1ex"><div>Hello \
Robin,<br></div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; \
border-left:2px #729fcf solid;padding-left:1ex"><div>On 11 Oct 2022, at 23:01, Robin \
Roevens <<a href="mailto:robin.roevens@disroot.org">robin.roevens@disroot.org</a>> \
wrote:<br></div><div><br></div><div>Hi all<br></div><div><br></div><div>After \
carefully reviewing and adopting all comments Michael had about<br></div><div>the \
code in previous patchset, here is again a new version of \
it.<br></div><div><br></div><div>Only PATCH 1 was changed since v2:<br></div><div>- \
propper initialization of variables where required preventing \
possible<br></div><div> undefined behaviour<br></div><div>- more reliable error \
checking: all functions now return a meaningfull<br></div><div> integer one way \
or another to indicate if and what kind of error<br></div><div> happend. No need \
anymore to set errno to 0 manually as it no longer<br></div><div> depended \
on.<br></div><div>- No more checking against NULL making many comparisions easier for \
the<br></div><div> eye<br></div><div>- fix some possible memory \
leaks<br></div><div>- fix a possible unallocation of not yet allocated \
memory<br></div><div>- in case of a system error, a descriptive error is shown \
instead of<br></div><div> the number, using the %m \
directive.<br></div><div><br></div><div>Hoping to pass the required strict evaluation \
this time :-).<br></div></blockquote><div><br></div><div>Yes, I think \
so.<br></div><div><br></div><div>It is a huge patchset, so hard to review, but I am \
happy with it and happy to put my stamp on it :)<br></div><div><br></div><div>Thank \
you very much for your very hard work on this and all those hours that you put \
in.<br></div></blockquote><div><br></div><div>Done with pleasure. Happy to be able to \
dust off my programming skills :-) And happy that my work is \
appreciated.</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; \
border-left:2px #729fcf solid;padding-left:1ex"><div><br></div><div>What is coming \
next? :)<br></div></blockquote><div><br></div><div>I was thinking maybe integrating \
AI enhanced bitcoin trading into pakfire ? :-p</div><div>Joking aside, things on my \
wishlist for now are </div><div>- better integration of the zabbix addon by \
adding UI zabbix-agent log viewing and a UI configuration page for it.</div><div>- \
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)..</div><div>- a redesign of the pakfire \
UI page integrating the summary meta-data</div><div><br></div><div>So you'll probably \
hear me again soon with some difficult to review things and/or discussions with \
decisions to make :-)</div><div><br></div><div>Robin</div><blockquote type="cite" \
style="margin:0 0 0 .8ex; border-left:2px #729fcf \
solid;padding-left:1ex"><div><br></div><div>-Michael<br></div><div><br></div><blockquote \
type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf \
solid;padding-left:1ex"><div>For reference, the content of the summary mail that was \
sent with v1 of<br></div><div>the patch:<br></div><div>---<br></div><div>This \
patchset fixes Bug#12935<br></div><div>(<a \
href="https://bugzilla.ipfire.org/show_bug.cgi?id=12935">https://bugzilla.ipfire.org/s \
how_bug.cgi?id=12935</a>)<br></div><div><br></div><div>Summary:<br></div><div>Addons \
where the initscript does not match the addon-name and addons \
with<br></div><div>multiple initscripts are now listed on services.cgi since \
CU170.<br></div><div>But addonctrl still expected addon name to be equal \
to<br></div><div>initscript name; Hence starting/stopping/enabling/disabling of \
such<br></div><div>addons was not possible.<br></div><div>This has always been like \
that, but that problem was hidden as<br></div><div>services.cgi also did not display \
those addon services.<br></div><div><br></div><div>After discussing this with Adolf \
on the Bug report, we concluded that we<br></div><div>should adapt addonctrl to work \
with the new addon metadata<br></div><div>Services-field \
instead.<br></div><div><br></div><div>I basically rewrote addonctrl to not only use \
the new services metadata<br></div><div>but also to have better errorchecking and \
added the posibility to check<br></div><div>if a service is currently enabled or \
disabled.<br></div><div>As a result services.cgi no longer has to go checking the \
precense of<br></div><div>runlevel initscripts, but can just ask \
addonctrl.<br></div><div>I also added a warning to services.cgi if a runlevel \
initscript does not<br></div><div>exists, to prevent the user from wondering why he \
can't enable a<br></div><div>specific service. (Adolf pointed out some services don't \
install<br></div><div>runlevel initscripts by \
default)<br></div><div><br></div><div>More details in the bugreport and in the \
commit-messages of the \
patches.<br></div><div><br></div><div>Regards<br></div><div>Robin<br></div><div><br></div><div><br></div><div><br></div><div>-- \
<br></div><div>Dit bericht is gescanned op virussen en andere \
gevaarlijke<br></div><div>inhoud door MailScanner en lijkt schoon te \
zijn.<br></div><div><br></div></blockquote><div><br></div><div><br></div></blockquote><br \
/>-- <br />Dit bericht is gescanned op virussen en andere gevaarlijke
<br />inhoud door
<a href="http://www.mailscanner.info/"><b>MailScanner</b></a> en lijkt schoon te \
zijn. </body></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic