[gentoo-portage-dev] [PATCH v2] xattr: centralize the various shims in one place

[Mike Frysinger]

Rather than each module implementing its own shim around the various
methods for accessing extended attributes, start a dedicated module
that exports a consistent API.
---
v2
	- passes unittests w/python 2.6 2.7 3.2 3.3

 bin/xattr-helper.py                  |  11 +-
 pym/portage/tests/util/test_xattr.py | 178 ++++++++++++++++++++++++++++++
 pym/portage/util/_xattr.py           | 205 +++++++++++++++++++++++++++++++++++
 pym/portage/util/movefile.py         | 100 ++++-------------
 4 files changed, 407 insertions(+), 87 deletions(-)
 create mode 100644 pym/portage/tests/util/test_xattr.py
 create mode 100644 pym/portage/util/_xattr.py

diff --git a/bin/xattr-helper.py b/bin/xattr-helper.py
index 6d99521..69b83f7 100755
--- a/bin/xattr-helper.py
+++ b/bin/xattr-helper.py
@@ -17,16 +17,7 @@ import re
 import sys
 
 from portage.util._argparse import ArgumentParser
-
-if hasattr(os, "getxattr"):
-
-	class xattr(object):
-		get = os.getxattr
-		set = os.setxattr
-		list = os.listxattr
-
-else:
-	import xattr
+from portage.util._xattr import xattr
 
 
 _UNQUOTE_RE = re.compile(br'\\[0-7]{3}')
diff --git a/pym/portage/tests/util/test_xattr.py b/pym/portage/tests/util/test_xattr.py
new file mode 100644
index 0000000..e1f6ee8
--- /dev/null
+++ b/pym/portage/tests/util/test_xattr.py
@@ -0,0 +1,178 @@
+# Copyright 2010-2013 Gentoo Foundation
+# Distributed under the terms of the GNU General Public License v2
+
+"""Tests for the portage.util._xattr module"""
+
+from __future__ import print_function
+
+try:
+	# Try python-3.3 module first.
+	from unittest import mock
+except ImportError:
+	try:
+		# Try standalone module.
+		import mock
+	except ImportError:
+		mock = None
+
+import subprocess
+
+from portage.tests import TestCase
+from portage.util._xattr import (xattr as _xattr, _XattrSystemCommands,
+                                 _XattrStub)
+
+
+orig_popen = subprocess.Popen
+def MockSubprocessPopen(stdin):
+	"""Helper to mock (closely) a subprocess.Popen call
+
+	The module has minor tweaks in behavior when it comes to encoding and
+	python versions, so use a real subprocess.Popen call to fake out the
+	runtime behavior.  This way we don't have to also implement different
+	encodings as that gets ugly real fast.
+	"""
+	proc = orig_popen(['cat'], stdout=subprocess.PIPE, stdin=subprocess.PIPE)
+	try:
+		proc.stdin.write(bytes(stdin))
+	except TypeError:
+		proc.stdin.write(bytes(stdin, 'ascii'))
+	return proc
+
+
+class SystemCommandsTest(TestCase):
+	"""Test _XattrSystemCommands"""
+
+	OUTPUT = '\n'.join([
+		'# file: /bin/ping',
+		'security.capability=0sAQAAAgAgAAAAAAAAAAAAAAAAAAA=',
+		'user.foo="asdf"',
+		'',
+	])
+
+	def _setUp(self):
+		if mock is None:
+			self.skipTest('need mock for testing')
+
+		return _XattrSystemCommands
+
+	def _testGetBasic(self):
+		"""Verify the get() behavior"""
+		xattr = self._setUp()
+		with mock.patch.object(subprocess, 'Popen') as call_mock:
+			# Verify basic behavior, and namespace arg works as expected.
+			xattr.get('/some/file', 'user.foo')
+			xattr.get('/some/file', 'foo', namespace='user')
+			self.assertEqual(call_mock.call_args_list[0], call_mock.call_args_list[1])
+
+			# Verify nofollow behavior.
+			call_mock.reset()
+			xattr.get('/some/file', 'user.foo', nofollow=True)
+			self.assertIn('-h', call_mock.call_args[0][0])
+
+	def testGetParsing(self):
+		"""Verify get() parses output sanely"""
+		xattr = self._setUp()
+		with mock.patch.object(subprocess, 'Popen') as call_mock:
+			# Verify output parsing.
+			call_mock.return_value = MockSubprocessPopen('\n'.join([
+				'# file: /some/file',
+				'user.foo="asdf"',
+				'',
+			]))
+			call_mock.reset()
+			self.assertEqual(xattr.get('/some/file', 'user.foo'), b'"asdf"')
+
+	def testGetAllBasic(self):
+		"""Verify the get_all() behavior"""
+		xattr = self._setUp()
+		with mock.patch.object(subprocess, 'Popen') as call_mock:
+			# Verify basic behavior.
+			xattr.get_all('/some/file')
+
+			# Verify nofollow behavior.
+			call_mock.reset()
+			xattr.get_all('/some/file', nofollow=True)
+			self.assertIn('-h', call_mock.call_args[0][0])
+
+	def testGetAllParsing(self):
+		"""Verify get_all() parses output sanely"""
+		xattr = self._setUp()
+		with mock.patch.object(subprocess, 'Popen') as call_mock:
+			# Verify output parsing.
+			call_mock.return_value = MockSubprocessPopen(self.OUTPUT)
+			exp = [
+				(b'security.capability', b'0sAQAAAgAgAAAAAAAAAAAAAAAAAAA='),
+				(b'user.foo', b'"asdf"'),
+			]
+			self.assertEqual(exp, xattr.get_all('/some/file'))
+
+	def testSetBasic(self):
+		"""Verify the set() behavior"""
+		xattr = self._setUp()
+		with mock.patch.object(subprocess, 'Popen') as call_mock:
+			# Verify basic behavior, and namespace arg works as expected.
+			xattr.set('/some/file', 'user.foo', 'bar')
+			xattr.set('/some/file', 'foo', 'bar', namespace='user')
+			self.assertEqual(call_mock.call_args_list[0], call_mock.call_args_list[1])
+
+	def testListBasic(self):
+		"""Verify the list() behavior"""
+		xattr = self._setUp()
+		with mock.patch.object(subprocess, 'Popen') as call_mock:
+			# Verify basic behavior.
+			xattr.list('/some/file')
+
+			# Verify nofollow behavior.
+			call_mock.reset()
+			xattr.list('/some/file', nofollow=True)
+			self.assertIn('-h', call_mock.call_args[0][0])
+
+	def testListParsing(self):
+		"""Verify list() parses output sanely"""
+		xattr = self._setUp()
+		with mock.patch.object(subprocess, 'Popen') as call_mock:
+			# Verify output parsing.
+			call_mock.return_value = MockSubprocessPopen(self.OUTPUT)
+			exp = [b'security.capability', b'user.foo']
+			self.assertEqual(exp, xattr.list('/some/file'))
+
+	def testRemoveBasic(self):
+		"""Verify the remove() behavior"""
+		xattr = self._setUp()
+		with mock.patch.object(subprocess, 'Popen') as call_mock:
+			# Verify basic behavior, and namespace arg works as expected.
+			xattr.remove('/some/file', 'user.foo')
+			xattr.remove('/some/file', 'foo', namespace='user')
+			self.assertEqual(call_mock.call_args_list[0], call_mock.call_args_list[1])
+
+			# Verify nofollow behavior.
+			call_mock.reset()
+			xattr.remove('/some/file', 'user.foo', nofollow=True)
+			self.assertIn('-h', call_mock.call_args[0][0])
+
+
+class StubTest(TestCase):
+	"""Test _XattrStub"""
+
+	def testBasic(self):
+		"""Verify the stub is stubby"""
+		# Would be nice to verify raised errno is OperationNotSupported.
+		self.assertRaises(OSError, _XattrStub.get, '/', '')
+		self.assertRaises(OSError, _XattrStub.set, '/', '', '')
+		self.assertRaises(OSError, _XattrStub.get_all, '/')
+		self.assertRaises(OSError, _XattrStub.remove, '/', '')
+		self.assertRaises(OSError, _XattrStub.list, '/')
+
+
+class StandardTest(TestCase):
+	"""Test basic xattr API"""
+
+	MODULES = (_xattr, _XattrSystemCommands, _XattrStub)
+	FUNCS = ('get', 'get_all', 'set', 'remove', 'list')
+
+	def testApi(self):
+		"""Make sure the exported API matches"""
+		for mod in self.MODULES:
+			for f in self.FUNCS:
+				self.assertTrue(hasattr(mod, f),
+					'%s func missing in %s' % (f, mod))
diff --git a/pym/portage/util/_xattr.py b/pym/portage/util/_xattr.py
new file mode 100644
index 0000000..db3cf6e
--- /dev/null
+++ b/pym/portage/util/_xattr.py
@@ -0,0 +1,205 @@
+# Copyright 2010-2013 Gentoo Foundation
+# Distributed under the terms of the GNU General Public License v2
+
+"""Portability shim for xattr support
+
+Exported API is the xattr object with get/get_all/set/remove/list operations.
+
+See the standard xattr module for more documentation.
+"""
+
+from __future__ import print_function
+
+import contextlib
+import os
+import subprocess
+
+from portage.exception import OperationNotSupported
+
+
+class _XattrGetAll(object):
+	"""Implement get_all() using list()/get() if there is no easy bulk method"""
+
+	@classmethod
+	def get_all(cls, item, nofollow=False, namespace=None):
+		return [(name, cls.get(item, name, nofollow=nofollow, namespace=namespace))
+		        for name in cls.list(item, nofollow=nofollow, namespace=namespace)]
+
+
+class _XattrSystemCommands(_XattrGetAll):
+	"""Implement things with getfattr/setfattr"""
+
+	@staticmethod
+	def _parse_output(output):
+		for line in output.readlines():
+			if line.startswith(b'#'):
+				continue
+			line = line.rstrip()
+			if not line:
+				continue
+			# The lines will have the format:
+			#	user.hex=0x12345
+			#	user.base64=0sAQAAAgAgAAAAAAAAAAAAAAAAAAA=
+			#	user.string="value0"
+			# But since we don't do interpretation on the value (we just
+			# save & restore it), don't bother with decoding here.
+			yield line.split(b'=', 1)
+
+	@staticmethod
+	def _call(*args, **kwargs):
+		proc = subprocess.Popen(*args, **kwargs)
+		proc.stdin.close()
+		proc.wait()
+		return proc
+
+	@classmethod
+	def get(cls, item, name, nofollow=False, namespace=None):
+		if namespace:
+			name = '%s.%s' % (namespace, name)
+		cmd = ['getfattr', '--absolute-names', '-n', name, item]
+		if nofollow:
+			cmd += ['-h']
+		proc = cls._call(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+
+		value = None
+		for _, value in cls._parse_output(proc.stdout):
+			break
+
+		proc.stdout.close()
+		return value
+
+	@classmethod
+	def set(cls, item, name, value, _flags=0, namespace=None):
+		if namespace:
+			name = '%s.%s' % (namespace, name)
+		cmd = ['setfattr', '-n', name, '-v', value, item]
+		cls._call(cmd)
+
+	@classmethod
+	def remove(cls, item, name, nofollow=False, namespace=None):
+		if namespace:
+			name = '%s.%s' % (namespace, name)
+		cmd = ['setfattr', '-x', name, item]
+		if nofollow:
+			cmd += ['-h']
+		cls._call(cmd)
+
+	@classmethod
+	def list(cls, item, nofollow=False, namespace=None, _names_only=True):
+		cmd = ['getfattr', '-d', '--absolute-names', item]
+		if nofollow:
+			cmd += ['-h']
+		cmd += ['-m', ('^%s[.]' % namespace) if namespace else '']
+		proc = cls._call(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+
+		ret = []
+		if namespace:
+			namespace = '%s.' % namespace
+		for name, value in cls._parse_output(proc.stdout):
+			if namespace:
+				if name.startswith(namespace):
+					name = name[len(namespace):]
+				else:
+					continue
+			if _names_only:
+				ret.append(name)
+			else:
+				ret.append((name, value))
+
+		proc.stdout.close()
+		return ret
+
+	@classmethod
+	def get_all(cls, item, nofollow=False, namespace=None):
+		return cls.list(item, nofollow=nofollow, namespace=namespace,
+		                _names_only=False)
+
+
+# pylint: disable=W0613
+class _XattrStub(_XattrGetAll):
+	"""Fake object since system doesn't support xattrs"""
+
+	@staticmethod
+	def _raise():
+		e = OSError('stub')
+		e.errno = OperationNotSupported.errno
+		raise e
+
+	@classmethod
+	def get(cls, item, name, nofollow=False, namespace=None):
+		cls._raise()
+
+	@classmethod
+	def set(cls, item, name, value, flags=0, namespace=None):
+		cls._raise()
+
+	@classmethod
+	def remove(cls, item, name, nofollow=False, namespace=None):
+		cls._raise()
+
+	@classmethod
+	def list(cls, item, nofollow=False, namespace=None):
+		cls._raise()
+
+
+if hasattr(os, 'getxattr'):
+	# Easy as pie -- active python supports it.
+	class xattr(_XattrGetAll):
+		"""Python >=3.3 and GNU/Linux"""
+		get = os.getxattr
+		set = os.setxattr
+		remove = os.removexattr
+		list = os.listxattr
+
+else:
+	try:
+		# Maybe we have the xattr module.
+		import xattr
+
+	except ImportError:
+		try:
+			# Maybe we have the attr package.
+			with open(os.devnull, 'wb') as f:
+				subprocess.call(['getfattr', '--version'], stdout=f)
+				subprocess.call(['setfattr', '--version'], stdout=f)
+			xattr = _XattrSystemCommands
+
+		except OSError:
+			# Stub it out completely.
+			xattr = _XattrStub
+
+
+@××××××××××.contextmanager
+def preserve_xattrs(path, nofollow=False, namespace=None):
+	"""Context manager to save/restore extended attributes on |path|
+
+	If you want to rewrite a file (possibly replacing it with a new one), but
+	want to preserve the extended attributes, this will do the trick.
+
+	# First read all the extended attributes.
+	with save_xattrs('/some/file'):
+		... rewrite the file ...
+	# Now the extended attributes are restored as needed.
+	"""
+	kwargs = {'nofollow': nofollow,}
+	if namespace:
+		# Compiled xattr python module does not like it when namespace=None.
+		kwargs['namespace'] = namespace
+
+	old_attrs = dict(xattr.get_all(path, **kwargs))
+	try:
+		yield
+	finally:
+		new_attrs = dict(xattr.get_all(path, **kwargs))
+		for name, value in new_attrs.iteritems():
+			if name not in old_attrs:
+				# Clear out new ones.
+				xattr.remove(path, name, **kwargs)
+			elif new_attrs[name] != old_attrs[name]:
+				# Update changed ones.
+				xattr.set(path, name, value, **kwargs)
+
+		for name, value in old_attrs.iteritems():
+			if name not in new_attrs:
+				# Re-add missing ones.
+				xattr.set(path, name, value, **kwargs)
diff --git a/pym/portage/util/movefile.py b/pym/portage/util/movefile.py
index 452e77f..20859fe 100644
--- a/pym/portage/util/movefile.py
+++ b/pym/portage/util/movefile.py
@@ -11,7 +11,6 @@ import os as _os
 import shutil as _shutil
 import stat
 import sys
-import subprocess
 import textwrap
 
 import portage
@@ -23,6 +22,7 @@ from portage.exception import OperationNotSupported
 from portage.localization import _
 from portage.process import spawn
 from portage.util import writemsg
+from portage.util._xattr import xattr
 
 def _apply_stat(src_stat, dest):
 	_os.chown(dest, src_stat.st_uid, src_stat.st_gid)
@@ -68,86 +68,32 @@ class _xattr_excluder(object):
 
 		return False
 
-if hasattr(_os, "getxattr"):
-	# Python >=3.3 and GNU/Linux
-	def _copyxattr(src, dest, exclude=None):
-
-		try:
-			attrs = _os.listxattr(src)
-		except OSError as e:
-			if e.errno != OperationNotSupported.errno:
-				raise
-			attrs = ()
-		if attrs:
-			if exclude is not None and isinstance(attrs[0], bytes):
-				exclude = exclude.encode(_encodings['fs'])
-			exclude = _get_xattr_excluder(exclude)
-
-		for attr in attrs:
-			if exclude(attr):
-				continue
-			try:
-				_os.setxattr(dest, attr, _os.getxattr(src, attr))
-				raise_exception = False
-			except OSError:
-				raise_exception = True
-			if raise_exception:
-				raise OperationNotSupported(_("Filesystem containing file '%s' "
-					"does not support extended attribute '%s'") %
-					(_unicode_decode(dest), _unicode_decode(attr)))
-else:
+def _copyxattr(src, dest, exclude=None):
+	"""Copy the extended attributes from |src| to |dest|"""
 	try:
-		import xattr
-	except ImportError:
-		xattr = None
-	if xattr is not None:
-		def _copyxattr(src, dest, exclude=None):
-
-			try:
-				attrs = xattr.list(src)
-			except IOError as e:
-				if e.errno != OperationNotSupported.errno:
-					raise
-				attrs = ()
+		attrs = xattr.list(src)
+	except (OSError, IOError) as e:
+		if e.errno != OperationNotSupported.errno:
+			raise
+		attrs = ()
 
-			if attrs:
-				if exclude is not None and isinstance(attrs[0], bytes):
-					exclude = exclude.encode(_encodings['fs'])
-				exclude = _get_xattr_excluder(exclude)
+	if attrs:
+		if exclude is not None and isinstance(attrs[0], bytes):
+			exclude = exclude.encode(_encodings['fs'])
+		exclude = _get_xattr_excluder(exclude)
 
-			for attr in attrs:
-				if exclude(attr):
-					continue
-				try:
-					xattr.set(dest, attr, xattr.get(src, attr))
-					raise_exception = False
-				except IOError:
-					raise_exception = True
-				if raise_exception:
-					raise OperationNotSupported(_("Filesystem containing file '%s' "
-						"does not support extended attribute '%s'") %
-						(_unicode_decode(dest), _unicode_decode(attr)))
-	else:
+	for attr in attrs:
+		if exclude(attr):
+			continue
 		try:
-			with open(_os.devnull, 'wb') as f:
-				subprocess.call(["getfattr", "--version"], stdout=f)
-				subprocess.call(["setfattr", "--version"], stdout=f)
-		except OSError:
-			def _copyxattr(src, dest, exclude=None):
-				# TODO: implement exclude
-				getfattr_process = subprocess.Popen(["getfattr", "-d", "--absolute-names", src], stdout=subprocess.PIPE)
-				getfattr_process.wait()
-				extended_attributes = getfattr_process.stdout.readlines()
-				getfattr_process.stdout.close()
-				if extended_attributes:
-					extended_attributes[0] = b"# file: " + _unicode_encode(dest) + b"\n"
-					setfattr_process = subprocess.Popen(["setfattr", "--restore=-"], stdin=subprocess.PIPE, stderr=subprocess.PIPE)
-					setfattr_process.communicate(input=b"".join(extended_attributes))
-					if setfattr_process.returncode != 0:
-						raise OperationNotSupported("Filesystem containing file '%s' does not support extended attributes" % dest)
-		else:
-			def _copyxattr(src, dest, exclude=None):
-				pass
+			xattr.set(dest, attr, xattr.get(src, attr))
+			raise_exception = False
+		except (OSError, IOError):
+			raise_exception = True
+		if raise_exception:
+			raise OperationNotSupported(_("Filesystem containing file '%s' "
+				"does not support extended attribute '%s'") %
+				(_unicode_decode(dest), _unicode_decode(attr)))
 
 def movefile(src, dest, newmtime=None, sstat=None, mysettings=None,
 		hardlink_candidates=None, encoding=_encodings['fs']):
-- 
1.8.3.2