Re: duplicate get_aliases subroutines found

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

Hello Adolf,

Welcome to the hell of the network code of IPFire 2.

I am sure there are lot more places where the aliases table is parsed but it is not even done in a function. This is why changing anything is super painful because there so many places in the code that do the same thing, but they don’t use a unique function. Therefore we will have to find all these places and update them all the same time. And it isn’t just copy & paste.

There was the intention to clean this up properly, but we simply never found the time for it. On the other hand, this works, and we don’t intend to make too many changes in the future, so putting all of this work in might never pay off.

That statement comes with the limitation that there are of course hotter paths in the code than others.

Regarding this specific function, I first of all think it should live in network-functions.pl, because it clearly is a networking function and is not only being used in the IDS.

The function in ids-functions.pl looks cleaner to me:

  https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/cfgroot/ids-functions.pl;hb=2112342dd3ccaf6008c742dddd4ca26b17c5651d#l1764

It even checks if the line can be read correctly and contains a valid IP address. After parsing the file it returns an array.

The function in general-functions.pl is a lot older:

  https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/cfgroot/general-functions.pl;hb=2112342dd3ccaf6008c742dddd4ca26b17c5651d#l261

It parses the line, even adds some sort of prefix, but does not return an array. Instead it populates a hash that has been declared somewhere else. It returns nothing. It gets called in a few places and the data is than being read from that globally declared array:

  https://git.ipfire.org/?p=ipfire-2.x.git&a=search&h=2112342dd3ccaf6008c742dddd4ca26b17c5651d&st=grep&s=get_aliases

This all would have to be changed if we wanted to adopt the better version of this function in ids-functions.pl.

Here is a list of other places where /var/ipfire/ethernet/aliases is being parsed (never mind the shell parts):

  https://git.ipfire.org/?p=ipfire-2.x.git&a=search&h=2112342dd3ccaf6008c742dddd4ca26b17c5651d&st=grep&s=ethernet%2Faliases

I know that the firewall code heavily relies on the prefix that is being added in the one function, because the actual IP address (which would be a unique key) is not being used when an alias is being selected in a firewall rule.

So long story short, I would be happy to see this cleaned up; but in this rather simple case of a very straight-forward ten-ish lines long function, you will probably have to spend the best part of a day to make sure that no new regressions will be added.

—

That being said, I think there are a lot more lower hanging fruits:

subtocidr/cidrtosub/iporsubtodec/iporsubtocidr/ip2dec/dec2ip/validipandmask and so on are only aliases that call the right function in network-functions.pl. So those will just have to be found in the code and replaced by the new function. Maybe that would be a good start to get general-functions.pl smaller?

Best,
-Michael

> On 8 Mar 2025, at 13:51, Adolf Belka <adolf.belka@ipfire.org> wrote:
> 
> Hi All,
> 
> I was searching through the bugs list and found bug11393 about refactoring the network functions.
> 
> Based on that I was having a look at what network functions were still in general-functions.pl and on investigating I found that there is a sub get_aliases in general-functions.pl and also in ids-functions.pl
> 
> They are clearly doing the same thing but with some differences. I am not able to tell which is the better version to keep or if a version combining aspects of both are needed.
> 
> If it is just a case of using one of them and putting it into the network-functions.pl library and then updating all the references, then I could do that without problems. If something needs to be changed then I would need some guidance.
> 
> Of course the above also depends on get_aliases being considered to be a network function, which was my interpretation.
> 
> I found quite a lot that are still in general-functions.pl
> 
> Regards,
> 
> Adolf.
> 
> -- 
> Sent from my laptop
> 
>