Thanks for the review Simon.  For the sake of brevity, I'll omit any comments
for issues that I'll just go ahead and fix.

Would you rather see a patch that addresses all your comments, and which
obsoletes the previous patches, or would you rather see one an incremental
patch to apply on top of the ones you've already reviewed?

(I'm still trying to get github to not hate me. ;)

On Dec 12, 2011, at 12:28 PM, <email address hidden> wrote:

>::: PY3PORT.rst
>@@ +38,5 @@
>> + - All object reprs are unicodes.  This change was made because it greatly
>> +   simplifies the implementation and cross-compatibility with Python 3.
>> + - Some values which were ints are now longs.  Primarily, this affects the
>> +   type of the `variant_level` attributes.
>> + - There is a new `dbus._BytesBase` class, but it is unused in Python 2.
>
>I don't think this should count as user-visible; anyone relying on _-prefixed
>stuff in the dbus module was already wrong.

Sounds right, I'll remove this from "user-visible changes".  Since the class
is unused in Python 2, should I #ifdef it out when compiled under Python 2?

>@@ +50,5 @@
>> + - `ByteArray`s must be initialed with bytes objects, not unicodes.  Use `b''`
>> +   literals in the constructor.  This also works in Python 2, where bytes
>> +   objects are aliases for 8-bit strings.
>> + - `ByteArray`s now derive from the new `dbus._BytesBase` class instead of
>> +   `dbus._StrBase`.
>
>If you feel the need to say this, please also say: Like everything with an
>underscore prefix, this is considered to be an implementation detail and may
>change in future versions.

I've replaced this with...

>
>I think the user-visible change you're actually describing is this?
>
>`ByteArray` is now a subclass of `bytes`, where it was previously a subclass
>of `str`.

 - `ByteArray` is now a subclass of `bytes`, where in Python 2 it is a
   subclass of `str`.

>@@ +51,5 @@
>> +   literals in the constructor.  This also works in Python 2, where bytes
>> +   objects are aliases for 8-bit strings.
>> + - `ByteArray`s now derive from the new `dbus._BytesBase` class instead of
>> +   `dbus._StrBase`.
>> + - `dbus.Byte` can be constructed from a 1-character byte or str object.
>
>"... or from an integer", if it still can?

It can.  Fixed description.

>@@ +53,5 @@
>> + - `ByteArray`s now derive from the new `dbus._BytesBase` class instead of
>> +   `dbus._StrBase`.
>> + - `dbus.Byte` can be constructed from a 1-character byte or str object.
>> + - `dbus.UTF8String` is gone.  Use `dbus.String`.
>> + - `dbus._IntBase` is gone.  Use `dbus._LongBase`.
>
>... but don't anyway, because these are private.

I'll just remove this.

>
>@@ +58,5 @@
>> + - All longs are now ints, since Python 3 has only a single int type.  This
>> +   also means that the class hierarchy for the dbus numeric types has changed
>> +   (all derive from int in Python 3).
>> + - Some exception strings have changed.
>> + - `dbus.is_py3` is a new flag that is True when running under Python 3.
>
>This is a change to Python 2 as well, surely? But I'd have called this
>dbus._is_py3 and not documented it.

Fair enough.  There's nothing so magical about that variable that it needs to
be in the public API.  In which case, I can remove this item since it's not a
user-visible change.

>@@ +61,5 @@
>> + - Some exception strings have changed.
>> + - `dbus.is_py3` is a new flag that is True when running under Python 3.
>> + - `dbus.keywords()` is a new function that helps with writing compatible code
>> +   under Python 2 and Python 3.  It filters out `utf8_string` arguments under
>> +   Python 3.
>
>I don't think this deserves to be public API. dbus._adjust_keywords()?

Yes.  Let's also suggest people just stop using `utf8_strings`.

>I think mapping 'int' to 'x' will break real-world code; many D-Bus services
>are not tolerant of integer type mismatches. I'd recommend continuing to map
>int to 'i', even though int is now larger than it used to be. In my
>experience, if people want 'x' they're more likely to ask for it.

So, specifically, that message Message.guess_signature(7) should return
dbus.Signature('i') instead of dbus.Signature('x').  Are there other places
that need fixing?

What would (or should) happen if an interface specified to use 'i' is handed a
Python integer outside that range?  Ideally, an exception would be thrown
instead of data corruption.  It's not clear to me that this is handled, or
where the int value would be checked, or if there are unit tests for this
case.  Any thoughts?

There did not seem to be a test in the test suite either for the specific case
of guessing 'i' for ints, so I added one.  After making this change, I see no
other impact on the test suite, so it's not clear if there's anything else I
need to change.  The new test is in test-standalone.py:

    def test_guess_signature_python_ints(self):
        aeq = self.assertEqual
        from _dbus_bindings import Message
        aeq(Message.guess_signature(7), 'i')
        if not _is_py3:
            aeq(Message.guess_signature(make_long(7)), 'x')


>@@ +175,5 @@
>> +
>> +There were a few places where I noticed what might be considered bugs,
>> +unchecked exception conditions, or possible reference count leaks.  In these
>> +cases, I've just fixed what I can and hopefully haven't made the situation
>> +worse.
>
>You're very very welcome to report bugs for this sort of thing, preferably
>with patches!

Mostly, I just folded them into this mega-patch, or rejiggered the
implementation enough to replace the code.

The one refleak that comes to mind is in DBusPyServer_set_auth_mechanisms().
PySequence_Fast() returns a new reference, but it's never decref'd at the end
of the function.  Not to worry though because when I adjusted the
implementation to handle unicodes coming out of auth_mechanisms, I had to free
the intermediate bytes objects at the end anyway, so I added a decref of
fast_seq.

>@@ +222,5 @@
>> + - Update all C extension docstrings for accuracy.
>> + - Should `dbus.ByteArray` - which is a subclass of `dbus._BytesBase` which is
>> +   itself a subclass of bytes, i.e. str in Python 2 - accept UTF8 encoded
>> +   unicode strings?  Currently, no, they must be bytes in Python 3, str in
>> +   Python 2.
>
>I think this is right. If your string is UTF-8 (without embedded NULs), it
>should usually be a str/unicode in Python and a 's' on D-Bus; and if what you
>want is genuinely ByteArray(s.encode('utf-8')), explicit is better than
>implicit.

I agree!  I'll remove this as an open issue.

>D-Bus requires UTF-8 in strings (as in 's') but has nothing to say about the
>encoding of byte arrays.
>
>::: _dbus_bindings/bytes.c
>@@ +92,5 @@
>> +	if (!obj_as_bytes) {
>> +	    return NULL;
>> +	}
>> +	obj = PyLong_FromLong(
>> +	    (unsigned char)(PyBytes_AS_STRING(obj_as_bytes)[0]));
>
>As far as I can tell, this doesn't check for length 1, so
>
>    Byte(u'\x12xxxxxxxxxxxxx')
>
>is misinterpreted as
>
>    Byte(u'\x12')
>
>which is 0x12. It should be an error, IMO.

Good catch, thanks.  I'll add a test for this and fix it.

>
>@@ +172,5 @@
>> +        PyErr_SetString(PyExc_RuntimeError, "Integer outside range 0-255");
>> +        return NULL;
>> +    }
>> +    str[0] = (unsigned char)i;
>> +    return PyUnicode_FromStringAndSize((char *)str, 1);
>
>Is it OK for tp_str to return a unicode, even in Python 2? (I thought that was
>what tp_unicode was for?)

Yes.  Despite the documentation, in both Python 2.6 and 2.7, it's legal to
return unicodes from tp_str and tp_repr, as well as __str__() and __repr__().
I actually looked at the Python source code to figure out exactly what
happens.  When one of these returns a unicode, it gets turned into an 8-bit
string via PyUnicode_AsEncodedString() with NULL error and encoding arguments.
This means the default encoding will be used, and as shown by
sys.getdefaultencoding() will always be 'ascii' unless someone (e.g. the OS
vendor) hacks site.py to do something different.  Debian/Ubuntu doesn't change
this near as I can tell, and while I can't speak for other distros, I think
they'd be insane to do anything different. ;)

This make sense, exactly for Python 3 portability, though it probably should
be documented.  So even though we can return unicodes from tp_str in both
cases, a Python user will always see strs in both cases.  E.g. 8-bit strings
in Python 2 and unicodes in Python 3.

Since dbus-python's reprs and strs should always be ascii encoded, this should
always be safe.

>::: _dbus_bindings/conn-methods.c
>@@ +562,4 @@
>>      ok = dbus_connection_get_unix_fd (self->conn, &fd);
>>      Py_END_ALLOW_THREADS
>>      if (!ok) Py_RETURN_NONE;
>> +    return PyLong_FromLong(fd);
>
>Is it considered OK for (Unix) file descriptors to be long in Python 2? (For
>instance, does os.fdopen work?)

Yep.  I didn't dig into the code, but here's what happens on Ubuntu:

Python 2.6.7 (r267:88850, Aug 11 2011, 12:18:09) 
[GCC 4.6.1] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> fp = os.fdopen(2L)
>>> fp.fileno()
2

You'll get an OverflowError if the long can't be converted to an int.

>::: _dbus_bindings/conn.c
>@@ +232,4 @@
>>      self->has_mainloop = (mainloop != Py_None);
>>      self->conn = NULL;
>>      self->filters = PyList_New(0);
>> +    self->weaklist = NULL;
>
>Is this related to the porting, or an unrelated bug fix?

It's related to the porting, as (one of) the porting guides recommendations
for handling weaklists in Python 3.  Py_TPFLAGS_HAVE_WEAKREFS is gone.  It may
be overkill but it can't hurt.

>@@ +324,5 @@
>> +	!PyBytes_Check(address_or_conn)
>> +#endif
>> +	)
>> +    {
>> +	PyErr_SetString(PyExc_TypeError, "connection or str expected");
>
>I tend to prefer "is it one of these possibilities?" checks to be in the form:
>
>    if (first thing) {
>        ...
>    }
>    else if (second thing) {
>        ...
>    }
>    else {
>        /* none of the above, usually an error */
>    }
>
>Putting the "none of the above" case in the middle like you have here:
>
>    if (first thing) {
>        ...
>    }
>    else if (!(second thing)) {
>        /* none of the above, usually an error */
>    }
>    else {
>        /* second thing */
>    }
>
>guarantees that you'll have to rewrite the conditional if you add a third
>thing.

Fair enough.  I definitely want to stick to your preferred style, although it
does open us up for some code duplication; removing that duplication doesn't
seem worth complicating the implementation.

In this particular case, is there a reason not to accept either bytes or
unicodes in both versions of Python?   My rewrite based on your feedback does
enable that, but if you'd rather I can add some #ifdefs to limit it to "native
strings" (see below).

>::: _dbus_bindings/containers.c
>@@ +407,5 @@
>> +#ifdef PY3K
>> +	    !PyUnicode_Check(signature)
>> +#else
>> +	    !PyBytes_Check(signature)
>> +#endif
>
>I keep seeing this construct. I feel as though for as long as we remain
>compatible with Python 2, the dbus-bindings header ought to #define a
>STR_CHECK() or something, meaning "is it the thing that this version of Python
>calls str?"

Alternatively, we could accept both bytes and unicodes for these APIs (see
above).  But if you prefer to accept only "native strings", i.e. whatever that
version of Python calls `str`, then the macro you suggest makes sense.

Unlike above, in this case, I added the macro (NATIVE_STR_CHECK) and limited
the implementation, but it's probably better to be consistent.  I don't have a
strong feeling either way, so let me know what you'd prefer and I'll fix all
the cases.

>@@ +701,5 @@
>>  {
>>      PyObject *key, *value;
>>  
>> +#ifdef PY3K
>> +    if (PyUnicode_CompareWithASCIIString(name, "signature")) {
>
>Similarly, I'm starting to think we could do with a locally-defined
>Str_CompareWithASCIIString.

I think this case is a bit different, because you're not going to get bytes
attribute names in Python 3, so that version has a much shorter
implementation.  The existing Python 2 code allows 8-bit or unicode attribute
names, coerces the latter to 8-bit strings and then does a strcmp() for the
comparison.  So I think the utility of the macro depends on whether you want
to keep that behavior for Python 2 or not.  If so, then the macro doesn't buy
you much I think.  I.e. it's probably not worth putting all that coercion and
error checking into the macro, because the error-exit cleanups could
potentially be different.

>::: _dbus_bindings/generic.c
>@@ +38,4 @@
>>  {
>>      if (op == Py_EQ || op == Py_NE) {
>>          if (self == other) {
>> +            return PyLong_FromLong(op == Py_EQ);
>
>Just to confirm, is it OK for comparators to return long in Python 2?

I went to the Python source to double check, but after calling the
tp_richcompare slot function, Python turns this into a C int via
PyObject_IsTrue().  Other than checking against Py_NotImplemented, the rich
comparison's return value type isn't checked.

OTOH, unless I'm mistaken, dbus_py_tp_richcompare_by_pointer() and
dbus_py_tp_hash_by_pointer() aren't used anywhere internally.  Is it worth
even keeping these?  (or does my grep-fu suck? :)

>::: _dbus_bindings/int.c
>@@ +25,5 @@
>>  
>>  #include "types-internal.h"
>>  
>> +#ifdef PY3K
>> +#define INTBASE &DBusPyLongBase_Type
>
>Should have parentheses around the RHS, to avoid non-obvious precedence...

Good catch, thanks.

>
>@@ +71,5 @@
>>      }
>>      tuple = Py_BuildValue("(i)", PyObject_IsTrue(value) ? 1 : 0);
>>      if (!tuple) return NULL;
>> +#ifdef PY3K
>> +    self = (DBusPyLongBase_Type.tp_new)(cls, tuple, kwargs);
>
>... then you could use INTBASE->tp_new without the conditional :-)

Right! :)

>@@ +264,4 @@
>>  dbus_uint16_t
>>  dbus_py_uint16_range_check(PyObject *obj)
>>  {
>> +    long i = PyLong_AsLong(obj);
>
>I think dbus_py_uint16_range_check (and friends) are sometimes called on int
>objects in Python 2. With that in mind, is this usage OK?

Yep.  Again U(ing)TSL shows that PyLong_AsLong() drops back to PyInt_AsLong()
in Python 2.

>::: _dbus_bindings/message-append.c
>@@ +166,5 @@
>>      }
>>  }
>>  
>> +#ifdef PY3K
>> +#define FROMSTRING PyUnicode_FromString
>
>Call this STR_FROMSTRING and put it in the header?

Let's go with NATIVE_FROMSTR to match the previous macro.

>@@ +209,5 @@
>> +    if (PyLong_Check(obj)) {
>> +        if (DBusPyInt64_Check(obj))
>> +            return PyUnicode_FromString(DBUS_TYPE_INT64_AS_STRING);
>> +        else if (DBusPyUInt32_Check(obj))
>> +            return PyUnicode_FromString(DBUS_TYPE_UINT32_AS_STRING);
>
>This ordering is really weird. Any particular reason why they're not in some
>sort of systematic ordering, like "sort by size and then by signedness"?

Mostly because I copied the ordering from the Python 2 PyInt and PyLong cases
and then tried to combine them.  I noticed the comment just before this
stanza:

    /* Ordering is important: some of these are subclasses of each other. */

so I wanted to mess with that ordering as little as possible.  Maybe it does
make more sense to change the ordering under Python 3 as you suggest, since
there's only one that covers all int types.

What do you think?

>@@ +223,5 @@
>> +            return PyUnicode_FromString(DBUS_TYPE_UINT16_AS_STRING);
>> +        else if (DBusPyBoolean_Check(obj))
>> +            return PyUnicode_FromString(DBUS_TYPE_BOOLEAN_AS_STRING);
>> +        else
>> +            return PyUnicode_FromString(DBUS_TYPE_INT64_AS_STRING);
>
>As pointed out already, I think this will break existing code, and int32 is a
>better default. You could make it emit deprecation warnings if you like.

I think I'm happy just to make it return INT32.  By emitting deprecation
warnings, I think that's similar to your previous suggestion that the type
inferencing shouldn't be relied upon, and probably deprecated.  Hmm, it might
not be a bad idea, but I don't have strong opinions about it either way.

>@@ +503,2 @@
>>      DBG("Message_guess_signature: returning Signature at %p \"%s\"", ret,
>> +	ret ? PyUnicode_AS_DATA(ret) : "(NULL)");
>
>Is %s really right to printf the array of wide characters (codepoints or
>UTF-16 2-byte words) in a Unicode?
>
>(I wouldn't necessarily object to just getting rid of the DBG(), though.)

PyUnicode_AS_DATA() returns a char*, but it is the internal buffer, so while
probably safe, I guess it might not really be the right thing to return.  I
was lazy and didn't want to add all the conversion stuff for a DBG message.
Yeah, let's just remove the DBG.

>@@ +566,3 @@
>>          DBG("Performing actual append: string (from unicode) %s", s);
>>          if (!dbus_message_iter_append_basic(appender, sig_type, &s)) {
>> +	    Py_CLEAR(utf8);
>
>This is an unrelated leak fix, right?

Yep.

>@@ +593,5 @@
>>          }
>> +        y = *(unsigned char *)PyBytes_AS_STRING(obj);
>> +    }
>> +    else if (PyUnicode_Check(obj)) {
>> +	PyObject *obj_as_bytes = PyUnicode_AsUTF8String(obj);
>
>This logic (to make a Byte from a single-byte unicode string) seems really
>strange, and a bit inefficient.
>
>If you're using UTF-8 (but why is UTF-8 special here?), then it'd be way more
>efficient to check that the length of the Unicode is 1 and the first (and
>only) character is < 128. (Otherwise, it'd encode to more than one UTF-8
>byte.)
>
>But I think more sensible semantics would be to check that the length of the
>Unicode is 1, and use the numeric value of the first *codepoint* - so it's an
>error if it isn't in the latin-1 range, between U+0000 and U+00FF?
>
>(Optimization: even if Python is encoding its strings in UTF-16 like it does
>on Windows, it's enough to get the first-and-only Py_UNICODE - either a UCS-4
>32-bit codepoint, or a 16-bit unit of UTF-16 which is either itself or half
>of a surrogate pair - and check that it's in the range 0 to 255. If it is,
>the right answer is that byte; if not, error.)

XXX COME BACK TO ME

>::: _dbus_bindings/message-get-args.c
>@@ +252,4 @@
>>                                      args, kwargs);
>>              }
>>              else {
>> +#endif
>
>I'd prefer to still have a { in the Python 3 case so that the } doesn't need to
>be conditional.
>
>@@ +256,5 @@
>> +		/* Watch out for this call failing. */
>> +		unicode = PyUnicode_DecodeUTF8(u.s, strlen(u.s), NULL);
>> +		if (!unicode)
>> +		    break;
>> +                args = Py_BuildValue("(N)", unicode);
>
>Unrelated bugfix?

Yep.

>Is unicode leaked here, in your version?

It's not, because the 'N' code is just like 'O' except it steals the
reference.  (Technically, it doesn't incref the argument, so it steals it even
if the call fails for other reasons.)

>libdbus guarantees to return valid UTF-8, and its definition of valid UTF-8 is
>stricter than Python's (it excludes noncharacters like U+FEFF), so this should
>never fail in any case.

Cool.  This is mostly just my hyper-cautious Python C API hacker trait showing
through. :)

>::: _dbus_bindings/message.c
>@@ +115,5 @@
>>      return 0;
>>  }
>>  
>> +static PyObject *
>> +MethodCallMessage_tp_repr(PyObject *self)
>
>Not really related to this port...

No, but I found it *really* helpful for debugging it.

>::: _dbus_bindings/server.c
>@@ +101,4 @@
>>      {
>>          const char *list[length + 1];
>>  
>> +	if (!(references = PyTuple_New(length)))
>
>Is it OK to free a tuple some of whose entries are NULL?

Yep, it's a pretty typical Python idiom.

>@@ +402,4 @@
>>  
>>      self = DBusPyServer_NewConsumingDBusServer(cls, server, conn_class,
>>              mainloop, auth_mechanisms);
>> +    ((Server *)self)->weaklist = NULL;
>
>Unrelated fix, or what?

Yep, see the above discussion about the weakref differences in Python 3.

>::: configure.ac
>@@ +164,4 @@
>>     nested-externs \
>>     pointer-arith \
>>     format-security \
>> +   no-unused-parameter \
>
>This is in the wrong place; put "unused-parameter" in the list of undesirable
>warnings, next to "missing-field-initializers", without the "no-" prefix.
>
>(The difference is that for each thing in the list of undesirable warnings, if
>"-Wno-THING -Wno-error=THING" doesn't work, the macro will refuse to enable
>-Werror.)

Gotcha, thanks.  I should have been more careful about checking that macro.
I fixed the upstream Python bug that forces this on us, but it won't be
available until Python 3.2.3 (or Python 3.3).  Still, we probably want to keep
it even once that version's been released, so it'll still compile with 3.2,
3.2.1, and 3.2.2.

>::: dbus/__init__.py
>@@ +68,5 @@
>>             # submodules
>>             'service', 'mainloop', 'lowlevel'
>> +           ]
>> +
>> +is_py3 = getattr(sys.version_info, 'major', sys.version_info[0]) == 3
>
>Should be private, IMO... or if not, should be documented, and questions
>should be asked about why Python itself doesn't provide this.

Yep, I made it private.  Python doesn't provide this because it's mostly a
convenience to make dbus-python simpler.

>(Shouldn't it be >=, too?)

It could be, but hopefully we'll all be long retired before Python 4 shows
up. :)

>Having this in 'dbus' (as opposed to dbus._compat or something) works against
>the possibility of ever not having this module be full of circular
>dependencies... although we can probably never break the circular dependencies
>without breaking API anyway.

That's a good point.  I did think about the circular imports problem here,
which is why it's sometimes imported at function scope instead of module
scope.  I toyed with the idea of a _compat module for this and keywords() but
eventually didn't need it.

I'm happy to move those helpers to a dbus._compat if you prefer.

>@@ +73,5 @@
>> +
>> +if not is_py3:
>> +    __all__.append('UTF8String')
>> +
>> +def keywords(**kws):
>
>Should be private, IMO.

Yep, done as part of your previous suggestion.

>I think it should also raise an exception if you use utf8_strings (at least if
>it's set to a true value) in Python 3, rather than silently swallowing it -
>then part of porting to Python 3 will be "stop using utf8_strings, which no
>longer does anything useful".

The one reason why it silently swallows the argument is to aid in porting the
existing uses of utf8_strings in the Python layer of the library.  For
example, in bus.py, you'll find this in methods such as .list_names(),
.get_name_owner() and so on.  You'll find its use in other places in the
library, and in the test suite.

I wanted to simplify the cross-Python porting of these internal implementation
and test methods; I wasn't as much concerned about user code.  Raising an
exception will make it more difficult to have one code construct for both
versions.  I think making this a private function will mean that users should
never use it and thus not get much benefit out of an exception.

I'm fairly certain that at the C layer, nothing accepts a `utf8_strings`
argument in Python 3.  E.g. under Python 3, it's #ifdef'd out of the
Message_get_args_options struct and the acceptable keyword arguments in
dbus_py_Message_get_args_list().  It's not accepted as a positional argument
in the latter under Python 3 either.

It gets a little trickier at the Python layer.  For example, there's a slot in
SignalMatch for _utf8_strings, but it's essentially ignored.
add_signal_receiver() also accepts but ignores it, as does call_async() and
call_blocking() in connection.py.  There may be other cases.  The question is
whether we want to track these down and add exceptions or deprecation warnings
to nudge folks into getting rid of these (and in some cases, what to do about
their use within the dbus-python package itself).  Is it worth it to
complicate the Python code, or break backward compatibility?

I couldn't answer these questions, so I just punted and ignored it, but I'm
happy to do something different, based on your answers to the above.

>::: dbus/connection.py
>@@ +28,2 @@
>>  try:
>> +    import _thread as thread
>
>Is this really the recommended way to do things in Python-3-land? Aren't
>underscore prefixes normally meant to mean "no user-serviceable parts inside"?

Yes, but this is essentially the transformation that 2to3 would do, so I
mimicked that with my one-code-base approach.  See the Note at the top of
http://docs.python.org/library/thread.html for "official" recommendations
here.

I looked into this in more detail, and AFAICT, connection.py and service.py
really only wants the thread module for .allocate_lock(), so this one could
easily be changed to use threading.Lock (or RLock?) objects.

One thing I could not answer though is why _dbus.py imports thread.  Maybe we
can get rid of that and switch the other two to use threading?

>@@ +186,5 @@
>>              return False
>>          if self._int_args_match is not None:
>>              # extracting args with utf8_strings and byte_arrays is less work
>> +            args = message.get_args_list(
>> +                **keywords(utf8_strings=True, byte_arrays=True))
>
>It seems as though now it'd be better to just accept the overhead of UTF-8 ->
>Unicode conversion...

...

>@@ +192,4 @@
>>                  if (index >= len(args)
>> +                    or (not isinstance(args[index], UTF8String)
>> +                        if not is_py3
>> +                        else False)
>
>... which would let you use String as the thing that you check with
>isinstance, which can then be unconditional.
>
>Removing the UTF8String check is a semantic change: previously we insisted
>that arguments matched with args_match were (in D-Bus terms) strings, whereas
>now you'll accept anything string-like. The underlying D-Bus match rules only
>accept strings (and not, for instance, object paths), so we should be
>consistent with that.

Right, so the big question for you is whether that semantic and API change is
acceptable.  Getting rid of UTF8String and utf8_strings in both versions of
Python certainly would clean things up, at the cost of breaking existing code
(including dbus-python internally, which would be fixable).  I opted for
maximal compatibility with existing Python 2 code, but I'd certainly prefer
things to be cleaner. :)  I'll let you decide and will abide by your
preference.

>@@ +210,5 @@
>>              # doing so again
>>              if args is None or not self._utf8_strings or not self._byte_arrays:
>> +                args = message.get_args_list(
>> +                    **keywords(utf8_strings=self._utf8_strings,
>> +                               byte_arrays=self._byte_arrays))
>
>Be careful to change this too, when changing the above.
>
>::: dbus/dbus_bindings.py
>@@ +23,1 @@
>>  from dbus.types import *
>
>dbus_bindings is so deprecated that it should never have existed; it only
>exists to support apps that were wrongly using the C-level part of the binding
>(what's now _dbus_bindings). Can we take the opportunity to just not install
>it, or have it raise an exception on import, for Python 3?

It does emit a DeprecationWarning currently for both versions, so raising an
exception (ImportError most likely) in Python 3 is an easy change.  Perhaps it
should just be removed altogether at this point?  That's a change I'll leave
to you to decide. :)

I've made all the uncontroversial changes discussed above, and I'm just going
to finish up the testing.  I really appreciate the review, given how big the
changes are.  Let me know whether you'd prefer a full diff or incremental, and
let me know what you think about the open questions above, and I can upload
new diffs by tomorrow.

Cheers,
-Barry