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

[gentoo-portage-dev] [PATCH] 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.
---
 bin/xattr-helper.py          |  11 +--
 pym/portage/util/_xattr.py   | 189 +++++++++++++++++++++++++++++++++++++++++++
 pym/portage/util/movefile.py |  99 ++++++-----------------
 3 files changed, 213 insertions(+), 86 deletions(-)
 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/util/_xattr.py b/pym/portage/util/_xattr.py
new file mode 100644
index 0000000..0e594f9
--- /dev/null
+++ b/pym/portage/util/_xattr.py
@@ -0,0 +1,189 @@
+# 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.
+"""
+
+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 proc.stdout.readlines():
+			if line.startswith('#'):
+				continue
+			line = line.rstrip()
+			if not line:
+				continue
+			# The line will have the format:
+			# user.name0="value0"
+			yield line.split('=', 1)
+
+	@classmethod
+	def get(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 = subprocess.call(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+		proc.wait()
+
+		value = None
+		for _, value in cls._parse_output(proc.stdout):
+			break
+
+		proc.stdout.close()
+		return value
+
+	@staticmethod
+	def set(item, name, value, flags=0, namespace=None):
+		if namespace:
+			name = '%s.%s' % (namespace, name)
+		cmd = ['setfattr', '-n', name, '-v', value, item]
+		subprocess.call(cmd)
+
+	@staticmethod
+	def remove(item, name, nofollow=False, namespace=None):
+		if namespace:
+			name = '%s.%s' % (namespace, name)
+		cmd = ['setfattr', '-x', name, item]
+		subprocess.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 = subprocess.call(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+		proc.wait()
+
+		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):
+		cls.list(item, nofollow=nofollow, namespace=namespace, _names_only=False)
+
+
+class _XattrStub(_XattrGetAll):
+	"""Fake object since system doesn't support xattrs"""
+
+	@staticmethod
+	def _raise():
+		e = OSError('stub')
+		e.errno = OperationNotSupported.errno
+		raise e
+
+	@staticmethod
+	def get(item, name, nofollow=False, namespace=None):
+		raise OperationNotSupported('stub')
+
+	@staticmethod
+	def set(item, name, value, flags=0, namespace=None):
+		raise OperationNotSupported('stub')
+
+	@staticmethod
+	def remove(item, name, nofollow=False, namespace=None):
+		raise OperationNotSupported('stub')
+
+	@staticmethod
+	def list(item, nofollow=False, namespace=None):
+		raise OperationNotSupported('stub')
+
+
+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
+
+
+@contextlib.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,
+		'namespace': namespace,
+	}
+	old_attrs = dict(xattr.get_all(path, **kwargs))
+	try:
+		yield
+	finally:
+		new_attrs = dict(xattrs.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:
+				# Update changed ones.
+				xattr.set(path, name, value, **kwargs)
+
+		for name, value in old_attrs.iteritems():
+			if name not in new_attr:
+				# 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 4f158cd..553374a 100644
--- a/pym/portage/util/movefile.py
+++ b/pym/portage/util/movefile.py
@@ -23,6 +23,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 +69,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) as e:
+			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

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

[Arfrever Frehtes Taifersar Arahesis]

[-- Attachment #1: Type: text/plain, Size: 11861 bytes --]

2013-10-16 23:03 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.

Some things are incompatible with Python 3.
See other comments below.

> ---
>  bin/xattr-helper.py          |  11 +--
>  pym/portage/util/_xattr.py   | 189 +++++++++++++++++++++++++++++++++++++++++++
>  pym/portage/util/movefile.py |  99 ++++++-----------------
>  3 files changed, 213 insertions(+), 86 deletions(-)
>  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/util/_xattr.py b/pym/portage/util/_xattr.py
> new file mode 100644
> index 0000000..0e594f9
> --- /dev/null
> +++ b/pym/portage/util/_xattr.py
> @@ -0,0 +1,189 @@
> +# 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.
> +"""
> +
> +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 proc.stdout.readlines():

NameError: global name 'proc' is not defined

> +			if line.startswith('#'):
> +				continue
> +			line = line.rstrip()
> +			if not line:
> +				continue
> +			# The line will have the format:
> +			# user.name0="value0"
> +			yield line.split('=', 1)
> +
> +	@classmethod
> +	def get(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 = subprocess.call(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> +		proc.wait()

AttributeError: 'int' object has no attribute 'wait'

> +
> +		value = None
> +		for _, value in cls._parse_output(proc.stdout):
> +			break
> +
> +		proc.stdout.close()
> +		return value
> +
> +	@staticmethod
> +	def set(item, name, value, flags=0, namespace=None):
> +		if namespace:
> +			name = '%s.%s' % (namespace, name)
> +		cmd = ['setfattr', '-n', name, '-v', value, item]

Calling setfattr once per attribute is slower than once per file (as is now).

> +		subprocess.call(cmd)
> +
> +	@staticmethod
> +	def remove(item, name, nofollow=False, namespace=None):
> +		if namespace:
> +			name = '%s.%s' % (namespace, name)
> +		cmd = ['setfattr', '-x', name, item]
> +		subprocess.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 = subprocess.call(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> +		proc.wait()

AttributeError: 'int' object has no attribute 'wait'

> +
> +		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):
> +		cls.list(item, nofollow=nofollow, namespace=namespace, _names_only=False)
> +
> +
> +class _XattrStub(_XattrGetAll):
> +	"""Fake object since system doesn't support xattrs"""
> +
> +	@staticmethod
> +	def _raise():
> +		e = OSError('stub')
> +		e.errno = OperationNotSupported.errno
> +		raise e
> +
> +	@staticmethod
> +	def get(item, name, nofollow=False, namespace=None):
> +		raise OperationNotSupported('stub')
> +
> +	@staticmethod
> +	def set(item, name, value, flags=0, namespace=None):
> +		raise OperationNotSupported('stub')
> +
> +	@staticmethod
> +	def remove(item, name, nofollow=False, namespace=None):
> +		raise OperationNotSupported('stub')
> +
> +	@staticmethod
> +	def list(item, nofollow=False, namespace=None):
> +		raise OperationNotSupported('stub')
> +
> +
> +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
> +
> +
> +@contextlib.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,
> +		'namespace': namespace,
> +	}
> +	old_attrs = dict(xattr.get_all(path, **kwargs))
> +	try:
> +		yield
> +	finally:
> +		new_attrs = dict(xattrs.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:
> +				# Update changed ones.
> +				xattr.set(path, name, value, **kwargs)
> +
> +		for name, value in old_attrs.iteritems():
> +			if name not in new_attr:
> +				# 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 4f158cd..553374a 100644
> --- a/pym/portage/util/movefile.py
> +++ b/pym/portage/util/movefile.py
> @@ -23,6 +23,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 +69,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) as e:

Unused variable e

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1097 bytes --]

On Wednesday 16 October 2013 20:02:50 Arfrever Frehtes Taifersar Arahesis 
wrote:
> 2013-10-16 23:03 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.
> 
> Some things are incompatible with Python 3.
> See other comments below.

i can run a linter on the code (probably should make this a git hook).  i'm 
interested more in review on the bigger picture.

also, please snip context you aren't actually replying to.  it makes it much 
harder to read replies when you don't do that.

> > +	@staticmethod
> > +	def set(item, name, value, flags=0, namespace=None):
> > +		if namespace:
> > +			name = '%s.%s' % (namespace, name)
> > +		cmd = ['setfattr', '-n', name, '-v', value, item]
> 
> Calling setfattr once per attribute is slower than once per file (as is
> now).

true, however:
	- setfattr is the fallback logic, not the primary code path
	- the # of attributes per file is low (rarely more than 2)
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 677 bytes --]

On Wednesday 16 October 2013 22:51:17 Mike Frysinger wrote:
> On Wednesday 16 October 2013 20:02:50 Arfrever Frehtes Taifersar Arahesis
> 
> wrote:
> > 2013-10-16 23:03 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.
> > 
> > Some things are incompatible with Python 3.
> > See other comments below.
> 
> i can run a linter on the code (probably should make this a git hook).  i'm
> interested more in review on the bigger picture.

also, none of your comments were py3 issues that i saw
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Arfrever Frehtes Taifersar Arahesis]

[-- Attachment #1: Type: Text/Plain, Size: 1413 bytes --]

2013-10-17 04:53 Mike Frysinger napisał(a):
> On Wednesday 16 October 2013 22:51:17 Mike Frysinger wrote:
> > On Wednesday 16 October 2013 20:02:50 Arfrever Frehtes Taifersar Arahesis
> > 
> > wrote:
> > > 2013-10-16 23:03 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.
> > > 
> > > Some things are incompatible with Python 3.
> > > See other comments below.
> > 
> > i can run a linter on the code (probably should make this a git hook).  i'm
> > interested more in review on the bigger picture.
> 
> also, none of your comments were py3 issues that i saw

I said "other comments", so I meant comments not related to incompatibility with Python 3.

About incompatibility with Python 3:
- subprocess.check_output(), subprocess.Popen().stdout.read(), subprocess.Popen().stderr.read() return
  bytes, which is incorrectly compared with str in your patches.
- dict.iteritems() was renamed to dict.items() (and its return type was changed from dictionary-itemiterator
  to dict_items, but it does not matter here).
- Queue module was renamed to queue.
- cStringIO module should not be used. io module is a replacement available since Python 2.6.
- Maybe other problems...

--
Arfrever Frehtes Taifersar Arahesis

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 2063 bytes --]

On Wednesday 16 October 2013 23:42:26 Arfrever Frehtes Taifersar wrote:
> 2013-10-17 04:53 Mike Frysinger napisał(a):
> > On Wednesday 16 October 2013 22:51:17 Mike Frysinger wrote:
> > > On Wednesday 16 October 2013 20:02:50 Arfrever Frehtes wrote:
> > > > 2013-10-16 23:03 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.
> > > > 
> > > > Some things are incompatible with Python 3.
> > > > See other comments below.
> > > 
> > > i can run a linter on the code (probably should make this a git hook). 
> > > i'm interested more in review on the bigger picture.
> > 
> > also, none of your comments were py3 issues that i saw
> 
> I said "other comments", so I meant comments not related to incompatibility
> with Python 3.

well, w/out providing specific concerns, it's hard to address feedback

> About incompatibility with Python 3:
> - subprocess.check_output(), subprocess.Popen().stdout.read(),
> subprocess.Popen().stderr.read() return bytes, which is incorrectly
> compared with str in your patches.

i've changed the mock code to use subprocess directly so the tests catch this

> - dict.iteritems() was renamed to dict.items() (and its return type was
> changed from dictionary-itemiterator to dict_items, but it does not matter
> here).

fixed

> - Queue module was renamed to queue.

i've sent a patch upstream for this

> - cStringIO module should not be used. io module is a replacement available
> since Python 2.6.

unfortunately, that doesn't work as well as it should.  python 2.7's interface 
is annoyingly different when using other python 2.7 modules.  i'll have the 
code import the old module and then fallback to using the new io module.

> - Maybe other problems...

sorry, but this isn't useful.  i'm going to run the unittests against python 
3.2 and 3.3 and once those pass, i'm done looking at it.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[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
+
+
+@contextlib.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

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

[Arfrever Frehtes Taifersar Arahesis]

[-- Attachment #1: Type: Text/Plain, Size: 1473 bytes --]

2013-10-21 05:00 Mike Frysinger napisał(a):
> On Wednesday 16 October 2013 23:42:26 Arfrever Frehtes Taifersar wrote:
> > - cStringIO module should not be used. io module is a replacement available
> > since Python 2.6.
> 
> unfortunately, that doesn't work as well as it should.  python 2.7's interface 
> is annoyingly different when using other python 2.7 modules.  i'll have the 
> code import the old module and then fallback to using the new io module.

(io.StringIO works only with unicode strings.
When bytes-compatible functions are really needed, then io.BytesIO can be used, which works only with bytes.
cStringIO.StringIO is designed to work with bytes, but cStringIO.StringIO().write(instance_of_unicode)
implicitly encodes its argument to bytes.)

In your another patch, portage.tests.bin.test_prepstrip.PrepStripFull._prepstrip() passes an instance of
cStringIO.StringIO class to portage.bin.prepstrip.main(), which passes it to portage.bin.prepstrip.Prepstrip(),
which passes it to print(), portage.elog.messages.eqawarn() and portage.elog.messages.ewarn().

pym/portage/bin/prepstrip.py should have:
from __future__ import unicode_literals

Then print('...', file=out) calls will work with an instance of io.StringIO class.

(portage.elog.messages.eqawarn() and portage.elog.messages.ewarn() internally decode message,
so they already work with out=io.StringIO, but not out=io.BytesIO.)

--
Arfrever Frehtes Taifersar Arahesis

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Arfrever Frehtes Taifersar Arahesis]

[-- Attachment #1: Type: Text/Plain, Size: 19369 bytes --]

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
> +
> +
> +@contextlib.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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[gentoo-portage-dev] [PATCH v3] 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.
---
 bin/xattr-helper.py                  |  11 +-
 pym/portage/tests/util/test_xattr.py | 178 ++++++++++++++++++++++++++++
 pym/portage/util/_xattr.py           | 221 +++++++++++++++++++++++++++++++++++
 pym/portage/util/movefile.py         | 100 ++++------------
 4 files changed, 423 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 3e9b81e..19f25f9 100755
--- a/bin/xattr-helper.py
+++ b/bin/xattr-helper.py
@@ -19,16 +19,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..2e2564a
--- /dev/null
+++ b/pym/portage/tests/util/test_xattr.py
@@ -0,0 +1,178 @@
+# Copyright 2010-2015 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.
+	# pylint: disable=no-name-in-module
+	from unittest import mock
+except ImportError:
+	try:
+		# Try standalone module.
+		import mock
+	except ImportError:
+		mock = None
+
+import subprocess
+
+import portage
+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.
+	"""
+	# pylint: disable=protected-access
+	proc = orig_popen(['cat'], stdout=subprocess.PIPE, stdin=subprocess.PIPE)
+	proc.stdin.write(portage._unicode_encode(stdin, portage._encodings['stdio']))
+	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..4436c05
--- /dev/null
+++ b/pym/portage/util/_xattr.py
@@ -0,0 +1,221 @@
+# Copyright 2010-2015 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)
+		if proc.stdin:
+			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)
+
+
+class _XattrStub(_XattrGetAll):
+	"""Fake object since system doesn't support xattrs"""
+
+	# pylint: disable=unused-argument
+
+	@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"""
+
+		# pylint: disable=unused-argument
+
+		@staticmethod
+		def get(item, name, nofollow=False, namespace=None):
+			return os.getxattr(item, name, follow_symlinks=not nofollow)
+
+		@staticmethod
+		def set(item, name, value, flags=0, namespace=None):
+			return os.setxattr(item, name, value, flags=flags)
+
+		@staticmethod
+		def remove(item, name, nofollow=False, namespace=None):
+			return os.removexattr(item, name, follow_symlinks=not nofollow)
+
+		@staticmethod
+		def list(item, nofollow=False, namespace=None):
+			return os.listxattr(item, follow_symlinks=not nofollow)
+
+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
+
+
+@contextlib.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.items():
+			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.items():
+			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 d00f624..0cb1977 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']):
-- 
2.4.1

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

[Zac Medico]

On 05/30/2015 08:14 AM, Mike Frysinger wrote:
> +	@classmethod
> +	def list(cls, item, nofollow=False, namespace=None, _names_only=True):
> +		cmd = ['getfattr', '-d', '--absolute-names', item]

All getfattr calls need to use -m- ('-' pattern means match all) in
order to list all attributes.
-- 
Thanks,
Zac

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

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

On 30 May 2015 12:21, Zac Medico wrote:
> On 05/30/2015 08:14 AM, Mike Frysinger wrote:
> > +	@classmethod
> > +	def list(cls, item, nofollow=False, namespace=None, _names_only=True):
> > +		cmd = ['getfattr', '-d', '--absolute-names', item]
> 
> All getfattr calls need to use -m- ('-' pattern means match all) in
> order to list all attributes.

i use -m already.  here's a bit more of that function:

    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)

so when there's no namespace specified, it ends up doing:
	getfattr -d --absolute-names /path/file -m ''

which is effectively the same thing, but i can change the '' to '-' since that's 
what the documentation suggests.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

On 30 May 2015 11:14, Mike Frysinger wrote:
> 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.

and it looks like i just missed the new pym/portage/util/xattr.py module.
i'll have to merge that in too.  but first about the API.

the current code does:
	import portage.util.xattr as _xattr
and then the _xattr module provides three functions:
	getxattr
	listxattr
	setxattr
which is kind of confusing as both pyxattr and python 3.3 (via the os module) 
provide these functions, but with different signatures.  for example:
	os.getxattr(path, attribute, follow_symlinks=True)
	xattr.getxattr(item, attribute[, nofollow=False])
i think this is why the pyxattr module has actually marked all of these as 
deprecated.

mine is:
	from portage.util._xattr import xattr
and then the xattr module provides the standard pyxattr functions:
	xattr.get
	xattr.get_all
	xattr.list
	xattr.remove
	xattr.set
and i omit all of the confusing variants.  the reason i don't put the functions 
into the module itself is so that we can more xattr related functions which i've
already done with the context manager:
	from portage.util._xattr import preserve_xattrs
	with preserve_xattrs('/some/file'):
		...

so if there's no reason to keep portage.util.xattr, i'll kill it and move the 
new quickpkg usage to _xattr.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[gentoo-portage-dev] [PATCH v4] 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.
---
v4
	- merge in recent quickpkg changes
	- add a XATTRS_WORKS symbol for easy testing
	- use - with -m when matching all

 bin/quickpkg                         |  12 +-
 bin/xattr-helper.py                  |  11 +-
 pym/portage/dbapi/vartree.py         |  10 +-
 pym/portage/tests/util/test_xattr.py | 178 +++++++++++++++++++++++++++
 pym/portage/util/_xattr.py           | 228 +++++++++++++++++++++++++++++++++++
 pym/portage/util/movefile.py         | 100 ++++-----------
 pym/portage/util/xattr.py            |  20 ---
 7 files changed, 441 insertions(+), 118 deletions(-)
 create mode 100644 pym/portage/tests/util/test_xattr.py
 create mode 100644 pym/portage/util/_xattr.py
 delete mode 100644 pym/portage/util/xattr.py

diff --git a/bin/quickpkg b/bin/quickpkg
index 726abff..262fda4 100755
--- a/bin/quickpkg
+++ b/bin/quickpkg
@@ -21,8 +21,8 @@ from portage.dbapi.dep_expand import dep_expand
 from portage.dep import Atom, use_reduce
 from portage.exception import (AmbiguousPackageName, InvalidAtom, InvalidData,
 	InvalidDependString, PackageSetNotFound, PermissionDenied)
-from portage.util import ConfigProtect, ensure_dirs, shlex_split
-import portage.util.xattr as _xattr
+from portage.util import ConfigProtect, ensure_dirs, shlex_split, _xattr
+xattr = _xattr.xattr
 from portage.dbapi.vartree import dblink, tar_contents
 from portage.checksum import perform_md5
 from portage._sets import load_default_config, SETPREFIX
@@ -36,7 +36,7 @@ def quickpkg_atom(options, infos, arg, eout):
 	vartree = trees["vartree"]
 	vardb = vartree.dbapi
 	bintree = trees["bintree"]
-	xattr = 'xattr' in settings.features
+	xattrs = 'xattr' in settings.features
 
 	include_config = options.include_config == "y"
 	include_unmodified_config = options.include_unmodified_config == "y"
@@ -137,8 +137,8 @@ def quickpkg_atom(options, infos, arg, eout):
 			# The tarfile module will write pax headers holding the
 			# xattrs only if PAX_FORMAT is specified here.
 			tar = tarfile.open(binpkg_tmpfile, "w:bz2",
-				format=tarfile.PAX_FORMAT if xattr else tarfile.DEFAULT_FORMAT)
-			tar_contents(contents, root, tar, protect=protect, xattr=xattr)
+				format=tarfile.PAX_FORMAT if xattrs else tarfile.DEFAULT_FORMAT)
+			tar_contents(contents, root, tar, protect=protect, xattrs=xattrs)
 			tar.close()
 			xpak.tbz2(binpkg_tmpfile).recompose_mem(xpdata)
 		finally:
@@ -238,7 +238,7 @@ def quickpkg_main(options, args, eout):
 		eout.eerror("No write access to '%s'" % bintree.pkgdir)
 		return errno.EACCES
 
-	if 'xattr' in portage.settings.features and not hasattr(_xattr, 'getxattr'):
+	if 'xattr' in portage.settings.features and not _xattr.XATTRS_WORKS:
 		eout.eerror("No xattr support library was found, "
 			"so xattrs will not be preserved!")
 		portage.settings.unlock()
diff --git a/bin/xattr-helper.py b/bin/xattr-helper.py
index 3e9b81e..19f25f9 100755
--- a/bin/xattr-helper.py
+++ b/bin/xattr-helper.py
@@ -19,16 +19,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/dbapi/vartree.py b/pym/portage/dbapi/vartree.py
index 62d880e..e755fde 100644
--- a/pym/portage/dbapi/vartree.py
+++ b/pym/portage/dbapi/vartree.py
@@ -35,7 +35,7 @@ portage.proxy.lazyimport.lazyimport(globals(),
 	'portage.util.movefile:movefile',
 	'portage.util.path:first_existing,iter_parents',
 	'portage.util.writeable_check:get_ro_checker',
-	'portage.util:xattr@_xattr',
+	'portage.util._xattr:xattr',
 	'portage.util._dyn_libs.PreservedLibsRegistry:PreservedLibsRegistry',
 	'portage.util._dyn_libs.LinkageMapELF:LinkageMapELF@LinkageMap',
 	'portage.util._async.SchedulerInterface:SchedulerInterface',
@@ -5268,7 +5268,7 @@ def write_contents(contents, root, f):
 		f.write(line)
 
 def tar_contents(contents, root, tar, protect=None, onProgress=None,
-	xattr=False):
+                 xattrs=False):
 	os = _os_merge
 	encoding = _encodings['merge']
 
@@ -5390,13 +5390,13 @@ def tar_contents(contents, root, tar, protect=None, onProgress=None,
 					encoding=encoding,
 					errors='strict')
 
-				if xattr:
+				if xattrs:
 					# Compatible with GNU tar, which saves the xattrs
 					# under the SCHILY.xattr namespace.
-					for k in _xattr.listxattr(path_bytes):
+					for k in xattr.list(path_bytes):
 						tarinfo.pax_headers['SCHILY.xattr.' +
 							_unicode_decode(k)] = _unicode_decode(
-							_xattr.getxattr(path_bytes, _unicode_encode(k)))
+							xattr.get(path_bytes, _unicode_encode(k)))
 
 				with open(path_bytes, 'rb') as f:
 					tar.addfile(tarinfo, f)
diff --git a/pym/portage/tests/util/test_xattr.py b/pym/portage/tests/util/test_xattr.py
new file mode 100644
index 0000000..2e2564a
--- /dev/null
+++ b/pym/portage/tests/util/test_xattr.py
@@ -0,0 +1,178 @@
+# Copyright 2010-2015 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.
+	# pylint: disable=no-name-in-module
+	from unittest import mock
+except ImportError:
+	try:
+		# Try standalone module.
+		import mock
+	except ImportError:
+		mock = None
+
+import subprocess
+
+import portage
+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.
+	"""
+	# pylint: disable=protected-access
+	proc = orig_popen(['cat'], stdout=subprocess.PIPE, stdin=subprocess.PIPE)
+	proc.stdin.write(portage._unicode_encode(stdin, portage._encodings['stdio']))
+	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..9a8704d
--- /dev/null
+++ b/pym/portage/util/_xattr.py
@@ -0,0 +1,228 @@
+# Copyright 2010-2015 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.
+We do not include the functions that Python 3.3+ provides in the os module as
+the signature there is different compared to xattr.
+
+See the standard xattr module for more documentation:
+	https://pypi.python.org/pypi/pyxattr
+"""
+
+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)
+		if proc.stdin:
+			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)
+
+
+class _XattrStub(_XattrGetAll):
+	"""Fake object since system doesn't support xattrs"""
+
+	# pylint: disable=unused-argument
+
+	@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"""
+
+		# pylint: disable=unused-argument
+
+		@staticmethod
+		def get(item, name, nofollow=False, namespace=None):
+			return os.getxattr(item, name, follow_symlinks=not nofollow)
+
+		@staticmethod
+		def set(item, name, value, flags=0, namespace=None):
+			return os.setxattr(item, name, value, flags=flags)
+
+		@staticmethod
+		def remove(item, name, nofollow=False, namespace=None):
+			return os.removexattr(item, name, follow_symlinks=not nofollow)
+
+		@staticmethod
+		def list(item, nofollow=False, namespace=None):
+			return os.listxattr(item, follow_symlinks=not nofollow)
+
+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
+
+
+# Add a knob so code can take evasive action as needed.
+XATTRS_WORKS = xattr != _XattrStub
+
+
+@contextlib.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.items():
+			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.items():
+			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 d00f624..0cb1977 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']):
diff --git a/pym/portage/util/xattr.py b/pym/portage/util/xattr.py
deleted file mode 100644
index b8c4620..0000000
--- a/pym/portage/util/xattr.py
+++ /dev/null
@@ -1,20 +0,0 @@
-# Copyright 2015 Gentoo Foundation
-# Distributed under the terms of the GNU General Public License v2
-
-from __future__ import absolute_import
-
-import os as _os
-
-if hasattr(_os, "getxattr"):
-	getxattr = _os.getxattr
-	listxattr = _os.listxattr
-	setxattr = _os.setxattr
-else:
-	try:
-		import xattr as _xattr
-	except ImportError:
-		pass
-	else:
-		getxattr = _xattr.get
-		listxattr = _xattr.list
-		setxattr = _xattr.set
-- 
2.4.1

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

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 274 bytes --]

On 30 May 2015 16:59, Mike Frysinger wrote:
> 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.

ping ... are people ok with this change in API ?
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Zac Medico]

On 05/30/2015 01:59 PM, Mike Frysinger wrote:
> 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.
> ---
> v4
> 	- merge in recent quickpkg changes
> 	- add a XATTRS_WORKS symbol for easy testing
> 	- use - with -m when matching all
> 
>  bin/quickpkg                         |  12 +-
>  bin/xattr-helper.py                  |  11 +-
>  pym/portage/dbapi/vartree.py         |  10 +-
>  pym/portage/tests/util/test_xattr.py | 178 +++++++++++++++++++++++++++
>  pym/portage/util/_xattr.py           | 228 +++++++++++++++++++++++++++++++++++
>  pym/portage/util/movefile.py         | 100 ++++-----------
>  pym/portage/util/xattr.py            |  20 ---
>  7 files changed, 441 insertions(+), 118 deletions(-)
>  create mode 100644 pym/portage/tests/util/test_xattr.py
>  create mode 100644 pym/portage/util/_xattr.py
>  delete mode 100644 pym/portage/util/xattr.py
> 

LGTM, except this one line is indented with spaces instead of tabs in
vartree.py:

>  def tar_contents(contents, root, tar, protect=None, onProgress=None,
> -	xattr=False):
> +                 xattrs=False):

-- 
Thanks,
Zac

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

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]

On 10 Jun 2015 11:54, Zac Medico wrote:
> On 05/30/2015 01:59 PM, Mike Frysinger wrote:
> LGTM, except this one line is indented with spaces instead of tabs in
> vartree.py:
> 
> >  def tar_contents(contents, root, tar, protect=None, onProgress=None,
> > -	xattr=False):
> > +                 xattrs=False):

i don't know if we have a standard here.  sometimes it's a single tab, sometimes 
it's spaces to line up.  i prefer the latter as that's generally what PEP8 does.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Zac Medico]

On 06/10/2015 10:39 PM, Mike Frysinger wrote:
> On 10 Jun 2015 11:54, Zac Medico wrote:
>> On 05/30/2015 01:59 PM, Mike Frysinger wrote:
>> LGTM, except this one line is indented with spaces instead of tabs in
>> vartree.py:
>>
>>>  def tar_contents(contents, root, tar, protect=None, onProgress=None,
>>> -	xattr=False):
>>> +                 xattrs=False):
> 
> i don't know if we have a standard here.  sometimes it's a single tab, sometimes 
> it's spaces to line up.  i prefer the latter as that's generally what PEP8 does.
> -mike
> 

Ah, ok. The test pass, so guess it's fine that way.
-- 
Thanks,
Zac

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

[Brian Dolbec]

On Wed, 10 Jun 2015 22:43:38 -0700
Zac Medico <zmedico@gentoo.org> wrote:

> On 06/10/2015 10:39 PM, Mike Frysinger wrote:
> > On 10 Jun 2015 11:54, Zac Medico wrote:
> >> On 05/30/2015 01:59 PM, Mike Frysinger wrote:
> >> LGTM, except this one line is indented with spaces instead of tabs
> >> in vartree.py:
> >>
> >>>  def tar_contents(contents, root, tar, protect=None,
> >>> onProgress=None,
> >>> -	xattr=False):
> >>> +                 xattrs=False):
> > 
> > i don't know if we have a standard here.  sometimes it's a single
> > tab, sometimes it's spaces to line up.  i prefer the latter as
> > that's generally what PEP8 does. -mike
> > 
> 
> Ah, ok. The test pass, so guess it's fine that way.


But I still don't like it being mixed.  It may not be a problem due to
it being contained inside the def statement.  But I've had some strange
results when code is run with mixed spaces/tabs indents.

Portage code is not nearly pep8 and we have not set pep8 as a standard
for us to adhere to.  Although, I personally try to keep my stuff close
to pep8.  Aside from the fact I prefer a few things different than that
standard.

In this case, it is minor, but 1 tab indent (which is normal) also does
not show the continuation of the def readily.  I have at times just
given it an additional indent tab to differentiate it from the following
code.  I personally don't adhere to the line up all params with the 

     foo(param1,
         param2)

It's fine when it is close to the left side, but when the
start ( is near the right side, it can be down right
impossible to stay within the 80 col. limit for long
parameters.  Besides the often times wasted space of having
all params on separate lines.  IMHO it can take away from
readability at times while increasing readability for complex
function calls.

I've come across code in portage with both a mix of tabs and
spaces on the same line(s)...  In such cases, I have my editor convert
all leading spaces to tabs.  I don't look for odd cases like the above.
But I will usually take note of them in the commit diff while making the
commit.

Personally, I'd prefer you stay with tabs, even if it does not
line up exactly with the (   ignoring personal preferences for
tab settings ( 1 tab = 4 spaces,...)

Yes, I very much like the code being centralized in one place.  :)

We can look at any more convention changes after the next
lead election.   
-- 
Brian Dolbec <dolsen>

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

[Alexander Berntsen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 11/06/15 09:06, Brian Dolbec wrote:
> Personally, I'd prefer you stay with tabs, even if it does not line
> up exactly with the (   ignoring personal preferences for tab
> settings ( 1 tab = 4 spaces,...)
Mixing tabs & spaces is a poor idea, so I agree with Brian.

(However, if I were king of the universe or somesuch, there would be
no tabs in our Python code -- at all.)
- -- 
Alexander
bernalex@gentoo.org
https://secure.plaimi.net/~alexander
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCgAGBQJVetSlAAoJENQqWdRUGk8BbHsQANJusb5/FRKL+wKRNzVtcF7b
06AwCEcYDiMHWOjzeWOwVNnKFRmuDCadUpAj1Zwt78wxB2TZd4Dk2UoO8y6mrKmX
AvN6tTCExoisEkP3xUCUkXMX2dorNnttrOThQg4YeczMG1UfKce8PkXHjyQEFHCe
yNY13UPvJsuZbjfQF4TvlRcC9CetvyjpKpheMbuxcGhtJiRU1Wm+K9KTVzRWT1lI
h5u5nTYXlmAPTNHUFbyr2tSEIotIBye8YtiBkpocjduqomYEF+bWQyROGEys0MQO
xTmyQT9+hkTUE5tyjDKRtg3cViG4YTK4cDau8FQ0sRj5NZOVCsxnYJvCXtxU10bE
TCZl0KrXoNGkZ2iUWcjuFXxc7BbCpV/+pMWypAFxoa5SZMoy6/8G1gImdVH86lT9
k5JmTcD3jrjookaBPd6FqVg2BcED6UHDjm4c3m5f8iUpCgvRcWB0/o3o++UvFMKe
mHboHvlIS2WOC2r+cN9xNzT5Og18H7rs350b0A1KopItLKVF1QCfizSi3UuNHAtM
/WmuJ6y55lgS/uqeUys7V+K5AJ6rWQpOCkcfVumVRnAYs8Arr2Bkjkg0f3Z1h9Ui
7P5lBM9QENZtnQxn+gYpARdkMcLxKjDUDCskpsZw5UYCVb1wImM9GqhtKK5LV2Lk
1c1kIz90PECAoMH9xe4f
=jgxr
-----END PGP SIGNATURE-----

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

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]

On 30 May 2015 16:59, Mike Frysinger wrote:
> 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.

noticed i still hadn't pushed this, so i've done so now

if you run into test failures, make sure you delete your old pyc files:
	find -name '*.pyc' -delete
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]