Dummy file for writing the review in the PR. #1
Conversation
Signed-off-by: gbenhaim <[email protected]>
| ## define global variables | ||
| export DEBUG="set +x" ## disabled debug | ||
| export execution_count=0 ## total executions counter | ||
| export DEFAULT_EXIT_CODE=255 ## default exit-code |
There was a problem hiding this comment.
It's a good practice to avoid global variables whenever possible.
For example, DEFAULT_EXIT_CODE is being used only in the finalize method, so it can be a local variable.
Also, the export command makes the variable an environment variable (which is then accessible by child process'), and I don't think it's needed in our case.
| # in case no returned-codes (such case if the script killed while the first invocation) , then it will exit with default-return-code (predefined as 255) | ||
| finalize() { | ||
|
|
||
| frequent_rc=${DEFAULT_EXIT_CODE} |
There was a problem hiding this comment.
Use local to make the variable valid only in the function scope otherwise, it will be declared globally.
| printf "|------Return Codes Summary-----|\n" | ||
| printf "| Return-Code | Occurrence |\n" | ||
| printf "|-------------------------------|\n" | ||
| echo ${list_rc[@]} | tr ' ' '\n' | sort | uniq -c | sort -rnk 1 | awk '{ printf "|\t%2d\t|\t%2d\t|\n", $2, $1 }' |
There was a problem hiding this comment.
You have here 5 calls to subprocess, which is a lot of overhead for finding the max value in an array.
I recommend using a simple for loop for this task.
| echo ${list_rc[@]} | tr ' ' '\n' | sort | uniq -c | sort -rnk 1 | awk '{ printf "|\t%2d\t|\t%2d\t|\n", $2, $1 }' | ||
| printf "|-------------------------------|\n" | ||
|
|
||
| frequent_rc=$(echo ${list_rc[@]} |tr ' ' '\n' | sort | uniq -c | sort -rnk 1 | head -1 | awk '{print $2}') |
There was a problem hiding this comment.
Most of this line is a code duplication of line 60.
| exec_net_trace () { | ||
|
|
||
| export NET_PCAP="${OUTPUT_DIR}/net_trace_`${TIME_SIGNUTURE}`.pcap" | ||
| [[ ${NET_TRACE} ]] && /usr/sbin/tcpdump -i any -vvv -nnN -tttt -s0 -c 4000 -w ${NET_PCAP} |
There was a problem hiding this comment.
Can you please explain what each flag is doing and why it's needed?
There was a problem hiding this comment.
-i any = listen to capture packets from all interfaces
Needs : as we don't have any assumption about the current network interfaces .
-vvv = to print more verbose outputs like : ttl , identification, total length and options in an IP packet are printed , more fields are printed from NFS reply packets ,etc...
Needs : to collect more fields & data info .
-nnN = protocols , port numbers , DNQ of host names will not be converted to names either .
Needs : keep the sources/destinations/protocols without any names convert .
-tttt = Print a timestamp in default format proceeded by date on each dump line .
Needs : put packet time & date
-s0 = use the required length to catch whole packets(default is 68 bytes - is adequate for IP, ICMP, TCP and UDP but may truncate protocol information from name server and NFS packets )
-c 4000 = capture only 4000 packets .
In this case , it set to 4000packets , it could be ignore-able if the execution-command runs long time , so keep tcpdump child-process run in background till the command execution finish , and then kill the tcpdump process .
-w ${NET_PCAP} = capture and save the file in a .pcap format .
| # --call-trace | ||
| if [[ ${CALL_TRACE} ]] ; then | ||
|
|
||
| /usr/bin/strace -ttT -o ${CALL_TRACE_OUT} ${COMMAND} 1>>${LOG_OUT} 2>>${LOG_ERR} |
There was a problem hiding this comment.
You shouldn't have an assumption about the location of strace's binary, it does not have to be the same on all systems.
Just use strace ... and let the user configure the PATH variable.
There was a problem hiding this comment.
Thanks for this comment , and agree with you .
| /usr/bin/strace -ttT -o ${CALL_TRACE_OUT} ${COMMAND} 1>>${LOG_OUT} 2>>${LOG_ERR} | ||
|
|
||
| # getting return code from strace.out file 'exit_group' printted in the last line in strace.out and include exit-code exit_group(exit-code) | ||
| this_rc="`grep exit_group ${CALL_TRACE_OUT} | tail -1 | cut -d'(' -f2 | cut -d')' -f1`" |
There was a problem hiding this comment.
strace returns the status of the command which had been executed.
No need for string manipulation.
There was a problem hiding this comment.
I already tried this in the design/development stage , and referred to the command manual doc , as it will return 0 when the command found else it will return non-zero .
The strace version is strace -- version 4.5.18
example :-
$ ls x ; echo $?
ls: x: No such file or directory
2
$ strace ls x 1>/dev/null 2>/dev/null ; echo $?
0
In the this example : the command for execution is ls x and it fail , but the strace itself success to execute "failed-command"
It will fail when the command not found and return non-zero value
$ strace lss ; echo $?
strace: lss: command not found
1
| fi | ||
|
|
||
|
|
||
| wait ${wait_pids} |
There was a problem hiding this comment.
Don't you have to kill tcpdump here? why waiting?
| export DEFAULT_EXIT_CODE=255 ## default exit-code | ||
| export EXEC_NAME=$(basename $0) ## script name | ||
| export OUTPUT_DIR="`pwd`" ## path for output & generated files | ||
| export TIME_SIGNUTURE="date +"%Y%m%d_%H%M%S"" ## current date&time |
There was a problem hiding this comment.
I think that putting this logic in a function makes more sense.
It's a bit misleading because you have to remember using $(...) to evelaute the time signature.
There was a problem hiding this comment.
This is possible idea for improvement & code tuning .
All variables except TIME_SIGNATURE could be initiated by a function , but the variable
TIME_SIGNATURE will be defined into the memory that include date command with time formatting (date+time , like : 20180104_180400)
and it's executed more than one time and get unique value , so it will be more efficient as the script will get the value from the memory , comparing of calling a separate function many times that it will take more time than memory access .
| while [[ ${execution_count} != ${COUNT} ]] && [[ ${failed_executions} != ${FAIL_COUNT} ]] ; do | ||
|
|
||
| export START_DATE=$(date +"%m/%d/%Y %H:%M:%S") | ||
| export CALL_TRACE_OUT="${OUTPUT_DIR}/call_trace_`${TIME_SIGNUTURE}`.out" |
There was a problem hiding this comment.
If strace will be called more than once in less than a second, the log will be overridden.
You can utilize mktemp for this task.
There was a problem hiding this comment.
For the files allocation/creation decision - i thought about two option :-
- to set a filename pattern for almost of generated files :
"output-path/metric-name_timestamp.out" - To use
mktemp.
The only difference here is when using mktemp the file will be created with zero size , instead of the 1st option , the filename will be created only when redirect the output to it , and another advantage is that we have more details about the file & the content as it include the timestamp .
But we can use mktemp in this case , in order to not override another files .
Signed-off-by: gbenhaim [email protected]