Bug 72301

Created attachment 90212 [details] [review]
[PATCH 1/3] DBus Spec: fix format a little

OK, the format of spec file is rather crazy, just fix a little part of
indent before add document for "bind".

Created attachment 90213 [details] [review]
[PATCH 2/3] DBus Spec: add document of bind for tcp/nonce-tcp  transport

tcp/nonce-tcp transport has a "bind" key, which can be specified a
hostname and will override hostname specified in "host" key.

Created attachment 90214 [details] [review]
[PATCH 3/3] DBus-daemon(1): clarify hostname of bind to avoid  confusing

At previous, I confused by below string in dbus-daemon(1)

    The specified host should be a valid name of the local machine or
    weird stuff will happen.

"The specified host" may also understand as "the hostname specified by
'host' key". Maybe this is a problem for me (English is my second
language), so to avoid the others got confused, let's clarify it as "The
specified hostname of bind".

Comment on attachment 90212 [details] [review]
[PATCH 1/3] DBus Spec: fix format a little

Review of attachment 90212 [details] [review]:
-----------------------------------------------------------------

If you're changing the whitespace at all, I'd prefer 2 spaces per indent, which is as close as the D-Bus spec gets to consistency.

Comment on attachment 90213 [details] [review]
[PATCH 2/3] DBus Spec: add document of bind for tcp/nonce-tcp  transport

Review of attachment 90213 [details] [review]:
-----------------------------------------------------------------

I think a better way to express this might be to merge my patch defining listenable/connectable addresses, and say something like this:

bind (string)
The IP address of one of the local machine's interfaces (most commonly <literal>127.0.0.1</literal>), or a DNS name that resolves to one of those IP addresses, or <literal>0.0.0.0</literal> to listen on all interfaces simultaneously. If not specified, the default is the same value as "host".

I assume the purpose here is that if you're doing remote TCP on a machine with addresses 10.0.0.1 and 10.11.12.13, you can tell the dbus-daemon to listen on

tcp:host=10.0.0.1,bind=0.0.0.0

and it will report a "recommended" connectable address like

tcp:host=10.0.0.1,port=51423

but clients that try to connect to tcp:host=10.11.12.13,port=51423 would also connect successfully?

::: doc/dbus-specification.xml
@@ +3128,5 @@
> +           <entry>(string)</entry>
> +           <entry>hostname to bind to, which will override the "host" option
> +            specifying what address to bind to, without changing the address
> +            reported by the bus. The bind option can also take a special name
> +            '*' to cause the bus to listen on all local address (INADDR_ANY).

Is the special treatment of "*" something that is hard-coded in libdbus, or is it a quirk of the OS resolver?

If it's hard-coded in libdbus, fine.

If it's an OS resolver feature ("getent hosts '*'" happens to return "0.0.0.0" on that platform), we shouldn't document or recommend it.

Comment on attachment 90214 [details] [review]
[PATCH 3/3] DBus-daemon(1): clarify hostname of bind to avoid  confusing

Review of attachment 90214 [details] [review]:
-----------------------------------------------------------------

::: doc/dbus-daemon.1.xml.in
@@ +396,5 @@
>  the host option specifying what address to bind to, without changing
>  the address reported by the bus. The bind option can also take a
>  special name '*' to cause the bus to listen on all local address
> +(INADDR_ANY). The specified hostname of bind should be a valid
> +name of the local machine or it will fail.</para>

As in my review of the previous patch, I think the emphasis should be on IP addresses: the value for "bind" is either 0.0.0.0 to listen on all addresses, or one of the machine's IP addresses (typically 127.0.0.1 to listen on the loopback interface), or a hostname (or maybe use "DNS name" if you prefer to avoid the word "host" here) that will resolve to one of the machine's addresses.

(In reply to comment #4)
> Comment on attachment 90212 [details] [review] [review]
> [PATCH 1/3] DBus Spec: fix format a little
> 
> Review of attachment 90212 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> If you're changing the whitespace at all, I'd prefer 2 spaces per indent,
> which is as close as the D-Bus spec gets to consistency.

I was trying to find a tool to format dbus-specification.xml, however, xmllint and tidy doesn't only just do indent, which also change the other part I do not expected.

So I'd like to left the indent problem but replace '\t' with 8 spaces and another issue I found with tidy is four double quotes (") isn't in english format, here is (“”)

(In reply to comment #5)
> Comment on attachment 90213 [details] [review] [review]
> [PATCH 2/3] DBus Spec: add document of bind for tcp/nonce-tcp  transport
> 
> Review of attachment 90213 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> I think a better way to express this might be to merge my patch defining
> listenable/connectable addresses, and say something like this:

Sure, I remember I did r+ on that patch?

> 
> bind (string)
> The IP address of one of the local machine's interfaces (most commonly
> <literal>127.0.0.1</literal>), or a DNS name that resolves to one of those
> IP addresses, or <literal>0.0.0.0</literal> to listen on all interfaces
> simultaneously. If not specified, the default is the same value as "host".
> 
> I assume the purpose here is that if you're doing remote TCP on a machine
> with addresses 10.0.0.1 and 10.11.12.13, you can tell the dbus-daemon to
> listen on
> 
> tcp:host=10.0.0.1,bind=0.0.0.0
> 
> and it will report a "recommended" connectable address like
> 
> tcp:host=10.0.0.1,port=51423
> 
> but clients that try to connect to tcp:host=10.11.12.13,port=51423 would
> also connect successfully?

Didn't test in real environment, but I think this is correct, giving that it's the same as a system with a loopback address and a local NIC setup.

> 
> ::: doc/dbus-specification.xml
> @@ +3128,5 @@
> > +           <entry>(string)</entry>
> > +           <entry>hostname to bind to, which will override the "host" option
> > +            specifying what address to bind to, without changing the address
> > +            reported by the bus. The bind option can also take a special name
> > +            '*' to cause the bus to listen on all local address (INADDR_ANY).
> 
> Is the special treatment of "*" something that is hard-coded in libdbus, or
> is it a quirk of the OS resolver?

For key "bind", it's hard-coded in libdbus, so I think we should document it in DBus Spec, in a way prefer to 0.0.0.0

> 
> If it's hard-coded in libdbus, fine.
> 
> If it's an OS resolver feature ("getent hosts '*'" happens to return
> "0.0.0.0" on that platform), we shouldn't document or recommend it.

Created attachment 90331 [details] [review]
[PATCH v2 1/3] DBus Spec: replace tab with 8 spaces

Created attachment 90332 [details] [review]
[PATCH 2/4] DBus Spec: fix double quotes

Created attachment 90333 [details] [review]
[PATCH 3/4] DBus Spec: add document of bind for tcp/nonce-tcp  transport

Created attachment 90335 [details] [review]
[PATCH 4/4] dbus-daemon(1): align document about "bind" with DBus  Spec

Comment on attachment 90332 [details] [review]
[PATCH 2/4] DBus Spec: fix double quotes

Review of attachment 90332 [details] [review]:
-----------------------------------------------------------------

r-, no need for this. U+201C LEFT DOUBLE QUOTATION MARK and U+201D RIGHT DOUBLE QUOTATION MARK are correct English (formally, more correct than U+0022 QUOTATION MARK), and are fine to use for anything that isn't special syntax (e.g. U+0022 is the only non-misleading quote mark for C/C++ string syntax).

They're also annoying to type (Shift+AltGr (Compose), Shift+2 ("), Shift+Comma (<) on my UK X11 keyboard layout), so people who aren't pedantic about Unicode don't usually bother :-)

Comment on attachment 90331 [details] [review]
[PATCH v2 1/3] DBus Spec: replace tab with 8 spaces

Review of attachment 90331 [details] [review]:
-----------------------------------------------------------------

This is OK to a point, although it does break "git blame" and similar tools.

Comment on attachment 90333 [details] [review]
[PATCH 3/4] DBus Spec: add document of bind for tcp/nonce-tcp  transport

Review of attachment 90333 [details] [review]:
-----------------------------------------------------------------

::: doc/dbus-specification.xml
@@ +3125,5 @@
>            </row>
>            <row>
> +           <entry>bind</entry>
> +           <entry>(string)</entry>
> +           <entry>The IP address of one of the local machine's interfaces

You don't actually say what it *does* here. :-)

Perhaps something like this?

Used in a listenable address to configure the interface
on which the server will listen: either the IP address
of [... the rest of what you said ...]

@@ +3205,5 @@
>            </row>
>            <row>
> +           <entry>bind</entry>
> +           <entry>(string)</entry>
> +           <entry>The IP address of one of the local machine's interfaces

How about just saying "the same as for tcp: addresses" for nonce-tcp: addresses?

(There's not enough context in the diff to tell which is which - I'm assuming tcp: comes first.)

Comment on attachment 90335 [details] [review]
[PATCH 4/4] dbus-daemon(1): align document about "bind" with DBus  Spec

Review of attachment 90335 [details] [review]:
-----------------------------------------------------------------

::: doc/dbus-daemon.1.xml.in
@@ +392,4 @@
>  <para>Example: &lt;listen&gt;tcp:host=localhost,port=0&lt;/listen&gt;</para>
>  
>  
> +<para>tcp/nonce-tcp addresses also allow a bind=hostname option,

Again, you're saying this is allowed but haven't actually said what it does.

Created attachment 90546 [details] [review]
[PATCH v2 2/3] DBus Spec: add document of bind for tcp/nonce-tcp  transport

Created attachment 90547 [details] [review]
[PATCH v2 3/3] dbus-daemon(1): align document about "bind" with DBus  Spec

Fixed in git for 1.7.10, thanks