search

WKnak / fail2ban-block-ip-range

A python script to block attacks from a network range address, from CIDR /23 up to /31
24 stars 15 forks source link

compatibility issue with python 3.6 #16

Open jaccol opened 4 months ago

jaccol commented 4 months ago

Hi,

I notice a last compatibility issue with python 3.6 (and probably earlier) I get an error TypeError: __init__() got an unexpected keyword argument 'capture_output'

The issue is in lines 193 and 201 in the subprocess.run() function. Both capture_output and text are new additions to the function per python version 3.7. I Believe that stdout=PIPE, stderr=PIPE, universal_newlines=True is identical and is supported in older versions of python. Obviously PIPE needs to be imported from subprocess.

Jacco

pbiering commented 4 months ago

this was introduced by https://github.com/WKnak/fail2ban-block-ip-range/commit/30bbd7594ab13462fc3f38b91acd9f54c527726a

I will take care adding next fallback support, keep you updated

jaccol commented 4 months ago

Hi Peter,

Thanks for the quick PR. 2 things though:

  1. I think (untested) that you miss the stderr=PIPE in the second run command.
  2. I was under the impression, reading through the documentation, that the alternative I gave was identical, but supported by more python versions. I thought that there was no need for the discrimination. (But your code will work)

If capture_output is true, stdout and stderr will be captured. When used, the internal Popen object is automatically created with stdout and stdin both set to PIPE. The stdout and stderr arguments may not be supplied at the same time as capture_output. If you wish to capture and combine both streams into one, set stdout to PIPE and stderr to STDOUT, instead of using capture_output.

and

Changed in version 3.7: Added the text parameter as an alias for universal_newlines.

(https://docs.python.org/3/library/subprocess.html)

Hmm, rereading the docs I might have mixed-up stdin and stderr in the second sentence. After some test, I think that stderr=PIPE prints intelligible errors, while stderr=STDOUT does not.

Jacco

pbiering commented 4 months ago 
1. I think (untested) that you miss the `stderr=PIPE` in the second `run` command.

Overseen, fixed by https://github.com/WKnak/fail2ban-block-ip-range/pull/17/commits/9e471cc96a0ae697444eebd4d102953b3b361230

2. I was under the impression, reading through the documentation, that the alternative I gave was identical, but supported by more python versions. I thought that there was no need for the discrimination. (But your code will work)

I have zero trust in Python related to long term API stability in their functions, that's why I'm using Python version dependent coding...who knows when one decide to discard the support of legacy options in a future Python version...with current coding this would not harm.