[Freeipa-devel] [PATCH] 0748 Handle encoding for ipautil.run

[Jan Cholasta]

On 23.11.2015 07:43, Jan Cholasta wrote:
> On 19.11.2015 00:55, Petr Viktorin wrote:
>> On 11/03/2015 02:39 PM, Petr Viktorin wrote:
>>> Hello,
>>> Python 3's strings are Unicode, so data coming to or leaving a Python
>>> program needs to be decoded/encoded if it's to be handled as a string.
>>> One of the boundaries where encoding is necessary is external programs,
>>> specifically, ipautil.run.
>>> Unfortunately there's no one set of options that would work for all
>>> run() invocations, so I went through them all and specified the
>>> stdin/stdout/stderr encoding where necessary. I've also updated the call
>>> sites to make it clearer if the return values are used or not.
>>> If an encoding is not set, run() will accept/return bytes. (This is a
>>> fail-safe setting, since it can't raise errors, and using bytes where
>>> strings are expected generally fails loudly in py3.)
>>>
>>> Note that the changes are not effective under Python 2.
>>
>> ping,
>> Could someone look at this patch?
>
> Looking.

1) I think a better approach would be to use str for stdin/stdout/stderr 
by default in both Python 2 and 3, i.e. do nothing in Python 2 and 
encode/decode using locale.getpreferredencoding() in Python 3. This is 
consistent with sys.std{in,out,err} in both Python 2 and 3. Using bytes 
or different encoding should be opt-in.

Note that different encoding should be used only if the LC_ALL or LANG 
variables are overriden in the command's environment.


2) The str() here is wrong:

+    arg_string = nolog_replace(' '.join(str(shell_quote(a)) for a in 
args), nolog)

It converts b'data' to "b'data'" in Python 3.

Popen accepts both bytes and text in both Python 2 and 3, so there is 
nothing to be done for bytes arguments.


3) I think the "surrogateescape" error handler would be better for 
non-strict decoding, as the text can be encoded back to bytes without 
loss of information.


4) There are direct calls to subprocess.Popen() in a few places, have 
you checked them as well?


-- 
Jan Cholasta