TouchMe-Inc / l4d2_ds_script

Server management script
GNU General Public License v3.0
0 stars 0 forks source link

A few minor suggestions #1

Open keyCat opened 11 months ago

keyCat commented 11 months ago

I apologise in advance for the amount of formatting applied here, but my intention was to better illustrate each point.

==========================================

  1. I would recommend using set built-ins for your script. For example, set -e will exit the script for any error occurred during command execution, and thus prevents from performing any destructive / unstable actions accidentally:
    
    #!/bin/sh

set -e

2. Similarly, you could add `DEBUG` switch and activate `set -x` mode for traceability.

==========================================

3. You can simplify killing of your `screen`s by sending a [`quit`](https://linux.die.net/man/1/screen) command to them, instead of grep'ping and awk'ing PIDs for the `kill`, like you do here:
https://github.com/TouchMe-Inc/l4d2_ds_script/blob/fc0bf9aa5db197a03efc28ca81f667a27ba1009e/srcds#L96
https://github.com/TouchMe-Inc/l4d2_ds_script/blob/fc0bf9aa5db197a03efc28ca81f667a27ba1009e/srcds#L108

Example:

```bash
screen -S "$SCREENNAME" -X quit

==========================================

  1. I am not sure, why do you have to use a separate directory for each server fork? https://github.com/TouchMe-Inc/l4d2_ds_script/blob/fc0bf9aa5db197a03efc28ca81f667a27ba1009e/srcds#L41-L42

While this may be required for some wildly different server configs, unless you have that you can use the same SRCDS installation to run multiple identical instances, since there are no locks on any shared files (except for logs which can easily be forked by using sv_logsdir <dir> launch parameter).

You can validate it for yourself by running lslocks and examining the output:

$ lslocks
COMMAND            PID  TYPE   SIZE MODE  M START END PATH
srcds_linux     417413 FLOCK 231.4K READ  0     0   0 /home/user/Steam/logs/content_log.txt
srcds_linux     417413 FLOCK  70.4K READ  0     0   0 /home/user/Steam/logs/configstore_log.txt
srcds_linux     417413 FLOCK   2.3K READ  0     0   0 /home/user/Steam/logs/systemmanager.txt
srcds_linux     417413 FLOCK 249.2K READ  0     0   0 /home/user/Steam/logs/connection_log_27064.txt
srcds_linux     417413 FLOCK   820K WRITE 0     0   0 /home/user/server/left4dead2/logs15/L185_175_046_123_27064_202210132317_000.log
srcds_linux     418225 FLOCK 231.4K READ  0     0   0 /home/user/Steam/logs/content_log.txt
srcds_linux     418225 FLOCK  70.4K READ  0     0   0 /home/user/Steam/logs/configstore_log.txt
srcds_linux     418225 FLOCK   2.3K READ  0     0   0 /home/user/Steam/logs/systemmanager.txt
srcds_linux     418225 FLOCK 202.1K READ  0     0   0 /home/user/Steam/logs/connection_log_27080.txt
srcds_linux     418225 FLOCK   752K WRITE 0     0   0 /home/user/server/left4dead2/logs40/L185_175_046_123_27080_202210132317_000.log

==========================================

  1. Maybe I am missing something and this is intentional, but completely re-writting global PATH for the script may lead to unforeseen consequences in different environments: https://github.com/TouchMe-Inc/l4d2_ds_script/blob/fc0bf9aa5db197a03efc28ca81f667a27ba1009e/srcds#L66-L67

It might be better to concatenate your script's requirements with the existing PATH:

PATH="/bin:/usr/bin:/sbin:/usr/sbin:$PATH"

==========================================

  1. I can tell that this script is meant to be run as root (due to usage of su for command execution). This is generally very unsafe approach and it would be nice to make this configurable.

==========================================

Hope that helps. Cheers.

TouchMe-Inc commented 11 months ago

Hello. Thank you for writing. 1&2 Good point.

  1. This looks more like refactoring.
  2. I know about the possibility of running a server from one directory. In this case, there is a problem with sourcemod and its logs, as well as some plugins that have their own special configurations in “/configs/”. It's expensive to rewrite each of them.
  3. For the script I only considered Ubuntu 18/20
  4. Yes you are right. The script was written a long time ago and I simply missed this point. Thank you