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