Change in vdsm[master]: new hook: directlun
danken at redhat.com
danken at redhat.com
Thu Nov 10 18:50:30 UTC 2011
Dan Kenigsberg has posted comments on this change.
Change subject: new hook: directlun
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(10 inline comments)
....................................................
File vdsm_hooks/directlun/before_vm_start.py
Line 3: import os
Line 4: import sys
Line 5: import grp
Line 6: import pwd
Line 7: import utils
non-standard imports should come last
Line 8: import hooking
Line 9: import traceback
Line 10: from xml.dom import minidom
Line 11:
Line 6: import pwd
Line 7: import utils
Line 8: import hooking
Line 9: import traceback
Line 10: from xml.dom import minidom
imported but unused. Please run pyflakes on your code
Line 11:
Line 12: DEV_MAPPER_PATH = "/dev/mapper"
Line 13: DEV_DIRECTLUN_PATH = '/dev/directlun'
Line 14:
Line 45: disks = []
Line 46: for d in xmldisks:
Line 47: disks.append(d.getElementsByTagName('target')[0].getAttribute('dev'))
Line 48:
Line 49: for i in range(0, 27):
what's 27?
Line 50: if not indexToDiskName(i) in disks:
Line 51: target.setAttribute('dev', indexToDiskName(i))
Line 52: break
Line 53:
Line 68:
Line 69: def createDirectory(dirpath):
Line 70:
Line 71: # we don't use os.mkdir/chown because we need sudo
Line 72: command = ['/bin/mkdir', '-p', dirpath]
not a must, but could be nicer to use autoconf for the paths.
or importing constants
Line 73: retcode, out, err = utils.execCmd(command, sudo=True, raw=True)
Line 74: if retcode != 0:
Line 75: sys.stderr.write('directlun: error mkdir %s, err = %s\n' % (dirpath, err))
Line 76: sys.exit(2)
Line 75: sys.stderr.write('directlun: error mkdir %s, err = %s\n' % (dirpath, err))
Line 76: sys.exit(2)
Line 77:
Line 78: mode = '755'
Line 79: command = ['/bin/chmod', mode, dirpath]
this specific path is better replaced by a simple os.chmod() call.
Line 80: if retcode != 0:
Line 81: sys.stderr.write('directlun: error chmod %s %s, err = %s\n' % (dirpath, mode, err))
Line 82: sys.exit(2)
Line 83:
Line 113: user = pwd.getpwnam('qemu')
Line 114: uid = user.pw_uid
Line 115: owner = str(uid) + ':' + str(gid)
Line 116: command = ['/bin/chown', owner, devpath]
Line 117: retcode, out, err = utils.execCmd(command, sudo=True, raw=True)
wouldn't qemu group ownership be enough? that can be set without sudo (or fork)
Line 118: if retcode != 0:
Line 119: sys.stderr.write('directlun: error chown %s to %s, err = %s\n' % (devpath, owner, err))
Line 120: sys.exit(2)
Line 121:
....................................................
File vdsm_hooks/directlun/README
Line -3
Line -2
Line -1
Line 0
Line 1: Syntax:
mention that this is a custom_property?
Line 2: directlun: directlun=3600144f0e90c870000004d4194ed001f
Line 3: directlun=3600144f0e90c870000004d4194ed001f:readonly
Line 4: directlun=lun1[:option1[;option2]][,lun2[:option1[;option2]]]...
Line 5:
Line 7: will add disk device of specify lun id
Line 8: (lun id in client side, i.e. the one that multipath cretae as in this example 3xxxxxxx,
Line 9: 3 is Solaris prefix).
Line 10:
Line 11: there is also option sectoin (now only readonly):
sectoin->section
Line 12: directlun=lunid:readonly,lunid...
Line 13
Line 14
Line 15
....................................................
File vdsm_hooks/directlun/vdsm-hook-directlun.spec
Line 0
Line 1: %define directlun_name vdsm-hook-directlun
Line 2: %define vdsm_name vdsm
Line 3:
Line 4: Summary: Direct LUN support for Red-Hat VDSM
how should I build this rpm?
why not a vdsm subpackage just like vdsm-hook-vhostmd?
Line 5: Name: vdsm-hook-directlun
Line 6: Source: %{directlun_name}.tar.gz
Line 7: Version: 1.0
Line 8: Vendor: Red-Had
Line 4: Summary: Direct LUN support for Red-Hat VDSM
Line 5: Name: vdsm-hook-directlun
Line 6: Source: %{directlun_name}.tar.gz
Line 7: Version: 1.0
Line 8: Vendor: Red-Had
Had?
Line 9: Release: 1%{?dist}
Line 10: License: GPLv2+
Line 11: Group: Applications/System
Line 12: BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}
--
To view, visit http://gerrit.usersys.redhat.com/1101
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If72211ad12d8b50e9c09ad979b26489c2d4f5a74
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
More information about the vdsm-patches
mailing list