search

runfalk / synology-wireguard

WireGuard support for some Synology NAS drives
MIT License
919 stars 131 forks source link

Apply memneq patch only for platforms that require it #67

Closed Matige closed 3 years ago

Matige commented 3 years ago

I suggest applying memneq patch by default only for platforms that require it. crypto_memneq is included in the kernel from 3.13. All newly Synology NAS have a 4.4.x kernel, so there will be no need to change the code for new devices. The list of platforms was prepared on the basis of data from the platforms file of the pkgscripts-ng. The only exception is kvmx64 (Virtual DSM), which uses linux-4.4.x in its latest version.

runfalk commented 3 years ago

I agree that it makes sense to only enable it for those that need it.

I had an uncommitted change related to that specific line. I just committed them, sorry about that. It seems like none of the architectures require the patch for DSM 7 (I compiled all that are in release.sh). So I think the correct way is to only enable it for the architectures in your patch if the DSM_VER is 6.[0-9]+.

Should probably rename the env var instead and call it APPLY_MEMNEQ_PATCH. It's a bit weird to have flags that you have to specifically disable.

Would you be fine with updating the PR?

Matige commented 3 years ago

I have updated the pull request. The code is now more flexible. It searches for crypto_memneq function definitions in tool chain and if it does not find it, the patch is applied. I checked the code and works for DMS 6.2 and 7.0 (for all platforms from release.sh).

There was an error in the previous regular expression anyway. The caret char was used only for the first word and the dollar char for the last, so there was a problem with the kvmx64 platform. Expression ^bromolow|x64|cedarview$ is true for kvmx64. In this case, the expression should be ^(bromolow|x64|cedarview)$.

runfalk commented 3 years ago

Nice catch. I agree that this solution is a lot nicer.