duplicate get_aliases subroutines found

duplicate get_aliases subroutines found

[Adolf Belka]

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

Re: duplicate get_aliases subroutines found

[Michael Tremer]

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

Re: duplicate get_aliases subroutines found

[Adolf Belka]

Hi Michael,

On 10/03/2025 11:22, Michael Tremer wrote:
> Hello Adolf,
> 
> Welcome to the hell of the network code of IPFire 2.

Oh well, I had to start getting some involvement with it at some time.
> 
> 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.

Yes, that was definitely something I was not sure on how to deal with.
> 
> 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.

I hadn't thought of the link to the aliases file and the consequences related to that.
> 
> 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.

I think for me it would not be a day to make sure of no new regressions but more like a week and also requiring a lot of help on how to figure that out.

> 
> —
> 
> 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?

This definitely does sound more like what I can deal with. I think I will look at this first and then we can see if there is anything else that makes sense to do without a high risk of breaking something that is already working.

I think the stuff that fixes those quoted issues I found in bug12298 look like things I should definitely fix and I remember that I found that some of the cgi files don't use the cleanhtml or escape functions on some of the remarks, so I think those would also be good to put on my list of things to work on.

Thanks for the feedback.
Adolf.

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