-
Notifications
You must be signed in to change notification settings - Fork 242
Server: Correct handling of server list entry zero #2812
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
pljones
merged 1 commit into
jamulussoftware:master
from
pljones:patch/server-0-is-directory
Aug 29, 2022
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,10 +181,14 @@ CServerListManager::CServerListManager ( const quint16 iNPortNum, | |
| iServInfoNumSplitItems = slServInfoSeparateParams.count(); | ||
| } | ||
|
|
||
| // Init server list entry (server info for this server) with defaults. Per | ||
| // definition the client substitutes the IP address of the directory server | ||
| // itself for his server list. If we are a directory server, we assume that | ||
| // we are a permanent server. | ||
| /* | ||
| * Init server list entry (server info for this server) with defaults. | ||
| * | ||
| * The client will use the built in or custom address when retrieving the server list from a directory. | ||
| * The values supplied here only apply when using the server as a server, not as a directory. | ||
| * | ||
| * If we are a directory, we assume that we are a permanent server. | ||
| */ | ||
| CServerListEntry ThisServerListEntry ( haServerAddr, ServerPublicIP, "", QLocale::system().country(), "", iNumChannels, bIsDirectoryServer ); | ||
|
|
||
| // parse the server info string according to definition: | ||
|
|
@@ -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. | ||
|
|
||
| */ | ||
| void CServerListManager::RetrieveAll ( const CHostAddress& InetAddr ) | ||
| { | ||
|
|
@@ -646,27 +654,31 @@ void CServerListManager::RetrieveAll ( const CHostAddress& InetAddr ) | |
| // allocate memory for the entire list | ||
| CVector<CServerInfo> vecServerInfo ( iCurServerListSize ); | ||
|
|
||
| // copy the list (we have to copy it since the message requires | ||
| // a vector but the list is actually stored in a QList object and | ||
| // not in a vector object | ||
| for ( int iIdx = 0; iIdx < iCurServerListSize; iIdx++ ) | ||
| // copy list item for the directory and just let the protocol sort out the actual details | ||
| vecServerInfo[0] = ServerList[0]; | ||
| vecServerInfo[0].HostAddr = CHostAddress(); | ||
|
|
||
| // copy the list (we have to copy it since the message requires a vector but the list is actually stored in a QList object | ||
| // and not in a vector object) | ||
| for ( int iIdx = 1; iIdx < iCurServerListSize; iIdx++ ) | ||
| { | ||
| // copy list item | ||
| CServerInfo& siCurListEntry = vecServerInfo[iIdx] = ServerList[iIdx]; | ||
|
|
||
| bool serverIsInternal = NetworkUtil::IsPrivateNetworkIP ( siCurListEntry.HostAddr.InetAddr ); | ||
|
|
||
| bool wantHostAddr = clientIsInternal /* HostAddr is local IP if local server else external IP, so do not replace */ || | ||
| ( !serverIsInternal && iIdx != 0 && | ||
| ( !serverIsInternal && | ||
| InetAddr.InetAddr != siCurListEntry.HostAddr.InetAddr /* external server and client have different public IPs */ ); | ||
|
|
||
| if ( !wantHostAddr ) | ||
| { | ||
| vecServerInfo[iIdx].HostAddr = siCurListEntry.LHostAddr; | ||
| } | ||
|
|
||
| // do not send a "ping" to the directory itself or a server local to the directory | ||
| if ( iIdx != 0 && !serverIsInternal ) | ||
| // do not send a "ping" either to a server local to the directory (no need) | ||
| // or for a client local to the directory (the address will be wrong) | ||
| if ( !serverIsInternal && !clientIsInternal ) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for clarity, following from #2811. |
||
| { | ||
| // create "send empty message" for all other registered servers | ||
| // this causes the server (vecServerInfo[iIdx].HostAddr) | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't say the client shouldn't do it. Actually, I think it should be in
src/protocol.cppwhen 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.