Alon Bar-Lev has posted comments on this change.

Change subject: core: Provide a wrapper script with ovirt to...
......................................................................


Patch Set 6:

(10 comments)

....................................................
File packaging/setup/dbutils/engine-psql.sh
Line 1: #!/bin/sh
if you are using bash statements, please set as bash... however I really prefer 
to remove bashism from new files.
Line 2: 
#################################################################################
Line 3: # The purpose of this script is to be a wrapper for support data 
manipulations on the
Line 4: #   database done up to now by psql.
Line 5: # The script supports all original psql flags and records the executed 
SQL and its


Line 12: # Load vars file:
Line 13: . "$(dirname "$(readlink -f "$0")")"/vars.sh
Line 14: 
Line 15: usage() {
Line 16:     psql_help=$(psql --help | sed "s@psql@engine-psql@g")
1. quotes

2. replace with $0
Line 17:     echo "${psql_help}"
Line 18:     exit 0
Line 19: }
Line 20: 


Line 17:     echo "${psql_help}"
Line 18:     exit 0
Line 19: }
Line 20: 
Line 21: permission_error() {
use cat?

print errors to stderr

 cat >&2 << __EOF__
 line
 line
 __EOF__
Line 22:     echo  "Insufficient permissions on $LOGDIR"
Line 23:     echo  "Please execute again with a user that has write access to 
this directory or use sudo"
Line 24:     exit 4
Line 25: }


Line 27: 
Line 28: # we have to log the file content
Line 29: # supporting here all possible formats : -f <file> --file <file> and 
--file=<file>
Line 30: parseArgs() {
Line 31:         while [ -n "$1" ]; do
use tab as indent to align with other script of us.
Line 32:                 local x="$1"
Line 33:                 local v="${x#*=}"
Line 34:                 shift
Line 35:                 case "${x}" in


Line 51: # do this in function so we do not lose $@
Line 52: parseArgs "$@"
Line 53: 
Line 54: if [ ! -d "${LOGDIR}" ]; then
Line 55:     mkdir -p "${LOGDIR}"
this should be done at spec
Line 56:     if [ $? -ne 0 ]; then
Line 57:         echo "Can not create/find log directory ${LOGDIR}"
Line 58:         exit 1
Line 59:     fi


Line 65:     permission_error
Line 66: fi
Line 67: 
Line 68: # Echo file content to the log file
Line 69: if [[ -n "${FILE}" ]]; then
do not use bash please
Line 70:     if ! [ -r "${FILE}" ]; then
Line 71:         echo "${FILE} is unreadable." >> ${LOGFILE}
Line 72:         exit 2
Line 73:     fi


Line 70:     if ! [ -r "${FILE}" ]; then
Line 71:         echo "${FILE} is unreadable." >> ${LOGFILE}
Line 72:         exit 2
Line 73:     fi
Line 74:     content=$(cat ${FILE})
quot^2

 content="$(cat "${FILE}")"
Line 75:     echo  "file content is: " >> ${LOGFILE}
Line 76:     echo  "${content}" >> ${LOGFILE}
Line 77: fi
Line 78: 


Line 76:     echo  "${content}" >> ${LOGFILE}
Line 77: fi
Line 78: 
Line 79: echo  "result: " >> ${LOGFILE}
Line 80: psql "$@" &>> ${LOGFILE}
please don't use bash (&>>)
Line 81: ret=$?
Line 82: # Compress the file
Line 83: gzip ${LOGFILE}
Line 84: if [ $ret -ne 0 ]; then


Line 79: echo  "result: " >> ${LOGFILE}
Line 80: psql "$@" &>> ${LOGFILE}
Line 81: ret=$?
Line 82: # Compress the file
Line 83: gzip ${LOGFILE}
quote all ${LOGFILE}
Line 84: if [ $ret -ne 0 ]; then
Line 85:     exit 3


Line 80: psql "$@" &>> ${LOGFILE}
Line 81: ret=$?
Line 82: # Compress the file
Line 83: gzip ${LOGFILE}
Line 84: if [ $ret -ne 0 ]; then
why change ret?
Line 85:     exit 3


-- 
To view, visit http://gerrit.ovirt.org/16273
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I468f830196bd95dc013a5142f9aa0d508e687d90
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Eli Mesika <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Lee Yarwood <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to