Change in vdsm[master]: vdsmd and supervdsmd: extract common init tasks and add init...

Alon Bar-Lev alonbl at redhat.com
Thu Jul 18 10:22:16 UTC 2013


Alon Bar-Lev has posted comments on this change.

Change subject: vdsmd and supervdsmd: extract common init tasks and add init adapter
......................................................................


Patch Set 8: (5 inline comments)

....................................................
File vdsm/daemonAdapter
Line 47:     parser.add_argument(
Line 48:         '--target-option', dest='targetOption', default=list(),
Line 49:         metavar='target_opt0,target_opt1,...', action='append',
Line 50:         help='Add comma seperated options to be passed to target, multiple '
Line 51:         'occurrences of "--target-option opt" accumulates the options')
I recommend using action='append' and have multiple parameters.

 -t x -t y -t z

it avoids having special characters etc...
Line 52:     args = parser.parse_args(sys.argv[1:])
Line 53:     return args
Line 54: 
Line 55: 


....................................................
File vdsm/supervdsmd.service.in
Line 2: Description="Auxiliary vdsm service for running helper functions as root"
Line 3: 
Line 4: [Service]
Line 5: Type=simple
Line 6: ExecStart=@PYTHON@ "@VDSMDIR@/daemonAdapter" -t "@PYTHON@" "--target-option=@VDSMDIR@/supervdsmServer.pyc,--sockfile=@VDSMRUNDIR@/svdsm.sock"
I think that if we remove the py suffix from supervdsmServer, then we can avoid doing this python magic similar to what we do with vdsm.


Line 3
Line 4
Line 5
Line 6
Line 7
add environment?


....................................................
File vdsm/supervdsmServer.py
Line 409: 
Line 410: 
Line 411: def _parse_args():
Line 412:     argDict = {}
Line 413:     opts, args = getopt.getopt(sys.argv[1:], "h", ["sockfile=", "pidfile="])
I curios... why do you use getopt and not argparse in this case?
Line 414:     for o, v in opts:
Line 415:         o = o.lower()
Line 416:         if o == "--sockfile":
Line 417:             argDict['sockfile'] = v


....................................................
File vdsm/vdsmd.init.in
Line 84: 
Line 85:     echo $"Starting up vdsm daemon: "
Line 86:     NICELEVEL="${NICE_LOWEST}" daemon --user=vdsm "@VDSMDIR@/daemonAdapter" \
Line 87:         -r --respawn-option="--minlifetime,10,--daemon,--masterpid,${RESPAWNPIDFILE}" \
Line 88:         -c /dev/null -t "${VDSM_BIN}" "--target-option=--pidfile=${PIDFILE}"
I think that either short or long options should be used in one statement... but matter of taste.
Line 89:     RETVAL=$?
Line 90:     [ "$RETVAL" -eq 0 ] && log_success_msg $"$prog start" || log_failure_msg $"$prog start"
Line 91:     [ "$RETVAL" -eq 0 ] && touch /var/lock/subsys/vdsmd
Line 92: }


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id8e514df1ca88500f746242134ddb24c31588046
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: 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