Change in vdsm[master]: sriov: Make the hook safe for concurrency - vdsm-patches - Fedora mailing-lists

5 Apr 2013

Antoni Segura Puimedon has uploaded a new change for review.
Change subject: sriov: Make the hook safe for concurrency
......................................................................
sriov: Make the hook safe for concurrency
The current model uses a cache that is not locked and could
be overwritten by a second VM creation effectively preventing
the return of the chown to root:root.
This patch solves that by creating with exclusivity a file for
each virtual function that is attached (thus avoiding locking)
but having concurrency safety due to the OS guarantee that the
not more than a single processes can perform an open when the
flags O_CREAT | O_EXCL are set.
Change-Id: I2c6ad7f2ee53911312700396000e0aca07a917e9
Signed-off-by: Antoni S. Puimedon asegurap@redhat.com
---
M vdsm_hooks/sriov/after_vm_destroy.py
M vdsm_hooks/sriov/before_vm_start.py
2 files changed, 40 insertions(+), 46 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/41/13641/1

diff --git a/vdsm_hooks/sriov/after_vm_destroy.py b/vdsm_hooks/sriov/after_vm_destroy.py
index a5e25b5..e82dab0 100755
--- a/vdsm_hooks/sriov/after_vm_destroy.py
+++ b/vdsm_hooks/sriov/after_vm_destroy.py
@@ -6,15 +6,13 @@
import hooking
-SYS_NIC_PATH = '/sys/class/net/%s'
-VDSM_VAR_HOOKS_DIR = '/var/run/vdsm/hooks'
-SRIOV_CACHE_FILENAME = 'sriov.cache'
+VDSM_VAR_HOOKS_DIR = '/var/run/vdsm/hooks/sriov'
-def restoreDevicePermissions(addr, devpath):
+def restoreDevicePermissions(devpath):
     owner = 'root:root'
     for f in os.listdir(devpath):
-        if f.startswith('resource') or f == 'rom' or f == 'reset':
+        if f.startswith('resource') or f in ('rom', 'reset'):
             dev = os.path.join(devpath, f)
             command = ['/bin/chown', owner, dev]
             retcode, out, err = hooking.execCmd(command, sudo=True, raw=True)
@@ -26,24 +24,16 @@
 if 'sriov' in os.environ:
     try:
         lines = ''
-        nics = os.environ['sriov'].split(',')
-        path = VDSM_VAR_HOOKS_DIR + '/' + SRIOV_CACHE_FILENAME
-
-        if os.path.exists(path):
-            with open(path, 'r') as f:
-                for line in f:
-                    nicAddr = line.split('=')
-                    if nicAddr[0] in nics:
-                        restoreDevicePermissions(nicAddr[1],
-                                                 nicAddr[2].strip('\n'))
-                    else:
-                        lines += line
-
-            with open(path, 'w') as f:
-                f.writelines(lines)
-        else:
-            sys.stderr.write('sriov after_vm_destroy: cannot find sriov cache '
-                             'file %s\n' % path)
+        for nic in os.environ['sriov'].split(','):
+            vfFilePath = os.path.join(VDSM_VAR_HOOKS_DIR, nic)
+            if os.path.exists(vfFilePath):
+                with open(vfFilePath, 'r') as vfFile:
+                    restoreDevicePermissions(vfFile.read())
+                os.unlink(vfFilePath)
+            else:
+                sys.stderr.write('sriov after_vm_destroy: cannot find the '
+                                 'virtual function reservation file of %s'
+                                 'that should be at %s\n' % (nic, vfFilePath))
except:
         sys.stderr.write('sriov after_vm_destroy: [unexpected error]: %s\n' %
diff --git a/vdsm_hooks/sriov/before_vm_start.py b/vdsm_hooks/sriov/before_vm_start.py
index 42b79e9..c23797b 100755
--- a/vdsm_hooks/sriov/before_vm_start.py
+++ b/vdsm_hooks/sriov/before_vm_start.py
@@ -1,5 +1,6 @@
 #!/usr/bin/python
+import errno
 import os
 import sys
 import grp
@@ -11,8 +12,7 @@
 from vdsm import libvirtconnection
SYS_NIC_PATH = '/sys/class/net/%s'
-VDSM_VAR_HOOKS_DIR = '/var/run/vdsm/hooks'
-SRIOV_CACHE_FILENAME = 'sriov.cache'
+VDSM_VAR_HOOKS_DIR = '/var/run/vdsm/hooks/sriov'
'''
 sriov vdsm hook
@@ -93,32 +93,36 @@
     return 'pci_%s_%s_%s' % (tokens[0], tokens[1], tokens[2].replace('.', '_'))
-def writeSriovCache(name, addr, devpath):
+def writeVFReservationFile(nic, devpath):
     if not os.path.exists(VDSM_VAR_HOOKS_DIR):
         os.makedirs(VDSM_VAR_HOOKS_DIR)
-
-    f = open(VDSM_VAR_HOOKS_DIR + '/' + SRIOV_CACHE_FILENAME, 'a')
-    f.write(name + '=' + addr + '=' + devpath + '\n')
-    f.close()
+    try:
+        fd = os.open(os.path.join(VDSM_VAR_HOOKS_DIR, nic),
+                     os.O_WRONLY | os.O_CREAT | os.O_EXCL)
+        with os.fdopen(fd, 'w') as f:
+            f.write(devpath)
+    except OSError as e:
+        if e.errno == errno.EEXIST:
+            sys.stderr.write('sriov: Error. The device %s is already attached '
+                             'or in the process of attaching to a VM. Aborting'
+                             '.\n' % nic)
+        sys.stderr.write('sriov: Unexpected error creating virtual function '
+                         'reservation file for nic %s. Aborting.\n%s\n' %
+                         (nic, traceback.format_exc()))
+        sys.exit(2)
-def chown(devpath):
-    group = grp.getgrnam('qemu')
-    gid = group.gr_gid
-    user = pwd.getpwnam('qemu')
-    uid = user.pw_uid
-
+def chown(nic, devpath):
+    '''Uses sudo and chown to change the sriov ownership.'''
+    owner = ''.join([str(pwd.getpwnam('qemu').pw_uid), ':',
+                    str(grp.getgrnam('qemu').gr_gid)])
     for f in os.listdir(devpath):
-        if f.startswith('resource') or f == 'rom' or f == 'reset':
-            dev = os.path.join(devpath, f)
-
-            # we don't use os.chown because we need sudo
-            owner = str(uid) + ':' + str(gid)
-            command = ['/bin/chown', owner, dev]
+        if f.startswith('resource') or f in ('rom', 'reset'):
+            command = ['/bin/chown', owner, os.path.join(devpath, f)]
             retcode, out, err = hooking.execCmd(command, sudo=True, raw=True)
             if retcode != 0:
-                sys.stderr.write('sriov: error chown %s to %s, err = %s\n' %
-                                 (dev, owner, err))
+                sys.stderr.write('sriov: Error %s changing ownership of %s to'
+                                 'owner %s. Aborting.\n' % (err, nic, owner))
                 sys.exit(2)
@@ -141,9 +145,9 @@
sys.stderr.write('sriov: VF %s xml: %s\n' %
                                  (nic, interface.toxml()))
-                chown(devpath)
+                writeVFReservationFile(nic, devpath)
+                chown(nic, devpath)
                 devices.appendChild(interface)
-                writeSriovCache(nic, addr, devpath)
             else:
                 sys.stderr.write('sriov: cannot find nic "%s", aborting\n' %
                                  nic)
--
To view, visit http://gerrit.ovirt.org/13641
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2c6ad7f2ee53911312700396000e0aca07a917e9
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com

    