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

[Jan Cholasta]

On 14.12.2015 10:49, Petr Viktorin wrote:
> On 12/14/2015 10:29 AM, Jan Cholasta wrote:
>> On 9.12.2015 12:04, Petr Viktorin wrote:
>>> On 12/04/2015 12:49 PM, Jan Cholasta wrote:
>>>> On 4.12.2015 12:43, Petr Viktorin wrote:
>>>>> On 12/04/2015 12:15 PM, Jan Cholasta wrote:
>>>>>> On 4.12.2015 10:53, Petr Viktorin wrote:
>>>>>>> On 12/04/2015 08:51 AM, Jan Cholasta wrote:
>>>>>>>> On 3.12.2015 15:42, Petr Viktorin wrote:
>>>>>>>>> On 12/03/2015 10:55 AM, Jan Cholasta wrote:
>>>>> [...]
>>>>>>>>>> If we do encode/decode in run(), I think there should be a way to
>>>>>>>>>> override the default encoding. My idea was to either accept/return
>>>>>>>>>> only
>>>>>>>>>> bytes and let the caller handle encoding themselves, or to handle
>>>>>>>>>> encoding in run(), but be able to override the defaults.
>>>>>>>>>>
>>>>>>>>>> Given we handle encoding in run(), I would imagine it like this:
>>>>>>>>>>
>>>>>>>>>>          def run(args, stdin=None, raiseonerr=True, nolog=(),
>>>>>>>>>> env=None,
>>>>>>>>>>                  capture_stdout=False, skip_output=False, cwd=None,
>>>>>>>>>>                  runas=None, timeout=None, suplementary_groups=[],
>>>>>>>>>>                  capture_stderr=False, encode_stdout=False,
>>>>>>>>>>                  encode_stderr=False, encoding=None):
>>>>>>>>>>
>>>>>>>>>> i.e. nothing is captured by default, when stdout or stderr are
>>>>>>>>>> captured
>>>>>>>>>> they are returned as bytes by default, when stdout or stderr are
>>>>>>>>>> returned as text they are decoded using the locale encoding by
>>>>>>>>>> default,
>>>>>>>>>> or the encoding can be explicitly specified.
>>>>>>>>>>
>>>>>>>>>> Do you think this is feasible?
>>>>>>>>>
>>>>>>>>> Feasible, yes.
>>>>>>>>> In the majority of cases where the output is needed, we want text
>>>>>>>>> encoded with locale.getpreferredencoding(), so I'd rather make
>>>>>>>>> that the
>>>>>>>>> default rather than bytes.
>>>>>>>>>
>>>>>>>>> Or we could make "encode_stdout" imply "capture_stdout", so you
>>>>>>>>> wouldn't
>>>>>>>>> have to always specify both. (It would be better named
>>>>>>>>> "encoded_stdout"
>>>>>>>>> in that case.)
>>>>>>>>
>>>>>>>> I think it should be OK to decode by default. (IMO it would be
>>>>>>>> best to
>>>>>>>> reverse the logic and name it "raw_stdout", with False default.
>>>>>>>> Do we
>>>>>>>> need "raw_stderr" at all? I don't think we do.)
>>>>>>>
>>>>>>> Actually, now that I think about it: if the result was a namedtuple
>>>>>>> subclass, we can always set extra raw_stdout/raw_stderr attributes on
>>>>>>> it. (Unless skip_output=True, of course.)
>>>>>>>
>>>>>>>        result = run(['generate_binary_data'])
>>>>>>>        use(result.raw_stdout)
>>>>>>>
>>>>>>>        # and for backcompat,
>>>>>>>        rc, _out, _err = result
>>>>>>
>>>>>> Good idea.
>>>>>>
>>>>>> Perhaps we should use the same names as CalledProcessError for the
>>>>>> attributes, i.e. "returncode", "output", and "error_output",
>>>>>> "raw_output", "raw_error_output" for the new attributes?
>>>>
>>>> (Actually, since "returncode" is not "return_code", it should probably
>>>> be "returncode", "output", "erroroutput", "raw_output",
>>>> "raw_erroroutput".)
>>>>
>>>>>
>>>>> OK. It's also good to use different names than Popen's stdout/stderr,
>>>>> which are file-like objects rather than strings. But then,
>>>>> capture_stdout/capture_stderr should be
>>>>> capture_output/capture_error_output.
>>>
>>> Here is the new patch.
>>> I also added output_log and error_log to the result to abstract the
>>> following common pattern:
>>>
>>>       result = run([...])
>>>       if result.returncode != 0:
>>>           stderr = result.error_output
>>>           if six.PY3:
>>>               error = stderr.encode(locale.getpreferredencoding(),
>>> 'replace')
>>>           self.log.critical('...: %s', stderr)
>>
>> 1) certutil -L -r outputs raw DER cert, so this will explode in Python 3
>> when pem == False:
>>
>>           else:
>>               args.append('-r')
>>           try:
>> -            cert, err, returncode = self.run_certutil(args)
>> +            result = self.run_certutil(args, capture_output=True)
>>           except ipautil.CalledProcessError:
>>               raise RuntimeError("Failed to get %s" % nickname)
>> -        return cert
>> +        return result.output
>>
>>       def has_nickname(self, nickname):
>>           try:
>>
>>
>> 2) There are some PEP8 transgressions:
>>
>> $ git show -U0 | pep8 --diff
>> ./ipaplatform/base/services.py:297:22: E127 continuation line
>> over-indented for visual indent
>> ./ipapython/ipautil.py:279:1: E302 expected 2 blank lines, found 1
>> ./ipapython/ipautil.py:280:40: E128 continuation line under-indented for
>> visual indent
>> ./ipapython/ipautil.py:283:1: E302 expected 2 blank lines, found 1
>> ./ipapython/ipautil.py:330:80: E501 line too long (80 > 79 characters)
>> ./ipapython/ipautil.py:437:43: E127 continuation line over-indented for
>> visual indent
>> ./ipapython/ipautil.py:442:43: E127 continuation line over-indented for
>> visual indent
>> ./ipapython/kernel_keyring.py:57:11: E225 missing whitespace around
>> operator
>> ./ipaserver/install/cainstance.py:626:80: E501 line too long (80 > 79
>> characters)
>> ./ipaserver/install/ipa_backup.py:498:17: E128 continuation line
>> under-indented for visual indent
>> ./ipaserver/install/ipa_backup.py:521:21: E122 continuation line missing
>> indentation or outdented
>> ./ipaserver/install/ipa_backup.py:530:17: E122 continuation line missing
>> indentation or outdented
>> ./ipaserver/install/ipa_backup.py:605:52: E225 missing whitespace around
>> operator
>> ./ipaserver/install/ipa_backup.py:606:17: E122 continuation line missing
>> indentation or outdented
>> ./ipaserver/install/krbinstance.py:303:80: E501 line too long (84 > 79
>> characters)
>> ./ipatests/test_ipapython/test_keyring.py:97:33: E222 multiple spaces
>> after operator
>>
>>
>> 3) The patch needs a rebase.
>>
>>
>> I took the liberty of fixing all of the above, see the attached patch.
>>
>> ACK from me. If you are OK with the changes, I will push the patch.
>
> I am, ACK. Thank you!

Pushed to master: 099cf98307d4b2f0ace5d5e28754f264808bf59d

-- 
Jan Cholasta