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
Feature 7957, banlist: load/save ban lists from/to external files #8909
Conversation
@nielsmh Should I be requesting that you re-review this or wait for someone else? I do not know if you have "write access". |
@@ -117,6 +117,8 @@ Last updated: 2011-02-16 | |||
- You can protect your server with a password via the console: 'set server_pw', | |||
or via the Start Server menu. | |||
|
|||
- You can ban or kick problemantic players via the console: 'ban client_ip_address' or 'kick client_ip_address' |
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.
This is misaligned with the other elements, i.e. add a single space before the -
and it is good.
This line is much longer than any of the other lines, making it harder to read in a console environment. Maybe add a newline after the :
?
* @param path The path to load the ban list from. | ||
* @return The number of entries loaded from the file or -1 if an error occurred while loading entries. | ||
*/ | ||
static int LoadBanList(const std::string& path) |
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.
Coding style for OpenTTD dictates that the &
is next to the variable/parameter.
The reason for this is that int* p, q;
is equivalent to int *p; int q;
and not int* p; int* q;
even though someone would expect that given the "type" is int*
.
*/ | ||
static int LoadBanList(const std::string& path) | ||
{ | ||
const char* const group_names[] = { |
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.
Coding style dictates a space before and after the *
in this case.
|
||
int loaded_count = 0; | ||
for (const IniItem *item = ban_group->item; item != nullptr; item = item->next) { | ||
// banned clients are added to global ban list when kicked or banned |
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.
This comment is misleading. Kicking does not put clients on the ban list. Maybe something along the lines of:
// add IP address to the ban list and kick clients using that IP address
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.
If we're at coding style, it also uses the wrong comment type (should be /* */
).
DEF_CONSOLE_CMD(ConBanListLoad) | ||
{ | ||
if (argc == 0) { | ||
IConsoleHelp("Loads the IP's of banned clients from a file: Usage 'banlist_load <path_to_file>'"); |
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.
The deviates from the style used for the other help messages, and I think it should be IPs (plural) not IP's (possessive). Also make it clear that you do not replace the ban list, but rather add them to the ban list
Load the IPs of banned clients from a file and add them to the ban list. Usage 'banlist_load <path_to_file>
DEF_CONSOLE_CMD(ConBanListSave) | ||
{ | ||
if (argc == 0) { | ||
IConsoleHelp("Saves the banned list of clients to a file: Usage 'banlist_save <path_to_file>'"); |
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.
The deviates from the style used for the other help messages, and I think it should be IPs (plural) not IP's (possessive).
Save the IPs of banned clients to a file. Usage 'banlist_save <path_to_file>
Motivation / Problem
Containerized instances of OpenTTD may not have a writable configuration file and may with to allow load/saving of the ban list from an external source.
See #7957 and this comment for details.
Description
Implements a console command to support loading and saving ban lists to a user-specified file.
Limitations
The openttd.cfg will be updated with the contents of the updated ban list when the user quits the server. This may not be desirable. This limitation appears acceptable when used under the circumstances described in #7957 (i.e. the openttd.cfg file is read-only).
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.
I would like to have this merged in after #8894 is merged. The console API is improved in that branch and I'd like to use the new functions in this PR.