Alon Bar-Lev has posted comments on this change.
Change subject: Extend vdsm-tool: moving configure libvirt to external shell
script
......................................................................
Patch Set 17: Looks good to me, but someone else must approve
(21 inline comments)
....................................................
File lib/vdsm/tool/libvirt_configure.py
Line 53: """
Line 54: force = ''
Line 55: if len(sys.argv) >= 2:
Line 56: if sys.argv[2] == '--force':
Line 57: force = 'force'
I would have pass --force into the script and modify the script to accept
--force and remove this logic by passing all args, but not that important.
Line 58: return exec_libvirt_configure("reconfigure",
Line 59: force)
Line 60:
Line 61:
....................................................
File lib/vdsm/tool/libvirt_configure.sh.in
Line 1: #! /bin/sh
do we want to fix this file as well or do this in a different patch?
some comments for cleanup the script, not that the job will be done, but at
least it will be somewhat better.
Line 2: # Copyright 2013 Red Hat, Inc.
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 23: QEMU_DUMP_PATH="/var/log/core"
Line 24: LCONF=@sysconfdir@/libvirt/libvirtd.conf
Line 25: QCONF=@sysconfdir@/libvirt/qemu.conf
Line 26: LDCONF=@sysconfdir@/sysconfig/libvirtd
Line 27: QLCONF=@sysconfdir@/libvirt/qemu-sanlock.conf
missing quotes.
Line 28:
Line 29: # trigger for reconfiguration
Line 30: FORCE_RECONFIGURE=@VDSMLIBDIR@/reconfigure
Line 31:
Line 26: LDCONF=@sysconfdir@/sysconfig/libvirtd
Line 27: QLCONF=@sysconfdir@/libvirt/qemu-sanlock.conf
Line 28:
Line 29: # trigger for reconfiguration
Line 30: FORCE_RECONFIGURE=@VDSMLIBDIR@/reconfigure
quoes
Line 31:
Line 32: get_libvirt_conf_item() {
Line 33: local cfile
Line 34: local key
Line 33: local cfile
Line 34: local key
Line 35:
Line 36: cfile=$1
Line 37: key=$2
should be:
local cfile="$1"
local key="$2"
Line 38: /bin/grep "^\s*$key\s*=" "$cfile" | \
Line 39: /usr/bin/tail -1 | /bin/sed
"s/\s*$key\s*=\s*//;s/\s*\(#.*\)\?$//"
Line 40: }
Line 41:
Line 34: local key
Line 35:
Line 36: cfile=$1
Line 37: key=$2
Line 38: /bin/grep "^\s*$key\s*=" "$cfile" | \
don't understand the use of full paths... but I guess we won't solve this ever.
please use "${xxx}" notation
Line 39: /usr/bin/tail -1 | /bin/sed
"s/\s*$key\s*=\s*//;s/\s*\(#.*\)\?$//"
Line 40: }
Line 41:
Line 42: test_conflicting_conf() {
Line 41:
Line 42: test_conflicting_conf() {
Line 43: local listen_tcp
Line 44: local auth_tcp
Line 45: local ssl
move local to where it actually used
Line 46:
Line 47: lconf="$1"
Line 48: qconf="$2"
Line 49:
Line 44: local auth_tcp
Line 45: local ssl
Line 46:
Line 47: lconf="$1"
Line 48: qconf="$2"
local for the above? and move to be as first
Line 49:
Line 50: ssl=`"$GETCONFITEM" "$VDSM_CONF_FILE" vars ssl true | tr A-Z a-z`
Line 51: if [ "$ssl" = '' ]; then
Line 52: echo 'FAILED: Could not read SSL configuration' 1>&2
Line 47: lconf="$1"
Line 48: qconf="$2"
Line 49:
Line 50: ssl=`"$GETCONFITEM" "$VDSM_CONF_FILE" vars ssl true | tr A-Z a-z`
Line 51: if [ "$ssl" = '' ]; then
should be:
if [ -z "${ssl}" ]; then
Line 52: echo 'FAILED: Could not read SSL configuration' 1>&2
Line 53: return 3
Line 54: fi
Line 55:
Line 59: fi
Line 60:
Line 61: listen_tcp="`get_libvirt_conf_item $lconf listen_tcp`"
Line 62: auth_tcp="`get_libvirt_conf_item $lconf auth_tcp`"
Line 63: spice_tls="`get_libvirt_conf_item $qconf spice_tls`"
here specify local
please use $() instead of ``
Line 64:
Line 65: if [ "$listen_tcp" = "1" -a "$auth_tcp" = '"none"' -a "$spice_tls"
= "0" ];
Line 66: then
Line 67: echo "SUCCESS: No conflicts between configuration files"
Line 62: auth_tcp="`get_libvirt_conf_item $lconf auth_tcp`"
Line 63: spice_tls="`get_libvirt_conf_item $qconf spice_tls`"
Line 64:
Line 65: if [ "$listen_tcp" = "1" -a "$auth_tcp" = '"none"' -a "$spice_tls"
= "0" ];
Line 66: then
please decide which convention...
should be:
if xxx; then
else
fi
to be aligned with other blocks.
Line 67: echo "SUCCESS: No conflicts between configuration files"
Line 68: return 0
Line 69: else
Line 70: echo "FAILED: conflicting vdsm and libvirt-qemu tls
configuration."
Line 75: fi
Line 76: }
Line 77:
Line 78: libvirtd_sysv2upstart()
Line 79: {
please decide convention.
should be:
xxx() {
}
to be aligned with other blocks.
Line 80: # On RHEL 6, libvirtd can be started by either SysV init or
Upstart.
Line 81: # We prefer upstart because it respawns libvirtd if when libvirtd
crashed.
Line 82:
Line 83: if ! [ -x /sbin/initctl ]; then
Line 90:
Line 91: packaged=`/bin/rpm -ql libvirt libvirt-daemon | \
Line 92: /bin/grep libvirtd.upstart | /usr/bin/tail -1`
Line 93: target=/etc/init/libvirtd.conf
Line 94:
local here, use $()
Line 95: if ! [ -f "$packaged" ]; then
Line 96: # libvirtd package does not provide libvirtd.upstart,
Line 97: # this could happen in Ubuntu or other distro,
Line 98: # so continue to use system default init mechanism
Line 101:
Line 102: # Stop libvirt SysV service before configure upstart
Line 103: if [ ! -f "$target" ]; then
Line 104: @CHKCONFIG_PATH@ libvirtd off
Line 105: @SERVICE_PATH@ libvirtd stop
"@SERVICE_PATH@" ..
Line 106: fi
Line 107:
Line 108: if ! diff -q "$packaged" "$target" >/dev/null;
Line 109: then
Line 124: local key
Line 125: local val
Line 126: cfile="$1"
Line 127: key="$2"
Line 128: val="$3"
add local before vars remove the local up
Line 129:
Line 130: /bin/grep -q "^\s*$key\s*=" "$cfile" || \
Line 131: echo "$key=$val" >> "$cfile"
Line 132: }
Line 139: if [ -f /etc/rhev-hypervisor-release ]; then
Line 140: return 0
Line 141: else
Line 142: return 1
Line 143: fi
no need for this complex statement you can do something like:
isOvirt() {
[ "$(echo /etc/ovirt-node-*-release)" != "/etc/ovirt-node-*-release"] || \
[ -f /etc/rhev-hypervisor-release ]
}
Line 144: }
Line 145:
Line 146: start_configure() {
Line 147: # if SysV init file and Upstart job file both exist, prefer
Upstart
Line 146: start_configure() {
Line 147: # if SysV init file and Upstart job file both exist, prefer
Upstart
Line 148: libvirtd_sysv2upstart || return $?
Line 149:
Line 150: local force_reconfigure="$5"
this $5 is ugly here... better to parse the cmdline at main and set global
variables so that compare will be by name,.
Line 151: local by_vdsm="by vdsm"
Line 152: # The PACKAGE_VERSION macro is not used here because we do not
want to
Line 153: # update the libvirt configure file every time we change vdsm
package
Line 154: # version. In fact the configure generated here is almost
unrelated to the
Line 172:
Line 173: lconf="$1"
Line 174: qconf="$2"
Line 175: ldconf="$3"
Line 176: qlconf="$4"
please move parameter assignment to start of the function.
Line 177:
Line 178: # do not configure ovirt nodes before registration
Line 179: if isOvirt
Line 180: then
Line 176: qlconf="$4"
Line 177:
Line 178: # do not configure ovirt nodes before registration
Line 179: if isOvirt
Line 180: then
if isOvirt; then ... etc... all over this function.
Line 181: if [ ! -f /etc/pki/vdsm/certs/vdsmcert.pem ]
Line 182: then
Line 183: echo "$prog: Missing certificates, $prog not registered"
1>&2
Line 184: return 6
Line 201: return 0
Line 202: fi
Line 203:
Line 204: # Remove a previous configuration (if present)
Line 205: remove_vdsm_conf $lconf $qconf $ldconf $qlconf
quotes and "${}"
Line 206:
Line 207: # Write to all conf files the *initial* message of vdsm changes
Line 208: for arg in "${lconf}" "${qconf}" "${ldconf}" "${qlconf}"
Line 209: do
Line 216: set_if_default $lconf unix_sock_rw_perms \"0770\"
Line 217: set_if_default $lconf auth_unix_rw \"sasl\"
Line 218: set_if_default $lconf host_uuid \"$(uuidgen)\"
Line 219: set_if_default $lconf keepalive_interval -1
Line 220: set_if_default $qconf dynamic_ownership 0
quotes everything
Line 221:
Line 222: if [ "$ssl" = "true" ]; then
Line 223: set_if_default $qconf spice_tls 1
Line 224: else
--
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: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Bala.FA <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Giuseppe Vallarelli <[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
