Skip to content
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

Improve interaction between use of inifile and command line parameters (edited) #120

Closed
pljones opened this issue Apr 20, 2020 · 15 comments
Closed
Labels
feature request Feature request

Comments

@pljones
Copy link
Collaborator

pljones commented Apr 20, 2020

Brought over from #91, the suggestion is to improve the Server GUI

(a) by adding a warning if running with GUI and passing parameters
(b) by allowing the local port to be specified

Given it's more the server than the client that needs the port, that should be in the server config, too, from the GUI.

@corrados corrados added the feature request Feature request label Apr 21, 2020
@gilgongo
Copy link
Member

Just reading #91 and am a bit unclear - what's the use case for changing the local port exactly?

@pljones
Copy link
Collaborator Author

pljones commented Apr 25, 2020

You can only have one listen on a port. So if you want two Jamulus instances listening, you need to be able to control their port. That's why it's possible to do it using -p on the command line. However, it's not possible to do it in the GUI, which means you can't just run Jamulus -s to set everything up, you may still need to set command line options.

@pljones
Copy link
Collaborator Author

pljones commented May 10, 2020

OK, I'll be covering this off under #170 (which really ought to be renamed... or I should have used this ticket... or something). Anyway, I'll close this.

@pljones
Copy link
Collaborator Author

pljones commented May 11, 2020

image

@pljones pljones reopened this May 11, 2020
@pljones
Copy link
Collaborator Author

pljones commented May 11, 2020

OK, I'm going to leave #170 doing what was asked for there and accumulate other stuff here to try to keep increments smaller...

@pljones
Copy link
Collaborator Author

pljones commented May 11, 2020

OK, so moving also from #170, from *nix:

  • SIGUSR1 - re-read ini file
  • SIGUSR2 - write ini file

@corrados corrados changed the title FR: Server GUI improvements Server GUI improvements May 18, 2020
@pljones
Copy link
Collaborator Author

pljones commented Jun 3, 2020

I keep looking at

(a) by adding a warning if running with GUI and passing parameters
(b) by allowing the local port to be specified

and going "... mm, needed, ... mm, other things more needed ..."

So, given we have #294 sitting there now begging to take #120 (comment) away from here, that does leave this issue clear for the above two.

Unless I wrote somewhere else something else would be happening, of course.


  • Server - warning if run with an inifile and command line parameters that stored in the inifile
  • Server GUI - add local port
  • Server INIFILE - add local port

  • Client - warning if run with an inifile and command line parameters that stored in the inifile
  • Client GUI - add local port
  • Client INIFILE - add local port

Then there's probably #141 to take a look at and decide whether to close / implement both for client and server.

@pljones pljones changed the title Server GUI improvements Improve interaction between use of inifile and command line parameters (edited) Jul 6, 2020
@corrados
Copy link
Contributor

corrados commented Jul 7, 2020

Let's discuss the two points in your initial post:

1.) Given it's more the server than the client that needs the port, that should be in the server config, too, from the GUI.

The port number is critical since the server locks the network port on startup. So if the port is set to 22124 is already used and you try to start the GUI, you will get "cannot bind the socket". That is the reason why we can only change the port via the command line argument.
There is another reason: If you put the setting in the GUI and store it in the ini-file, it does not help. If you change the port and start the server twice, the second instance will use the same ini-file and therefore the same port.
In my opinion the port setting change is only useful when used as a command line argument (as it is implemented right now). Do you agree?

2.) (a) by adding a warning if running with GUI and passing parameters

I would not implement a warning. The question here is: What has higher priority? The command line argument or the same setting stored in the ini-file? I would define: The command line argument has highest priority.
As an example: If we would store the server welcome message in the ini-file and the user would then start the server with the -w command line argument -> I would overwrite the message from the ini-file by the one given by the command line argument. This command line argument welcome message would then also be shown in the GUI edit box. The ini-file text would be vanished. What is your opinion on that?

@pljones
Copy link
Collaborator Author

pljones commented Jul 8, 2020

So if the port is set to 22124 is already used and you try to start the GUI, you will get "cannot bind the socket". That is the reason why we can only change the port via the command line argument.

But if the GUI allowed it to be set, it could display the error, allow you to enter a new port and start listening, storing the setting in its inifile. The aim is a single command line parameter per server: the inifile.

The question here is: What has higher priority? The command line argument or the same setting stored in the ini-file? I would define: The command line argument has highest priority.

Yes, I've thought about this. The only concern I have is that it must be clear that any command line parameter must not write back to the inifile unless that's explicitly requested from the GUI.

That way, you allow a "starter" inifile, plus some specifics to be set, and the starter file can be shared between servers.

@corrados
Copy link
Contributor

corrados commented Jul 8, 2020

But if the GUI allowed it to be set, it could display the error, allow you to enter a new port and start listening, storing the setting in its inifile.

As an alternative, the server could automatically increment the port if it is blocked by another server. The client currently does this. So e.g. if 22124 is blocked, it automatically uses 22125. If you only use the server for registering in a public list, the port does not matter. The port only matters if you have a private server. For that you have to open the ports etc. And people who spend time on these router settings usually are able to start the Jamulus server with a command line option (because they only need that if they want to run more than one server at one PC which is quite special and only a few will do that).

The aim is a single command line parameter per server: the inifile.

Is this really the aim? I personally prefert to use command line options. E.g. in my server start script. I immediately see what's configured in the server by looking at the script. If the script just uses an ini file, I would have to look in that file. But usually you have all the parameters there and it is not as straightforward to find the essential ones. Bottomline: For me this would not be the aim actually.

The only concern I have is that it must be clear that any command line parameter must not write back to the inifile unless that's explicitly requested from the GUI.

That was my question. So you would treat both, the command line arguments and the settings of the ini-file, completely separat? Would that be your expectation? If we, e.g., have the possibility to set the welcome message in the GUI and you set a message via the command line argument -w, I would expect that this text given at the command line is then shown in the GUI as well. I would be very surprised if the text in the GUI is different but the one from the command line is used. That would be very confusing to the Jamulus users.

@pljones
Copy link
Collaborator Author

pljones commented Jul 8, 2020

As an alternative, the server could automatically increment the port if it is blocked by another server. The client currently does this. So e.g. if 22124 is blocked, it automatically uses 22125. If you only use the server for registering in a public list, the port does not matter.

Yes, the port matters. For firewall settings, it's essential. Admittedly, you should know what you're doing but if you have to need guides saying "open all ports from 22124 upwards for however many servers you think you might have", which isn't necessarily a good recommendation. Even if you have a public server, incoming packets should get blocked by any server firewall unless a rule is in place to allow them.

Is this really the aim?

That was my goal, I'm saying. Whilst personally I prefer command line options, lots of people use the GUI and using an inifile as a "starter" allows someone to try helping out by saying "Try this and then run with these arguments to get what you're after". It adds flexibility.

The question here is: What has higher priority?

The inifile gets read. Then the command line options override. So the command line takes priority.

However, the command line options must not get written to the inifile unless that write is explicitly requested by the server admin.

As above, once the person being helped has got the inifile and command line working the way they want, they can either keep running it that way or say "OK, that's good, save it to the inifile" and only have to run like that. And of course, as much as possible wouldn't need command line arguments as it could be set through the GUI until "that's good" was reached.

(Nice to have: an option to write out the non-UI parts of the inifile as command line arguments...)

@corrados
Copy link
Contributor

corrados commented Jul 9, 2020

Yes, the port matters. For firewall settings, it's essential.

No, it is not. On Windows 10 you simply start the Jamulus server and you can register. You do not have to change anything in the firewall other than the usual dialog showing up the first time you start an application with network access on Windows.
In my opinion the port parameter should still be a command line only parameter.

lots of people use the GUI and using an inifile as a "starter" allows someone to try helping out by saying "Try this and then run with these arguments to get what you're after"

My experience is that we have two types of Jamulus server operators. The one group uses the headleass server on a linux system and they are working with command line options. The other group are the typical Windows user who want to have a GUI. You can help the second group best by posting GUI screen shots.

As above, once the person being helped has got the inifile and command line working the way they want, they can either keep running it that way or say "OK, that's good, save it to the inifile" and only have to run like that. And of course, as much as possible wouldn't need command line arguments as it could be set through the GUI until "that's good" was reached.

We are now talking about the server operators which want to use the GUI. For these we usually do not need any command line arguments. And I do not talk about, e.g., the welcome message since this parameter belongs to the GUI but has not yet been implemented. I do not think that this would be the usual case to help a server operator which want so use a GUI to send them an ini-file which he has to load with the --inifile command line argument. They will not like to use any command line argument.

@corrados
Copy link
Contributor

they can either keep running it that way or say "OK, that's good, save it to the inifile"

I don't think we need that. Simple rule: Command line arguments have higher priority than settings file. I have just implemented that, see, e.g.: https://github.com/corrados/jamulus/blob/master/src/settings.cpp#L825

@corrados
Copy link
Contributor

@pljones With your pull request #513 I think we get what I have specified in my above post: "Simple rule: Command line arguments have higher priority than settings file". So I think we can close this Issue now, do you agree?
BTW, I will review your pull request after the 3.5.10 release on Sunday.

@pljones
Copy link
Collaborator Author

pljones commented Aug 14, 2020

Yes - it's now easy to do consistently.

At the moment, command line overrides on start up but then gets written - it might be nice not to ... but I'm not sure.

@pljones pljones closed this as completed Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Feature request
Projects
None yet
Development

No branches or pull requests

3 participants