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