ctubbsii commented on code in PR #5750:
URL: https://github.com/apache/accumulo/pull/5750#discussion_r2227032393
##########
assemble/bin/accumulo-service:
##########
@@ -230,14 +231,14 @@ function list_processes() {
local pid_file
local pid
local port
- pid_file="${ACCUMULO_PID_DIR}/accumulo-${process}.pid"
- pid=$(<"$pid_file")
- port=$(netstat -tnlp 2>/dev/null | grep "$pid" | awk '{print $4}' | awk -F
':' '{print $NF}' | paste -sd "," -)
- # check that only a single port was seen
- if (($(echo "$port" | grep -c -E '^[0-9]+(,[0-9]+)*$') != 1)); then
- echo "ERROR unexpected ports $(hostname) process:$process pid:$pid
ports:$port" >&2
- else
+ pid_file="$ACCUMULO_PID_DIR/accumulo-$process.pid"
+ pid=$(<"$pid_file") # read the contents of the file into the $pid variable
+ port=$(ss -tnlp 2>/dev/null | grep -F "pid=$pid," | awk '{print $4}' | awk
-F : '{print $NF}' | paste -sd,)
+
+ if [[ $port =~ ^[1-9][0-9]+(,[0-9]+)*$ ]]; then
Review Comment:
even with matching multiple ports, the pattern could be made stricter:
```suggestion
if [[ $port =~ ^[1-9][0-9]{1,4}(,[1-9][0-9]{1,4})*$ ]]; then
```
##########
assemble/bin/accumulo-service:
##########
@@ -230,14 +231,14 @@ function list_processes() {
local pid_file
local pid
local port
- pid_file="${ACCUMULO_PID_DIR}/accumulo-${process}.pid"
- pid=$(<"$pid_file")
- port=$(netstat -tnlp 2>/dev/null | grep "$pid" | awk '{print $4}' | awk -F
':' '{print $NF}' | paste -sd "," -)
- # check that only a single port was seen
- if (($(echo "$port" | grep -c -E '^[0-9]+(,[0-9]+)*$') != 1)); then
- echo "ERROR unexpected ports $(hostname) process:$process pid:$pid
ports:$port" >&2
- else
+ pid_file="$ACCUMULO_PID_DIR/accumulo-$process.pid"
+ pid=$(<"$pid_file") # read the contents of the file into the $pid variable
+ port=$(ss -tnlp 2>/dev/null | grep -F "pid=$pid," | awk '{print $4}' | awk
-F : '{print $NF}' | paste -sd,)
+
+ if [[ $port =~ ^[1-9][0-9]+(,[0-9]+)*$ ]]; then
echo "$process $pid $port"
+ else
+ echo "ERROR unexpected port format $(hostname) process:$process pid:$pid
ports:$port" >&2
Review Comment:
This will echo this to STDERR and then exit normally, without an error code.
It should probably do a `return 1` from the function after the echo to ensure
the calling code sees that the script experienced an error. This is also
happening in a loop, so maybe you want to keep going and exit with an error at
the end, instead?
##########
assemble/bin/accumulo-service:
##########
@@ -230,14 +231,14 @@ function list_processes() {
local pid_file
local pid
local port
- pid_file="${ACCUMULO_PID_DIR}/accumulo-${process}.pid"
- pid=$(<"$pid_file")
- port=$(netstat -tnlp 2>/dev/null | grep "$pid" | awk '{print $4}' | awk -F
':' '{print $NF}' | paste -sd "," -)
- # check that only a single port was seen
- if (($(echo "$port" | grep -c -E '^[0-9]+(,[0-9]+)*$') != 1)); then
- echo "ERROR unexpected ports $(hostname) process:$process pid:$pid
ports:$port" >&2
- else
+ pid_file="$ACCUMULO_PID_DIR/accumulo-$process.pid"
+ pid=$(<"$pid_file") # read the contents of the file into the $pid variable
+ port=$(ss -tnlp 2>/dev/null | grep -F "pid=$pid," | awk '{print $4}' | awk
-F : '{print $NF}' | paste -sd,)
Review Comment:
The grep for `pid=$pid` is good, and I see why you added the `,` to the
pattern, but the `fd` field is not guaranteed to always be there, and the pid
isn't always guaranteed to be prior to it. Instead, you could grep for word
boundaries, and then you don't need to look for the comma:
```suggestion
port=$(ss -tnlp 2>/dev/null | grep -wF "pid=$pid" | awk '{print $4}' |
awk -F : '{print $NF}' | paste -sd,)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
