Zhou Zheng Sheng has posted comments on this change.
Change subject: Extend vdsm-tool: moving configure libvirt to external shell
script
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(5 inline comments)
....................................................
File lib/vdsm/tool/libvirt_configure.py
Line 26: def libvirt_configure():
Line 27: """
Line 28: Configure libvirt conf files
Line 29: """
Line 30: rc, out, err = utils.execCmd(["./libvirt_configure.sh"], sudo=True)
The current working directory is the not
"%{python_sitearch}/%{vdsm_name}/tool/", so "./libvirt_configure.sh" does not
refer to the correct path.
I test the following solution
import os.path
shDir = os.path.dirname(os.path.realpath(__file__))
shPath = os.path.join(shDir, "libvirt_configure.sh")
rc, out, err = utils.execCmd([shPath], sudo=True)
Another choice (untested) is to put libvirt_configure.sh in /usr/libexec/vdsm.
Firstly, in Makefile.am, we can have
nodist_vdsmexec_SCRIPTS = \
libvirt_configure.sh
Then in constants.py.in, we can have
EXT_LIBVIRT_CONFIGURE = os.path.join('@LIBEXECDIR@', 'libvirt_configure.sh')
At last we can use EXT_LIBVIRT_CONFIGURE in libvirt_configure.py.
I also think .py module should forward all the arguments to .sh script.
def libvirt_configure(*args):
# ...
cmd = shPath
cmd.append(args)
rc, out, err = utils.execCmd(cmd, sudo=True)
# ...
Line 31:
Line 32: if rc != 0:
Line 33: msg = "Failed to configure libvirt: %s" % err
Line 34: raise RuntimeError(msg)
Line 31:
Line 32: if rc != 0:
Line 33: msg = "Failed to configure libvirt: %s" % err
Line 34: raise RuntimeError(msg)
Line 35: else:
Agree with Alon. The "else" can be removed, because the execution jumps out
when it raises an exception. We can safely move the following code out of
"else" block's protection.
Line 36: for msg in out:
Line 37: sys.stdout.write(msg + '\n')
....................................................
File lib/vdsm/tool/libvirt_configure.sh.in
Line 5: # Licensed to you under the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or
Line 7: # (at your option) any later version. See the files README and
Line 8: # LICENSE_GPL_v2 which accompany this distribution.
Line 9: #
I think it should use the newer copyright boilerplate that is used in the .py
files.
Line 10:
Line 11: . @LIBEXECDIR@/ovirt_functions.sh
Line 12: GETCONFITEM=@VDSMDIR@/get-conf-item
Line 13: CONF_FILE=@CONFDIR@/vdsm.conf
....................................................
File vdsm/vdsmd.init.in
Line 214: args="$args $LCONF $QCONF $LDCONF $QLCONF"
Line 215: ;;
Line 216: esac
Line 217: /usr/bin/vdsm-tool configure_libvirt $args
Line 218: }
Why don't we move this reconfigure() function and the $LCONF $QCONF $LDCONF
$QLCONF definitions to libvirt_configure.sh.in?
I see xCONF variable definitions are in libvirt_configure.sh.in, we can delete
those definitions from vdsmd.init.in, and move all the things that referring
those variables to libvirt_configure.sh.in.
Before this patch,
reconfigure force
reconfigure
does reconfiguration of libvirt, and
reconfigure noforce
does configuration of libvirt if it is not configured before.
After this patch, I think we should preserve this semantics in "vdsm-tool
libvirt-configure".
vdsm-tool libvirt-configure force
vdsm-tool libvirt-configure
force configure libvirt, and
vdsm-tool libvirt-configure noforce
configures libvirt if it is not configured before.
Then in libvirt_configure.py, we can passthru all the arguments (force/noforce)
to libvirt_configure.sh. In libvirt_configure.sh, we can have reconfigure()
function, and use ' reconfigure "$@" ' to invoke it.
Line 219:
Line 220: has_systemd() {
Line 221: /bin/mountpoint -q /cgroup/systemd ||
Line 222: /bin/mountpoint -q /sys/fs/cgroup/systemd
Line 358: RETVAL=$?
Line 359: ;;
Line 360: reconfigure)
Line 361: # Jump over 'reconfigure'
Line 362: shift 1
Looks ' vdsm-tool libvirt-configure "$@" ' is better. We can move all the
related things to vdsm-tool, otherwise we have xCONF definitions in both
vdsmd.init.in and /libvirt_configure.sh.in.
Line 363: reconfigure "$@"
Line 364: RETVAL=$?
Line 365: ;;
Line 366: *)
--
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: 2
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
