On 24.11.2015 17:21, Petr Viktorin wrote:
On 11/23/2015 10:50 AM, Jan Cholasta wrote:
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.

That would assume the output of *everything* run via ipautil.run can be
decoded using the locale encoding. Any stray invalid byte would make IPA
crash, even in cases where we don't care about the output. IPA calls too
many weird tools to assume they all output text.

Such a stray invalid byte may bubble through our code and cause havoc somewhere else, which is much harder to troubleshoot than a crash in ipautil.run(), where you can see that it was a misbehaving command that caused the crash and exactly what command it was.

If we don't care about output somewhere, we should not capture it there at all.

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.

Non-strict decoding is meant for logging, so we want backslash escapes
rather than invalid surrogates. If you need round-tripping then you
should use bytes.

I see. There is no guarantee that non-strict output will be used only for logging though.

I think it might be better to return raw output and let the caller use a decode-for-logging utility function instead. The logger already escapes invalid bytes itself, so the function would have to be used only when raising exceptions.

I've changed the error strategy to 'replace' though, as I realized
'backslashreplace' doesn't work for decoding.

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to