Change in vdsm[master]: vdsmd.init.in: break to pieces

Alon Bar-Lev alonbl at redhat.com
Sun Jun 30 19:06:38 UTC 2013


Alon Bar-Lev has posted comments on this change.

Change subject: vdsmd.init.in: break to pieces
......................................................................


Patch Set 8: (6 inline comments)

....................................................
File vdsm/init/init_gencerts.sh
Line 22:     local rt=0
Line 23:     if ! "${LIBEXECDIR}/vdsm-gencerts.sh" --check; then
Line 24:         printf $"Configuring a self-signed VDSM host certificate: "
Line 25:         "${LIBEXECDIR}/vdsm-gencerts.sh"
Line 26:         rt=$?
this can be replaced by || return 1 within the above statement
Line 27:         [ "${rt}" -eq 0 ] && echo "OK" || echo "Failed"
Line 28:     fi
Line 29:     return ${rt}
Line 30: }


Line 23:     if ! "${LIBEXECDIR}/vdsm-gencerts.sh" --check; then
Line 24:         printf $"Configuring a self-signed VDSM host certificate: "
Line 25:         "${LIBEXECDIR}/vdsm-gencerts.sh"
Line 26:         rt=$?
Line 27:         [ "${rt}" -eq 0 ] && echo "OK" || echo "Failed"
this should go into scheduler, common to all.
Line 28:     fi
Line 29:     return ${rt}
Line 30: }
Line 31: 


....................................................
File vdsm/init/init_shutdown_conflicting_srv.sh
Line 22: 
Line 23: shutdown_conflicting_srv() {
Line 24:     local srv
Line 25: 
Line 26:     for srv in "${CONFLICTING_SERVICES}"
if you use:

 if xxx;l then

you should use:

 for srv in xxx; do
Line 27:     do
Line 28:         /usr/bin/vdsm-tool service-disable "${srv}"
Line 29:         if /usr/bin/vdsm-tool service-status "${srv}" > /dev/null 2>&1;
Line 30:         then


Line 24:     local srv
Line 25: 
Line 26:     for srv in "${CONFLICTING_SERVICES}"
Line 27:     do
Line 28:         /usr/bin/vdsm-tool service-disable "${srv}"
all these absolute paths should be removed.

You are inconsistent with your-self... sometimes you use ${VDSM_TOOL} and sometimes hard code path.
Line 29:         if /usr/bin/vdsm-tool service-status "${srv}" > /dev/null 2>&1;
Line 30:         then
Line 31:             if [ "${srv}" = "libvirt-guests" ]; then
Line 32:                 /bin/rm -f "${LOCKDIR}/libvirt-guests"


....................................................
File vdsm/init/init_start_needed_srv.sh
Line 33:             /usr/bin/vdsm-tool service-start "${srv}"
Line 34:             ret_val=$?
Line 35:             if [ "${ret_val}" -ne 0 ]
Line 36:             then
Line 37:                 bark "${prog}: Dependent ${srv }failed to start"
don't know what bark is, and where you take prog from. I don't think this should be here as I wrote before. These functions should only log to file, and return success/failure. scheduler should decide what to print.
Line 38:                 return ${ret_val}
Line 39:             fi
Line 40:         fi
Line 41:     done


....................................................
File vdsm/init/init_test_space.sh
Line 25: 
Line 26: test_space() {
Line 27:     local MIN_SPACE_KB=10000
Line 28: 
Line 29: 	if [ "$(free_space /var/log/vdsm)" -lt "${MIN_SPACE_KB}" ]; then
tabs or spaces.
Line 30:         bark "${prog}: low log space"
Line 31:         return 1
Line 32:     fi
Line 33:     return 0


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0c652704000c764c6e6f248605c6a3f4f3af5ace
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list