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

[Arfrever Frehtes Taifersar Arahesis]

2013-10-21 05:07 Mike Frysinger napisał(a):
> 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

But portage.util._xattr._XattrSystemCommands does not work :) .

>>> import portage.util._xattr
>>> portage.util._xattr._XattrSystemCommands.list("/tmp")
...

See below.

>  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'))

It can fail with UnicodeEncodeError.
Use proc.stdin.write(portage._unicode_encode(stdin), portage._encodings['stdio']) instead of above 4 lines.

> +	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()

AttributeError: 'NoneType' object has no attribute 'close'

If closing stdin is necessary, then I suggest:

if proc.stdin is not None:
	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']):
>

--
Arfrever Frehtes Taifersar Arahesis