Change in vdsm[master]: vdsm_hooks: Add support for checkimages VDSM hook

danken at redhat.com danken at redhat.com
Sun Nov 11 20:47:45 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: vdsm_hooks: Add support for checkimages VDSM hook
......................................................................


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

(8 inline comments)

thanks, few comments within the code.

....................................................
File configure.ac
Line 191: 	tests/Makefile
Line 192: 	tests/functional/Makefile
Line 193: 	vds_bootstrap/Makefile
Line 194: 	vdsm_cli/Makefile
Line 195:         vdsm_hooks/checkimages/Makefile
all other lines are tabs.
Line 196: 	vdsm_hooks/directlun/Makefile
Line 197: 	vdsm_hooks/faqemu/Makefile
Line 198: 	vdsm_hooks/fileinject/Makefile
Line 199: 	vdsm_hooks/floppy/Makefile


....................................................
File vdsm_hooks/checkimages/before_vm_start.py
Line 68:         try:
Line 69:             req_timeout = checkimages.split(',')[1].split(':')[1]
Line 70:             if req_timeout != 'none':
Line 71:                 timeout = float(req_timeout)
Line 72:         except:
catch-all "except" is a bad practice. I'd rather have the hook fail on an invalid input over running happily after ignoring it.
Line 73:             sys.stderr.write('checkimages: Error obtaining timeout value. ' \
Line 74:                              'Continuing with no timeout.\n')
Line 75: 
Line 76:         domxml = hooking.read_domxml()


Line 69:             req_timeout = checkimages.split(',')[1].split(':')[1]
Line 70:             if req_timeout != 'none':
Line 71:                 timeout = float(req_timeout)
Line 72:         except:
Line 73:             sys.stderr.write('checkimages: Error obtaining timeout value. ' \
redundant trailing backslash, here and elsewhere.
Line 74:                              'Continuing with no timeout.\n')
Line 75: 
Line 76:         domxml = hooking.read_domxml()
Line 77:         disks = domxml.getElementsByTagName('disk')


Line 80:             sources = disk.getElementsByTagName('source')
Line 81:             drivers = disk.getElementsByTagName('driver')
Line 82:             
Line 83:             if (len(sources) > 0 and len(drivers) > 0):
Line 84:                 disk_path = sources[0].getAttribute('file')
we need to check "block" based disks, too.
Line 85:                 disk_type = drivers[0].getAttribute('type')
Line 86: 
Line 87:                 if ((disk_path and disk_type) and (disk_type == 'qcow2')):
Line 88:                     sys.stderr.write('checkimages: Checking disk = %s\n' %


Line 83:             if (len(sources) > 0 and len(drivers) > 0):
Line 84:                 disk_path = sources[0].getAttribute('file')
Line 85:                 disk_type = drivers[0].getAttribute('type')
Line 86: 
Line 87:                 if ((disk_path and disk_type) and (disk_type == 'qcow2')):
conditions seems a bit too complex. disk_type cannot be empty and 'qcow2'.
Line 88:                     sys.stderr.write('checkimages: Checking disk = %s\n' %
Line 89:                                      disk_path)
Line 90:                     checkImage(disk_path, timeout)
Line 91:                 elif disk_type != 'qcow2':


....................................................
File vdsm_hooks/checkimages/Makefile.am
Line 15: # along with this program; if not, write to the Free Software
Line 16: # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
Line 17: # 
Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: # 
trailing whitespace
Line 20: #
Line 21: 
Line 22: EXTRA_DIST = \
Line 23:         before_vm_start.py


Line 24: 
Line 25: install-data-local:
Line 26: 	$(MKDIR_P) $(DESTDIR)$(vdsmhooksdir)/before_vm_start
Line 27: 	$(INSTALL_SCRIPT) $(srcdir)/before_vm_start.py \
Line 28:         	$(DESTDIR)$(vdsmhooksdir)/before_vm_start/60_checkimages
another whitespace noise.
Line 29: 
Line 30: uninstall-local:


....................................................
File vdsm_hooks/Makefile.am
Line 24: 
Line 25: # Additional hooks
Line 26: if HOOKS
Line 27: SUBDIRS += \
Line 28:         checkimages \
tab
Line 29: 	directlun \
Line 30: 	fileinject \
Line 31: 	floppy \
Line 32: 	hostusb \


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4d88fb46a725d2c525d10ce2fcc808bba5b0ab4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Jan Lieskovsky <jlieskov at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>


More information about the vdsm-patches mailing list