On 25.11.2015 15:47, Petr Viktorin wrote:
On 11/25/2015 11:04 AM, Jan Cholasta wrote:
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.
The invalid byte can't bubble through code, because if you don't specify
an encoding you get bytes back. You'll get a crash when you try using it
as a string.
Invalid bytes *can* bubble through our code, into configuration files
and other external places. The fact that it has not caused much grief so
far suggests that it should be safe to use the locale encoding for most
commands.
If we do subscribe to the statement that any command can output invalid
bytes at any time, I don't see a point in handling encoding in
ipautil.run() at all - just always return bytes and let the caller worry
about encoding and handling related errors.
locale.getpreferredencoding() is not used consistently in all software,
especially if it's not written in Python. For instance, wget won't
magically re-encode the data it fetches for us. It's better to
explicitly specify the encoding every time.
I would say it is used consistently in most software used by IPA. The
wget example is obviously not relevant to this discussion, since it's
returning external data.
IMO setting the encoding to 'ascii' without also setting LC_ALL=C in the
environment, as seen frequently in your patch, has even higher
probability of breaking something than using the locale encoding.
If we don't care about output somewhere, we should not capture it there
at all.
Then people need to remember to put "capture_output=True" everywhere
(except that also disables logging of the output, so we'd need an
additional option).
capture_output=True is the default and capture_output=False should be
set where output is not needed.
IMO the output should always be logged if possible, regardless of
capture_output value.
--
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
