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