Server: Don't warn when no --serverinfo is provided

[hoffie]

Short description of changes
The intention of 5b2c2de was to introduce logging for bad --serverinfo parameters. However, it also warns about bad serverinfo if no --serverinfo is given at all. This is confusing.
Therefore, this commit removes the error when no --serverinfo has been given.

CHANGELOG: Server: Improved --serverinfo argument validation

Context: Fixes an issue?
Fixes: #2947

Does this change need documentation? What needs to be documented and how?
No.

Status of this Pull Request
Ready.

What is missing until this pull request can be merged?
Reviews.

Checklist

• I've verified that this Pull Request follows the general code principles
• I tested my code and it does what I want
• My code follows the style guide
• I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
• I've filled all the content above

[pljones]

When was there a change to make --serverinfo "" valid?

[pljones]

Can we at least change the PR title to match what the problem is (i.e. error on missing --serverinfo param) rather than on empty param argument.

What I'm saying above is that I don't think this check should be needed in CServer - the check for empty argument should be done in main.cpp and start up shouldn't happen.

[hoffie]

When was there a change to make --serverinfo "" valid?

I think there was no change.

Can we at least change the PR title to match what the problem is (i.e. error on missing --serverinfo param) rather than on empty param argument.

I agree that the title was confusing and I've changed the title and the commit message accordingly

What I'm saying above is that I don't think this check should be needed in CServer - the check for empty argument should be done in main.cpp and start up shouldn't happen.

Yeah, right, but that's a different, rather hypothetical issue, I'd say?

[ann0see]

@pljones Do you think this is ready?

[pljones]

Reviewed. I still don't think the PR is valid.

[ann0see]

Can we at least change the PR title to match what the problem is (i.e. error on missing --serverinfo param) rather than on empty param argument.

What I'm saying above is that I don't think this check should be needed in CServer - the check for empty argument should be done in main.cpp and start up shouldn't happen.

Ok. Due to this comment?

[pljones]

Ok. Due to this comment?

Yes.

[pljones]

We need to agree what happens next here. I'm proposing to drop from 3.10.0 until that's done.

[ann0see]

I think it's a regression which should be fixed.

At least the message should say that serverinfo is be empty or invalid as intermediate fix.

[pljones]

I think it's a regression which should be fixed.

At least the message should say that serverinfo is be empty or invalid as intermediate fix.

Yes, in main.cpp. It shouldn't be possible to pass syntactically invalid values through to server.cpp and serverlist.cpp, so no changes should be needed there.

[ann0see]

@pljones actually, I think, the else is related to line 197

if ( iServInfoNumSplitItems >= 3 )

[ann0see]

Ah yes, and empty serverinfo is valid for unregistered servers. And I'm quite sure that this isn't checked. Do you think that registered servers must provide a server info?

[pljones]

It's valid not to supply --serverinfo on the command line whether registering or not - serverlist.cpp:

    // prepare the server info information
    QStringList slServInfoSeparateParams;
    int         iServInfoNumSplitItems = 0;

    if ( !strServerInfo.isEmpty() )
    {
        // split the different parameter strings
        slServInfoSeparateParams = strServerInfo.split ( ";" );

        // get the number of items in the split list
        iServInfoNumSplitItems = slServInfoSeparateParams.count();
    }
...
    // parse the server info string according to definition:
    // [this server name];[this server city];[this server country as QLocale ID] (; ... ignored)
    // per definition, we expect at least three parameters
    if ( iServInfoNumSplitItems >= 3 )
    {
...
    }
    else
    {
        qWarning() << "Ignoring invalid serverinfo, please verify the parameter syntax.";
    }

There's nothing invalid about it. It simply isn't used if fewer than three entries are present and any extra ones (like the comment says) are ignored. The per definition could be a little less absolute, perhaps - maybe inside the if. The else clause should never have been added.

Once upon a time, a long long time ago, before the pandemic really got going, --serverinfo was used to pass the list of servers that the central server knew about (rather than have the few hard coded ones that preceeded that version).

Ideally, now, we'd pass a parsed "ServerInfo" object (actually a CServerListEntry) to the server on start up, to avoid the parsing deeper inside.

However, for now, I'd be happy just to see the parameter actually validated in main.cpp.

(It's probably worth checking the GUI doesn't allow ; in the field so that invalid data can't be entered.)

[ann0see]

Ideally, now, we'd pass a parsed "ServerInfo" object (actually a CServerListEntry) to the server on start up, to avoid the parsing deeper inside.

However, for now, I'd be happy just to see the parameter actually validated in main.cpp.

I think after reading and thinking about that, that we should invest in improving that and not go with a halfway working version.

So, the next step would be to find all places where serverinfo is set (GUI, JSONRPC, CLI) and then generate and pass the parsed object as you suggested. It should be doable. However, main.cpp doesn't create these kind of objects so far? Ultimately it's for sure a design choice.

[pljones]

The problem with main.cpp currently is around settings handling. We parse the command line fully before considering settings. That means the content of settings aren't taken into account (and we have dirty hacks in place to take the command line into account when processing settings).

Currently, the "keep it small and simple" approach would just be to validate the parameter when we're passed --serverinfo on the command line. The change that added the warning in serverlist.cpp when no --serverinfo is passed could also be reverted.

But I don't think we should just revert that serverlist.cpp change alone.

[ann0see]

@pljones could you please have a look at this PR then: #2599 it changes a lot in this area, I suppose?

[pljones]

Commented on #2599.

[ann0see]

@pljones I know you think this is wrong to merge, but can you update this PR to have it in an acceptable state? I believe this is a regression and should be fixed in the next release.

[pljones]

As a minimum, I think that if --serverinfo is provided on the command line then the required parameter should be checked to ensure it has two separator characters in (at least). No further check at that stage is required. At least this way, if it's provided it should be syntactically correct. Then the warning can be removed as well, as the validation will have been done in main.cpp, as it should be.

[ann0see]

Yes. Sounds good:

• Get rid of the else warning
• Verify syntax in main.cpp ?

[pljones]

I've taken the liberty of pushing to this branch to avoid raising a new PR.

This solution isn't perfect. It could check each string isn't empty...

$ ./Jamulus -s --serverinfo ';;' -p 60000
...
Using server info: name = "", city = "", country/region = "" (United Kingdom)
...

[pljones]

@ann0see are you happy with this approach?

[ann0see]

Not quite. If it is empty, I think it should be skipped without any output.

[ann0see]

See comment

[ann0see]

I'm happy now. Thanks