ctubbsii commented on code in PR #5165:
URL: https://github.com/apache/accumulo/pull/5165#discussion_r1879210488
##########
assemble/bin/accumulo-cluster:
##########
@@ -184,14 +184,14 @@ isDebug() {
}
debug() {
- isDebug && echo "${@@P}"
+ isDebug && echo "$1"
Review Comment:
I get that this is being done because it is assuming that there's only one
arg because debugAndRun has been changed to flatten it to a single arg, but if
this code is called anywhere else other than debugAndRun where that's not the
case, then only the first arg is taken and the rest ignored and not printed.
This could just be:
```suggestion
isDebug && echo "$@"
```
##########
assemble/bin/accumulo-service:
##########
@@ -114,16 +114,15 @@ function start_service() {
nohup "${bin}/accumulo" "$service_type" "$@" "${PROPERTY_OVERRIDES[@]}"
>"$outfile" 2>"$errfile" </dev/null &
echo "$!" >"${pid_file}"
- done
-
- # Check the max open files limit and selectively warn
- max_files_open=$(ulimit -n)
- if [[ -n $max_files_open ]]; then
- max_files_recommended=32768
- if ((max_files_open < max_files_recommended)); then
- echo "WARN : Max open files on $HOST is $max_files_open, recommend
$max_files_recommended" >&2
+ # Check the max open files limit and selectively warn
+ max_files_open=$(ulimit -n)
+ if [[ -n $max_files_open ]]; then
+ max_files_recommended=32768
+ if ((max_files_open < max_files_recommended)); then
+ echo "WARN : Max open files on $HOST is $max_files_open, recommend
$max_files_recommended" >&2
+ fi
fi
- fi
+ done
Review Comment:
Why do you want the max files warning inside the loop? Isn't once enough?
##########
assemble/bin/accumulo-service:
##########
@@ -98,7 +98,7 @@ function start_service() {
pid=$(cat "$pid_file")
if kill -0 "$pid" 2>/dev/null; then
echo "$HOST : ${service_name} already running (${pid})"
- exit 0
+ continue
Review Comment:
It looks like this might be a separate bug from the ones in accumulo-cluster?
##########
assemble/bin/accumulo-cluster:
##########
@@ -184,14 +184,14 @@ isDebug() {
}
debug() {
- isDebug && echo "${@@P}"
+ isDebug && echo "$1"
}
debugAndRun() {
- debug "$@"
+ # Use $* to convert all args as a single argument
+ debug "$*"
if ! isDebug; then
- # shellcheck disable=SC2294
- eval "${@@P}"
+ eval "$(printf "%q " "$@")"
Review Comment:
This doesn't actually fix the problem with the quoting, because eval is the
thing requiring an extra layer of quoting to work.
##########
assemble/bin/accumulo-cluster:
##########
@@ -304,7 +304,8 @@ function execute_command() {
if [[ $ARG_LOCAL == 1 ]]; then
debugAndRun ACCUMULO_CLUSTER_ARG="${servers_per_host}"
"${bin}/accumulo-service" "$service" "$control_cmd" "-o"
"general.process.bind.addr=$host" "$@"
else
- debugAndRun "$SSH" "$host" "bash -c
'ACCUMULO_CLUSTER_ARG=${servers_per_host} ${bin}/accumulo-service \"$service\"
\"$control_cmd\" \"-o\" \"general.process.bind.addr=$host\" ${*@Q}'"
+ SSH=("${INITAL_SSH[@]}" "$host" "bash -c
'ACCUMULO_CLUSTER_ARG=${servers_per_host} ${bin}/accumulo-service \"$service\"
\"$control_cmd\" \"-o\" \"general.process.bind.addr=$host\" $(printf "\"%q\" "
"${@}")'")
+ debugAndRun "${SSH[@]}"
Review Comment:
I get that the intent here is to avoid flattening the SSH args, but we don't
need to store this as a new variable. The args to the debugAndRun function are
already an array. We can just avoid flattening the SSH variable and initially
assign it as an array (below) rather than bother with the intermediate variable
in the first place.
However, while this does fix an issue with the SSH args being flattened
(which wasn't really ever a problem, since they are hard-coded in this script
anyway), it doesn't change the problems with the need to quote the contents of
the bash command multiple times. The eval will not see the same thing that the
debug line would have printed. It will see fewer quotes. That's not important
for some of the args, since many of them won't have spaces or quotes in them,
but it is a problem if this is written to handle `$@` in a general way, and
it's also a problem with `$bin`, since that's a path and its contents are
completely unknown and could contain spaces, so it needs to be properly quoted.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
