Server: Correct handling of server list entry zero #2812
Conversation
pljones
force-pushed
the
patch/server-0-is-directory
branch
from
8da36da to
ba73d7c
Compare
hoffie
changed the title
|
I think the commit message should be similar to the PR title as the current one needs more background knowledge and doesn't add that much IMHO. |
It was a workaround for an issue caused elsewhere before. I took it away in 3.9.0. This restores the workaround. I'd be perfectly happy not to apply this change... but now I'm also unhappy about what |
pljones
force-pushed
the
patch/server-0-is-directory
branch
from
ba73d7c to
b6d5834
Compare
| @@ -631,6 +635,10 @@ void CServerListManager::Remove ( const CHostAddress& InetAddr ) | |||
| - SERVER external to DIRECTORY (list entry external IP same LAN) | |||
| - CLIENT internal to SERVER (CLM same IP as list entry external IP): use "internal" fields (local LAN address) | |||
| - CLIENT external to SERVER (CLM not same IP as list entry external IP): use "external" fields (public IP address from protocol) | |||
|
|
|||
| Finally, when retrieving the entry for the directory itself life is more complicated still. It's easiest just to return "0" | |||
| and allow the client connect dialogue instead to use the IP and Port from which the list was received. | |||
There was a problem hiding this comment.
I haven't tried it, but I think the connect dialog should work independent of this PR as it works based on index 0 without looking at the address at all.
It's other protocol consumers (such as Jamulus Explorer) which break without this change.
There was a problem hiding this comment.
That means it's open to question where the fix should be made.
If Jamulus Explorer gets the address/port right with this fix, I'd rather leave the change in place and remove the work around in the client connect dialogue. The above is meant as a reminder that there's code there needing investigation... Maybe it says the wrong thing, though.
There was a problem hiding this comment.
I think this PR is correct and should be merged because 3.9.0 unintentionally changed protocol details. We don't have a formal specification of the protocol, so the Jamulus implementation effectively dictates the details.
Removing the logic in the client is not a trivial thing, IMO. If we remove it, all servers would have to generate a real, non-empty index 0, which is close to impossible... :)
Moving the logic from the GUI to the client makes sense, of course, and should be transparent to do.
There was a problem hiding this comment.
Removing the logic in the client is not a trivial thing, IMO.
I didn't say the client shouldn't do it. Actually, I think it should be in src/protocol.cpp when receiving the list. That way any use would get the right value. However, it definitely shouldn't be in the UI. That layer should NOT be required for correct behaviour. The RPC client, for example, should be able to retrieve the server list and connect to any entry.
pljones
force-pushed
the
patch/server-0-is-directory
branch
2 times, most recently
from
89c525b to
3a632c4
Compare
| if ( iIdx != 0 && !serverIsInternal ) | ||
| // do not send a "ping" either to a server local to the directory (no need) | ||
| // or to a client local to the directory (the address will be wrong) | ||
| if ( !serverIsInternal && !clientIsInternal ) |
There was a problem hiding this comment.
Just for clarity, following from #2811.
pljones
force-pushed
the
patch/server-0-is-directory
branch
from
3a632c4 to
f252d90
Compare
Short description of changes
@corrados reported an issue with a change made for Jamulus 3.9.0 which changed the behaviour for this entry.
Previous, omitting
--serverpublicipfor a directory had resulted in the server list entry for the directory being determined correctly.With the changes in 3.9.0, the wrong address could be determined, preventing use of the directory as a normal server.
For example, running jamulusdirectory8.drealm.info:22824 as follows (more or less):
/opt/Jamulus/bin/Jamulus-Directory8 -n -s -p 22824 -u 1 -l /opt/Jamulus/log/Directory8.log -e localhost -f [3.8.0] --directoryfile /opt/Jamulus/run/Directory8.persistence -o Directory on 22824;London;224 -w <h3>Directory on 22824</h3><p>Server list hosted by drealm.info.</p>would result in
where
192.168.1.19is the server address, not the public IP address, and is hence not reachable by Jamulus Explorer as a client.3.9.0 made quite a bit of change to the server list handling and I'm hoping this small adjustment is all that's needed to restore the previous behaviour. Server list entry zero is "self". When a directory gets asked to return the server list, that means its own entry always comes first. The change here returns a "default" (protocol determined) value for the host address that the client will use, regardless of whether it's LAN or internet.
CHANGELOG: Correct default "self" address for directory, enabling use as a server
Context:
Raised in email.
Does this change need documentation?
No.
Status of this Pull Request
jamulusdirectory8.drealm.info:22824 is running this patched version of the software. private.drealm.info:56850 is registered with the directory (to prove registration still works). Using Jamulus Explorer with the directory added, the display is correct.
Jamulus Explorer can see the welcome message and a LAN client can also connect to the directory as a server.
What is missing until this pull request can be merged?
Review
Checklist