Change in vdsm[master]: packaging: add a method to reconfigure without init.d parame...
juan.hernandez at redhat.com
juan.hernandez at redhat.com
Mon Oct 22 15:12:36 UTC 2012
Juan Hernandez has posted comments on this change.
Change subject: packaging: add a method to reconfigure without init.d parameter
......................................................................
Patch Set 1: Looks good to me, but someone else must approve
(3 inline comments)
Where is the "reconfigure" file created?
....................................................
File vdsm/vdsmd.init.in
Line 27: CONF_FILE=@CONFDIR@/vdsm.conf
Line 28: GETCONFITEM=@VDSMDIR@/get-conf-item
Line 29: prog=vdsm
Line 30: PIDFILE=@VDSMRUNDIR@/vdsmd.pid
Line 31: FORCE_RECONFIGURE=@VDSMLIBDIR@/reconfigure
FORCE_RECONFIGURE_FILE? A comment explaining why this is needed?
Line 32: RESPAWNPIDFILE=@VDSMRUNDIR@/respawn.pid
Line 33: CORE_DUMP_PATH=/var/log/core/core.%p.%t.dump
Line 34: DOM_METADATA_BACKUP_DIR=/var/log/vdsm/backup
Line 35: CORE_PATTERN=/proc/sys/kernel/core_pattern
Line 271: fi
Line 272:
Line 273: # do not reconfigure, return 0, so that vdsm start can continue.
Line 274: if [ "$force_reconfigure" != "force" ] &&
Line 275: ! [ -r "$FORCE_RECONFIGURE" ] &&
Why the "-r", isn't "-f" enough? As far as I understand you don't need to read the file at all.
A comment explaining here why we need two mechanisms for reconfigure? (As you did in the commit message)
Line 276: grep -q "$by_vdsm_vers" $lconf && grep -q "$by_vdsm_vers" $qconf && \
Line 277: grep -q "$by_vdsm_vers" $ldconf && grep -q "$by_vdsm_vers" $qlconf
Line 278: then
Line 279: log_success_msg $"$prog: libvirt already configured for vdsm "
Line 369: ovirt_store_config "$lconf" "$qconf" "$ldconf" "$llogr"
Line 370:
Line 371: /sbin/initctl restart libvirtd 2>/dev/null || :
Line 372:
Line 373: [ -r "$FORCE_RECONFIGURE" ] && rm -f "$FORCE_RECONFIGURE" || :
A comment explaining what is this?
This is really hard to understand, specially the || : . Can't we be a bit more verbose:
# Explain why we are doing this:
if condition
then
action
fi
Line 374: }
Line 375:
Line 376:
Line 377: RETVAL=0
--
To view, visit http://gerrit.ovirt.org/8725
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id4a8d5939fbefc6d3857d8083dfcf2180b9ba110
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernandez at redhat.com>
More information about the vdsm-patches
mailing list