- Download Diff

Summary:

10225 wsdiff broken after 9979

Review Request #1362 — Created Jan. 12, 2019 and submitted Jan. 16, 2019, 7:49 p.m.

Information
Owner:	citrus
Repository:	illumos-gate
Branch:	master
Bugs:	10225
Depends On:
Commit:	17f47b8...
Reviewers
Groups:	general
People:

Description

10225 wsdiff broken after 9979

Testing Done

See https://www.illumos.org/issues/10225

Issues

0
0
3
3
All Issues: 6

Description	From	Last Updated
Did we set the locale to a utf-8 aware locale? This is true everywhere we're using utf-8 in this file.	rm	Jan. 14, 2019, 10:24 a.m.
If I am reading the documentation correctly, subprocess.check_output() would raise a CalledProcessError exception in the event that the child process ...	jclulow	Jan. 15, 2019, 10:11 a.m.
So I think this should probably be a binary string, i.e., elfmagic = b'\177ELF', and then we should intead open ...	jclulow	Jan. 15, 2019, 10:17 a.m.
Should this call to open() actually be io.open() as on L458? I'm a bit confused about the difference between them.	jclulow	Jan. 15, 2019, 10:22 a.m.
Same question as above -- subprocess.check_output() used to throw on non-zero exit, but we now seem to be silently ignoring ...	jclulow	Jan. 15, 2019, 10:24 a.m.
Same question as above -- subprocess.check_output() used to throw on non-zero exit, but we now seem to be silently ignoring ...	jclulow	Jan. 15, 2019, 10:26 a.m.

Testing Done:

See https://www.illumos.org/issues/10225

usr/src/tools/scripts/wsdiff.py (Diff revision 1)

Show all issues

Did we set the locale to a utf-8 aware locale? This is true everywhere we're using utf-8 in this file.

citrus Jan. 12, 2019, 4:57 p.m.

I'm using decode() to convert from a bytes object to a string object in a way that's portable across python2 and python3. The thing that is being read is actually stdout from commands like diff so is expected to be plain ASCII (however we do have some extended characters in a few header file comments that can cause issues if one of them was to change in the workspace)

citrus Jan. 12, 2019, 6:55 p.m.

To be clearer, I'm saying that the output of the backend command should be treated as UTF-8, while expecting it to be almost always plain ASCII. decode() will convert that into the local locale but if a character cannot be represented in it then it will be replaced with '?'. This is good enough for the purposes of wsdiff.

Change Summary:

Drop explicit 'utf-8' encoding for input files.

Commit:

-	c7c47702f1a325ea237c5a8dcda617d3a715e0bc
+	c78082fe2f045bdbd905c6bea23e1a670b67c69f

Diff:

Revision 2 (+28 -35)

Show changes

usr/src/tools/scripts/wsdiff.py

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

usr/src/tools/scripts/wsdiff.py (Diff revision 2)

Show all issues

If I am reading the documentation correctly, subprocess.check_output() would raise a CalledProcessError exception in the event that the child process exited with a non-zero code.  This new function, getoutput(), appears to return the value (which is fine) but we then don't seem to check it here.  Does that mean we won't hit the except: block below where a diff failure would otherwise be reported?

citrus Jan. 15, 2019, 10:11 a.m.

This is one of the problems with having to review a fix for a botched previous commit.
Diff is expected to return non-zero status codes (1 if differences are found for example).
The original code (i.e. before 9979) was:
data = commands.getoutput(diff_cmd + " " + tmpf1 + " " + tmpf2)

which is documented as:


...the exit status is ignored and the return value is a string containing the command’s output.

The except cause will still be triggered if any errors are thrown from the Popen stuff in getoutput()

usr/src/tools/scripts/wsdiff.py (Diff revision 2)

Show all issues

So I think this should probably be a binary string, i.e., elfmagic = b'\177ELF', and then we should intead open the file in the binary mode:

with io.open(f, mode='rb') as fd:

Otherwise it seems like we could get confused by possible locale and encoding issues by trying to treat the file as text instead of binary.

citrus Jan. 15, 2019, 10:17 a.m.

Agreed, and fixed (and tested with python2 & python3).

usr/src/tools/scripts/wsdiff.py (Diff revision 2)

Show all issues

Should this call to open() actually be io.open() as on L458?  I'm a bit confused about the difference between them.

citrus Jan. 15, 2019, 10:17 a.m.

In python3, io.open() is an alias for open()

However, in python2, open() does not handle encoded files. That is, if the file contains any non-

citrus Jan. 15, 2019, 10:27 a.m.

Trying again.
In python3, io.open() is an alias for open()

However, in python2, open() does not handle encoding. That is, unless the data is pure ASCII, io.open() should be used. However, when opening a file in binary mode ('rb') it is fine to use open().

For consistency with isELF(), I'll change this to use io.open().

usr/src/tools/scripts/wsdiff.py (Diff revision 2)

Show all issues

Same question as above -- subprocess.check_output() used to throw on non-zero exit, but we now seem to be silently ignoring the rc value here.

citrus Jan. 15, 2019, 10:27 a.m.

As above, the original code used commands.getoutput() - The length of the returned data is checked below.

usr/src/tools/scripts/wsdiff.py (Diff revision 2)

Show all issues

Same question as above -- subprocess.check_output() used to throw on non-zero exit, but we now seem to be silently ignoring the rc value here.

citrus Jan. 15, 2019, 10:27 a.m.

Again, this was originally commands.getoutput().

Whilst I could add a check for the return status, if anything goes wrong with retrieving the output of uname -p then none of the required utilities will be found and wsdiff will abort early. (uname -p output is used to build the /opt/onbld/bin/i386/ path)