Change in vdsm[master]: Extend vdsm-tool: moving configure libvirt to external shell...

zhshzhou at linux.vnet.ibm.com zhshzhou at linux.vnet.ibm.com
Thu Jun 13 08:38:42 UTC 2013


Zhou Zheng Sheng has posted comments on this change.

Change subject: Extend vdsm-tool: moving configure libvirt to external shell script
......................................................................


Patch Set 10: I would prefer that you didn't submit this

(6 inline comments)

Some minor problems, very close to +1 ;-)

....................................................
File lib/vdsm/tool/libvirt_configure.py
Line 44: 
Line 45:     return 0
Line 46: 
Line 47: 
Line 48: def parseParams(args):
It seems what parseParams does is to verify/check/validate parameters, not parsing them, so a better name would be checkParams.
Line 49:     if len(args) == 0:
Line 50:         usageMsg = \
Line 51:             """
Line 52: Using default conf files:


....................................................
File lib/vdsm/tool/libvirt_configure.sh.in
Line 243:     set_if_default $qlconf auto_disk_leases 0
Line 244:     set_if_default $qlconf require_lease_for_disks 0
Line 245: 
Line 246:     # Configuring auto dump path
Line 247:     set_if_default $qconf auto_dump_path \"$QEMU_DUMP_PATH\"
QEMU_DUMP_PATH is not defined..
Line 248: 
Line 249:     # Write to all conf files the *end* message of vdsm changes
Line 250:     for arg in "${lconf}" "${qconf}" "${ldconf}" "${qlconf}"
Line 251:     do


Line 304:         ;;
Line 305:      test_conflict_configurations)
Line 306:         echo "Checking conflicts ..."
Line 307:         if [ "$#" = '2' ]; then
Line 308:             test_conflicting_conf "$2" "$3"
If "$#" equals to '2', then what's the value of "$3" here? Maybe the $# should equal to 3 to enter this block.
Line 309:         else
Line 310:             test_conflicting_conf "$LCONF" "$QCONF"
Line 311:         fi
Line 312:         RETVAL=$?


....................................................
File lib/vdsm/tool/Makefile.am
Line 34: 	__init__.py \
Line 35: 	dummybr.py \
Line 36: 	nwfilter.py \
Line 37: 	libvirt_configure.py \
Line 38: 	libvirt_configure.sh \
libvirt_configure should not be under a dist_xxx variable, because it is generated. It's not source, it's a build result. It should be like this.

nodist_vdsmtool_SCRIPTS = \
	libvirt_configure.sh \
	$(NULL)

and we add

CLEANFILES = \
	config.log \
	$(nodist_vdsmtool_DATA) \
	$(nodist_vdsmtool_SCRIPTS) \
	$(NULL)

In this way the libvirt_configure.sh is generated from .in when we "make", and it is installed to vdsm/tool with 0755 permission when we "make install", at last we can remove the 0755 attribute from the vdsm.spec.in because "make install" has already assigned the right permission bits to it. I think we should reply on vdsm.spec as little as possible, because it's distro specific mechanism.
Line 39: 	passwd.py \
Line 40: 	seboolsetup.py \
Line 41: 	service.py \
Line 42: 	vdsm-id.py \


....................................................
File vdsm.spec.in
Line 978: %endif
Line 979: %{python_sitearch}/%{vdsm_name}/tool/dummybr.py*
Line 980: %{python_sitearch}/%{vdsm_name}/tool/nwfilter.py*
Line 981: %{python_sitearch}/%{vdsm_name}/tool/libvirt_configure.py*
Line 982: %attr(0755, -, -) %{python_sitearch}/%{vdsm_name}/tool/libvirt_configure.sh*
If we write Makefile.am properly, attr 0755 is not needed here, and ".sh*" should be ".sh".
Line 983: %{python_sitearch}/%{vdsm_name}/tool/passwd.py*
Line 984: %{python_sitearch}/%{vdsm_name}/tool/seboolsetup.py*
Line 985: %{python_sitearch}/%{vdsm_name}/tool/service.py*
Line 986: %{python_sitearch}/%{vdsm_name}/tool/validate_ovirt_certs.py*


....................................................
File vdsm/vdsmd.init.in
Line 29: RESPAWNPIDFILE=@VDSMRUNDIR@/respawn.pid
Line 30: CORE_DUMP_PATH=/var/log/core/core.%p.%t.dump
Line 31: DOM_METADATA_BACKUP_DIR=/var/log/vdsm/backup
Line 32: CORE_PATTERN=/proc/sys/kernel/core_pattern
Line 33: QEMU_DUMP_PATH="/var/log/core"
QEMU_DUMP_PATH is not used in vdsmd.init.in, and it's used in libvirt_configure.sh.in, so it can be moved there.
Line 34: NEEDED_SERVICES="iscsid multipathd ntpd wdmd sanlock network libvirtd
Line 35:                  supervdsmd"
Line 36: CONFLICTING_SERVICES="libvirt-guests"
Line 37: VDSM_TOOL=@BINDIR@/vdsm-tool


-- 
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: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.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