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

Feature 7957, banlist: load/save ban lists from/to external files #8909

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@rpwjanzen
Copy link

@rpwjanzen rpwjanzen commented Mar 29, 2021

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.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

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.

@rpwjanzen rpwjanzen force-pushed the 7957-allow-external-banlist branch from 4ca5673 to 0656ebb Mar 29, 2021
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
@rpwjanzen rpwjanzen force-pushed the 7957-allow-external-banlist branch from 0656ebb to 908ec0a Mar 29, 2021
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
@rpwjanzen rpwjanzen force-pushed the 7957-allow-external-banlist branch 2 times, most recently from b521c4b to 5ecaf26 Mar 29, 2021
@rpwjanzen rpwjanzen force-pushed the 7957-allow-external-banlist branch from 5ecaf26 to d5934a7 Mar 29, 2021
@rpwjanzen rpwjanzen marked this pull request as ready for review Mar 30, 2021
@rpwjanzen
Copy link
Author

@rpwjanzen rpwjanzen commented Mar 31, 2021

@nielsmh Should I be requesting that you re-review this or wait for someone else? I do not know if you have "write access".

@rpwjanzen rpwjanzen requested a review from nielsmh Apr 1, 2021
@@ -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'
Copy link
Contributor

@rubidium42 rubidium42 Apr 27, 2021

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)
Copy link
Contributor

@rubidium42 rubidium42 Apr 27, 2021

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[] = {
Copy link
Contributor

@rubidium42 rubidium42 Apr 27, 2021

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
Copy link
Contributor

@rubidium42 rubidium42 Apr 27, 2021

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

Copy link
Member

@michicc michicc Apr 27, 2021

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>'");
Copy link
Contributor

@rubidium42 rubidium42 Apr 27, 2021

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>'");
Copy link
Contributor

@rubidium42 rubidium42 Apr 27, 2021

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>

@TrueBrain TrueBrain added this to the 1.12 milestone Aug 14, 2021
@TrueBrain TrueBrain removed this from the 12.0 milestone Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants