Zhou Zheng Sheng has posted comments on this change.
Change subject: Extend vdsm-tool: moving configure libvirt to external shell
script
......................................................................
Patch Set 6: I would prefer that you didn't submit this
(6 inline comments)
I'm very glad to see the new patch set. I agree to the design. -1 for some
problems in the implementation.
....................................................
File lib/vdsm/tool/libvirt_configure.py
Line 32: raise RuntimeError("Must run as root")
Line 33:
Line 34: rc, out, err = utils.execCmd([os.path.join(
Line 35: os.path.dirname(vdsm.tool.__file__),
Line 36: 'libvirt_configure.sh')] +
list(args),
It should insert "reconfigure" between the 'libvirt_configure.sh' and
'list(args)' when it's to configure libvirt. The vdsm.tool shifts out the
command name automatically in "vdsm-tool command args", and passes the rest of
of the arguments to this function.
Line 37: raw=True)
Line 38:
Line 39: sys.stdout.write(out)
Line 40: sys.stderr.write(err)
Line 41:
Line 42: if rc != 0:
Line 43: raise RuntimeError("Failed to configure libvirt")
Line 44:
Line 45: return 0
It needs a new function that expose "vdsm-tool test_conflict_configurations". I
suggest the following (not tested).
def do_libvirt_configure(action, *args):
"""
Invoke libvirt_configure.sh and passthru the args
"""
if os.getuid() != 0:
raise RuntimeError("Must run as root")
rc, out, err = utils.execCmd([os.path.join(
os.path.dirname(vdsm.tool.__file__),
'libvirt_configure.sh')] + [action] +
list(args),
raw=True)
sys.stdout.write(out)
sys.stderr.write(err)
if rc != 0:
raise RuntimeError("Failed to %s libvirt" % action)
return 0
@vdsm.tool.expose("libvirt-configure")
def libvirt_configure(*args):
"""
Blah
"""
return do_libvirt_configure("reconfigure", *args)
@vdsm.tool.expose("test_conflict_configurations"):
def test_conflict_configurations(*args):
"""
Blah
"""
return do_libvirt_configure("test_conflict_configurations", *args)
....................................................
File lib/vdsm/tool/libvirt_configure.sh.in
Line 291:
Line 292: case "$1" in
Line 293: reconfigure)
Line 294: if [ "$#" = '6' ]; then
Line 295: # Conf files as variables to allow pass them as parameter
Maybe the indent tabs can be replaced by spaces (line 295 to 314)
Line 296: start_configure "$1" "$2" "$3" "$4" "$5"
Line 297: else
Line 298: echo 'Configure default conf files:'"$LCONF" "$QCONF"
"$LDCONF" "$QLCONF"
Line 299: start_configure "$LCONF" "$QCONF" "$LDCONF" "$QLCONF" "$1"
Line 309: fi
Line 310: RETVAL=$?
Line 311: ;;
Line 312: *)
Line 313: echo "Usage: $0 {reconfigure *conf-files [force or
noforce]|test_conflicts *conf-files}"
The usage documentation does not agree with the code.
The line 303 matches "test_conflict_configurations", but the doc says
"test_conflicts".
Line 314: RETVAL=2
Line 315: esac
Line 316:
Line 317: exit $RETVAL
....................................................
File vdsm/vdsmd.init.in
Line 33: QEMU_DUMP_PATH="/var/log/core"
Line 34: NEEDED_SERVICES="iscsid multipathd ntpd wdmd sanlock network libvirtd
Line 35: supervdsmd"
Line 36: CONFLICTING_SERVICES="libvirt-guests"
Line 37: VDSM_TOOL=/usr/bin/vdsm-tool
Does "@BINDIR@/vdsm-tool" work?
Line 38:
Line 39: is_coredump=`$GETCONFITEM $CONF_FILE vars core_dump_enable false | tr
A-Z a-z`
Line 40: [ "$is_coredump" != "true" ] && is_coredump=false
Line 41:
Line 243:
Line 244: test_already_running && return 0
Line 245:
Line 246: if ! (test_space && test_lo && \
Line 247: `"$VDSM_TOOL" test_conflict_configurations`); then
I think
if ! (test_space && test_lo && \
"$VDSM_TOOL" test_conflict_configurations); then
should work. It just needs the return value of vdsm-tool, so back quotes are
not needed.
Line 248: return 1
Line 249: fi
Line 250:
Line 251: echo $"Starting up vdsm daemon: "
--
To view, visit http://gerrit.ovirt.org/15216
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id58b129afbf141a47a85b421961bf5b1776b41e4
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
