[gentoo-portage-dev] [PATCH] dispatch-conf: avoid symlink "File exists" error (535850)

[gentoo-portage-dev] [PATCH] dispatch-conf: avoid symlink "File exists" error (535850)

[Zac Medico]

Since commit f17448317166bfac42dc279b8795cd581c189582, an existing
symlink in /etc/config-archive could trigger a fatal "File exists"
error. Handle this by removing the destination file if it exists. This
was not necessary when dispatch-conf only supported regular files,
since shutil.copy2 would simply overwrite the regular destination file.

Fixes: f17448317166 ("dispatch-conf: symlink support for bug #485598")
X-Gentoo-Bug: 535850
X-Gentoo-Bug-URL: https://bugs.gentoo.org/show_bug.cgi?id=535850
---
 pym/portage/dispatch_conf.py | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/pym/portage/dispatch_conf.py b/pym/portage/dispatch_conf.py
index b27e68b..7d55182 100644
--- a/pym/portage/dispatch_conf.py
+++ b/pym/portage/dispatch_conf.py
@@ -179,6 +179,12 @@ def rcs_archive(archive, curconf, newconf, mrgconf):
 	if curconf_st is not None and \
 		(stat.S_ISREG(curconf_st.st_mode) or
 		stat.S_ISLNK(curconf_st.st_mode)):
+		# Remove destination file in order to ensure that the following
+		# symlink or copy2 call won't fail (see bug #535850).
+		try:
+			os.unlink(archive)
+		except OSError:
+			pass
 		try:
 			if stat.S_ISLNK(curconf_st.st_mode):
 				os.symlink(os.readlink(curconf), archive)
@@ -208,6 +214,12 @@ def rcs_archive(archive, curconf, newconf, mrgconf):
 		if has_branch:
 			os.rename(archive, archive + '.dist')
 
+		# Remove destination file in order to ensure that the following
+		# symlink or copy2 call won't fail (see bug #535850).
+		try:
+			os.unlink(archive)
+		except OSError:
+			pass
 		try:
 			if stat.S_ISLNK(mystat.st_mode):
 				os.symlink(os.readlink(newconf), archive)
@@ -264,6 +276,12 @@ def file_archive(archive, curconf, newconf, mrgconf):
 	if curconf_st is not None and \
 		(stat.S_ISREG(curconf_st.st_mode) or
 		stat.S_ISLNK(curconf_st.st_mode)):
+		# Remove destination file in order to ensure that the following
+		# symlink or copy2 call won't fail (see bug #535850).
+		try:
+			os.unlink(archive)
+		except OSError:
+			pass
 		try:
 			if stat.S_ISLNK(curconf_st.st_mode):
 				os.symlink(os.readlink(curconf), archive)
@@ -285,6 +303,12 @@ def file_archive(archive, curconf, newconf, mrgconf):
 		stat.S_ISLNK(mystat.st_mode)):
 		# Save off new config file in the archive dir with .dist.new suffix
 		newconf_archive = archive + '.dist.new'
+		# Remove destination file in order to ensure that the following
+		# symlink or copy2 call won't fail (see bug #535850).
+		try:
+			os.unlink(newconf_archive)
+		except OSError:
+			pass
 		try:
 			if stat.S_ISLNK(mystat.st_mode):
 				os.symlink(os.readlink(newconf), newconf_archive)
-- 
2.0.5

Re: [gentoo-portage-dev] [PATCH] dispatch-conf: avoid symlink "File exists" error (535850)

[Brian Dolbec]

On Sun, 11 Jan 2015 00:49:50 -0800
Zac Medico <zmedico@gentoo.org> wrote:

> Since commit f17448317166bfac42dc279b8795cd581c189582, an existing
> symlink in /etc/config-archive could trigger a fatal "File exists"
> error. Handle this by removing the destination file if it exists. This
> was not necessary when dispatch-conf only supported regular files,
> since shutil.copy2 would simply overwrite the regular destination
> file.
> 
> Fixes: f17448317166 ("dispatch-conf: symlink support for bug #485598")
> X-Gentoo-Bug: 535850
> X-Gentoo-Bug-URL: https://bugs.gentoo.org/show_bug.cgi?id=535850
> ---
>  pym/portage/dispatch_conf.py | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/pym/portage/dispatch_conf.py
> b/pym/portage/dispatch_conf.py index b27e68b..7d55182 100644
> --- a/pym/portage/dispatch_conf.py
> +++ b/pym/portage/dispatch_conf.py
> @@ -179,6 +179,12 @@ def rcs_archive(archive, curconf, newconf,
> mrgconf): if curconf_st is not None and \
>  		(stat.S_ISREG(curconf_st.st_mode) or
>  		stat.S_ISLNK(curconf_st.st_mode)):
> +		# Remove destination file in order to ensure that
> the following
> +		# symlink or copy2 call won't fail (see bug #535850).
> +		try:
> +			os.unlink(archive)
> +		except OSError:
> +			pass
>  		try:
>  			if stat.S_ISLNK(curconf_st.st_mode):
>  				os.symlink(os.readlink(curconf),
> archive) @@ -208,6 +214,12 @@ def rcs_archive(archive, curconf,
> newconf, mrgconf): if has_branch:
>  			os.rename(archive, archive + '.dist')
>  
> +		# Remove destination file in order to ensure that
> the following
> +		# symlink or copy2 call won't fail (see bug #535850).
> +		try:
> +			os.unlink(archive)
> +		except OSError:
> +			pass
>  		try:
>  			if stat.S_ISLNK(mystat.st_mode):
>  				os.symlink(os.readlink(newconf),
> archive) @@ -264,6 +276,12 @@ def file_archive(archive, curconf,
> newconf, mrgconf): if curconf_st is not None and \
>  		(stat.S_ISREG(curconf_st.st_mode) or
>  		stat.S_ISLNK(curconf_st.st_mode)):
> +		# Remove destination file in order to ensure that
> the following
> +		# symlink or copy2 call won't fail (see bug #535850).
> +		try:
> +			os.unlink(archive)
> +		except OSError:
> +			pass
>  		try:
>  			if stat.S_ISLNK(curconf_st.st_mode):
>  				os.symlink(os.readlink(curconf),
> archive) @@ -285,6 +303,12 @@ def file_archive(archive, curconf,
> newconf, mrgconf): stat.S_ISLNK(mystat.st_mode)):
>  		# Save off new config file in the archive dir
> with .dist.new suffix newconf_archive = archive + '.dist.new'
> +		# Remove destination file in order to ensure that
> the following
> +		# symlink or copy2 call won't fail (see bug #535850).
> +		try:
> +			os.unlink(newconf_archive)
> +		except OSError:
> +			pass
>  		try:
>  			if stat.S_ISLNK(mystat.st_mode):
>  				os.symlink(os.readlink(newconf),
> newconf_archive)


That makes duplicated code in 4 places :/  I know it's only 4 lines of
actual code plus 2 more for the same repeated comments.

BUT...

This diff doens't show if it's all in the same class.

If possible I'd prefer a small breakout function to do the unlink.
Decorate it with @staticmethod even.  If it's named right, you won't
even need the comments repeated throughout the code.   Just the
docstring with that comment info.

-- 
Brian Dolbec <dolsen>

Re: [gentoo-portage-dev] [PATCH] dispatch-conf: avoid symlink "File exists" error (535850)

[Zac Medico]

On 01/11/2015 05:21 PM, Brian Dolbec wrote:
> On Sun, 11 Jan 2015 00:49:50 -0800
> Zac Medico <zmedico@gentoo.org> wrote:
> 
>> Since commit f17448317166bfac42dc279b8795cd581c189582, an existing
>> symlink in /etc/config-archive could trigger a fatal "File exists"
>> error. Handle this by removing the destination file if it exists. This
>> was not necessary when dispatch-conf only supported regular files,
>> since shutil.copy2 would simply overwrite the regular destination
>> file.
>>
>> Fixes: f17448317166 ("dispatch-conf: symlink support for bug #485598")
>> X-Gentoo-Bug: 535850
>> X-Gentoo-Bug-URL: https://bugs.gentoo.org/show_bug.cgi?id=535850
>> ---
>>  pym/portage/dispatch_conf.py | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/pym/portage/dispatch_conf.py
>> b/pym/portage/dispatch_conf.py index b27e68b..7d55182 100644
>> --- a/pym/portage/dispatch_conf.py
>> +++ b/pym/portage/dispatch_conf.py
>> @@ -179,6 +179,12 @@ def rcs_archive(archive, curconf, newconf,
>> mrgconf): if curconf_st is not None and \
>>  		(stat.S_ISREG(curconf_st.st_mode) or
>>  		stat.S_ISLNK(curconf_st.st_mode)):
>> +		# Remove destination file in order to ensure that
>> the following
>> +		# symlink or copy2 call won't fail (see bug #535850).
>> +		try:
>> +			os.unlink(archive)
>> +		except OSError:
>> +			pass
>>  		try:
>>  			if stat.S_ISLNK(curconf_st.st_mode):
>>  				os.symlink(os.readlink(curconf),
>> archive) @@ -208,6 +214,12 @@ def rcs_archive(archive, curconf,
>> newconf, mrgconf): if has_branch:
>>  			os.rename(archive, archive + '.dist')
>>  
>> +		# Remove destination file in order to ensure that
>> the following
>> +		# symlink or copy2 call won't fail (see bug #535850).
>> +		try:
>> +			os.unlink(archive)
>> +		except OSError:
>> +			pass
>>  		try:
>>  			if stat.S_ISLNK(mystat.st_mode):
>>  				os.symlink(os.readlink(newconf),
>> archive) @@ -264,6 +276,12 @@ def file_archive(archive, curconf,
>> newconf, mrgconf): if curconf_st is not None and \
>>  		(stat.S_ISREG(curconf_st.st_mode) or
>>  		stat.S_ISLNK(curconf_st.st_mode)):
>> +		# Remove destination file in order to ensure that
>> the following
>> +		# symlink or copy2 call won't fail (see bug #535850).
>> +		try:
>> +			os.unlink(archive)
>> +		except OSError:
>> +			pass
>>  		try:
>>  			if stat.S_ISLNK(curconf_st.st_mode):
>>  				os.symlink(os.readlink(curconf),
>> archive) @@ -285,6 +303,12 @@ def file_archive(archive, curconf,
>> newconf, mrgconf): stat.S_ISLNK(mystat.st_mode)):
>>  		# Save off new config file in the archive dir
>> with .dist.new suffix newconf_archive = archive + '.dist.new'
>> +		# Remove destination file in order to ensure that
>> the following
>> +		# symlink or copy2 call won't fail (see bug #535850).
>> +		try:
>> +			os.unlink(newconf_archive)
>> +		except OSError:
>> +			pass
>>  		try:
>>  			if stat.S_ISLNK(mystat.st_mode):
>>  				os.symlink(os.readlink(newconf),
>> newconf_archive)
> 
> 
> That makes duplicated code in 4 places :/  I know it's only 4 lines of
> actual code plus 2 more for the same repeated comments.
> 
> BUT...
> 
> This diff doens't show if it's all in the same class.

The changes affect 2 different functions (rcs_archive and file_archive),
and add 2 unlink calls per function. Both function are very similar, but
only one of them is called, depending on whether or not the user has rcs
support enabled.

> If possible I'd prefer a small breakout function to do the unlink.
> Decorate it with @staticmethod even.  If it's named right, you won't
> even need the comments repeated throughout the code.   Just the
> docstring with that comment info.

Maybe should just omit the comments and leave the unlink calls inline?
It feels like overkill to split out a function for this.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] dispatch-conf: avoid symlink "File exists" error (535850)

[Brian Dolbec]

On Sun, 11 Jan 2015 17:39:23 -0800
Zac Medico <zmedico@gentoo.org> wrote:

> On 01/11/2015 05:21 PM, Brian Dolbec wrote:
> > On Sun, 11 Jan 2015 00:49:50 -0800
> > Zac Medico <zmedico@gentoo.org> wrote:

> >> archive) @@ -285,6 +303,12 @@ def file_archive(archive, curconf,
> >> newconf, mrgconf): stat.S_ISLNK(mystat.st_mode)):
> >>  		# Save off new config file in the archive dir
> >> with .dist.new suffix newconf_archive = archive + '.dist.new'
> >> +		# Remove destination file in order to ensure that
> >> the following
> >> +		# symlink or copy2 call won't fail (see bug
> >> #535850).
> >> +		try:
> >> +			os.unlink(newconf_archive)
> >> +		except OSError:
> >> +			pass
> >>  		try:
> >>  			if stat.S_ISLNK(mystat.st_mode):
> >>  				os.symlink(os.readlink(newconf),
> >> newconf_archive)
> > 
> > 
> > That makes duplicated code in 4 places :/  I know it's only 4 lines
> > of actual code plus 2 more for the same repeated comments.
> > 
> > BUT...
> > 
> > This diff doens't show if it's all in the same class.
> 
> The changes affect 2 different functions (rcs_archive and
> file_archive), and add 2 unlink calls per function. Both function are
> very similar, but only one of them is called, depending on whether or
> not the user has rcs support enabled.
> 
> > If possible I'd prefer a small breakout function to do the unlink.
> > Decorate it with @staticmethod even.  If it's named right, you won't
> > even need the comments repeated throughout the code.   Just the
> > docstring with that comment info.
> 
> Maybe should just omit the comments and leave the unlink calls inline?
> It feels like overkill to split out a function for this.


Ok, looking at the actual file in whole, it is even worse.

Here is the full chunk of code that is repeated 4 times with only some
variable names that are different.  Those differences are easily handled via
parameter passing.

		# Remove destination file in order to ensure that the following
		# symlink or copy2 call won't fail (see bug #535850).
		try:
			os.unlink(archive)
		except OSError:
			pass
		try:
			if stat.S_ISLNK(mystat.st_mode):
				os.symlink(os.readlink(newconf), archive)
			else:
				shutil.copy2(newconf, archive)
		except(IOError, os.error) as why:
			print(_('dispatch-conf: Error copying %(newconf)s to %(archive)s: %(reason)s; fatal') % \
				{"newconf": newconf, "archive": archive, "reason": str(why)}, file=sys.stderr)

That's a total of 14 lines of code for a single function to process.  And only 2 of them are comments

That is 42 lines of redundant code.

That is more than "WORTH IT" and not overkill IMHO.
But I do concede you should make a second commit optimizing this into one function so as to not confuse the optimizing with the original bugfix change.
-- 
Brian Dolbec <dolsen>

[gentoo-portage-dev] [PATCH] dispatch_conf: factor out _archive_copy

[Zac Medico]

This eliminates 4 instances of duplicate code from the rcs_archive and
file_archive functions.

Suggested-by: Brian Dolbec <dolsen@gentoo.org>
---
 pym/portage/dispatch_conf.py | 92 +++++++++++++++++---------------------------
 1 file changed, 36 insertions(+), 56 deletions(-)

diff --git a/pym/portage/dispatch_conf.py b/pym/portage/dispatch_conf.py
index 7d55182..790eacb 100644
--- a/pym/portage/dispatch_conf.py
+++ b/pym/portage/dispatch_conf.py
@@ -158,6 +158,38 @@ def read_config(mandatory_opts):
 
 	return opts
 
+def _archive_copy(src_st, src_path, dest_path):
+	"""
+	Copy file from src_path to dest_path. Regular files and symlinks
+	are supported. If an EnvironmentError occurs, then it is logged
+	to stderr.
+
+	@param src_st: source file lstat result
+	@type src_st: posix.stat_result
+	@param src_path: source file path
+	@type src_path: str
+	@param dest_path: destination file path
+	@type dest_path: str
+	"""
+	# Remove destination file in order to ensure that the following
+	# symlink or copy2 call won't fail (see bug #535850).
+	try:
+		os.unlink(dest_path)
+	except OSError:
+		pass
+	try:
+		if stat.S_ISLNK(src_st.st_mode):
+			os.symlink(os.readlink(src_path), dest_path)
+		else:
+			shutil.copy2(src_path, dest_path)
+	except EnvironmentError as e:
+		portage.util.writemsg(
+			_('dispatch-conf: Error copying %(src_path)s to '
+			'%(dest_path)s: %(reason)s\n') % {
+				"src_path": src_path,
+				"dest_path": dest_path,
+				"reason": e
+			}, noiselevel=-1)
 
 def rcs_archive(archive, curconf, newconf, mrgconf):
 	"""Archive existing config in rcs (on trunk). Then, if mrgconf is
@@ -179,20 +211,7 @@ def rcs_archive(archive, curconf, newconf, mrgconf):
 	if curconf_st is not None and \
 		(stat.S_ISREG(curconf_st.st_mode) or
 		stat.S_ISLNK(curconf_st.st_mode)):
-		# Remove destination file in order to ensure that the following
-		# symlink or copy2 call won't fail (see bug #535850).
-		try:
-			os.unlink(archive)
-		except OSError:
-			pass
-		try:
-			if stat.S_ISLNK(curconf_st.st_mode):
-				os.symlink(os.readlink(curconf), archive)
-			else:
-				shutil.copy2(curconf, archive)
-		except(IOError, os.error) as why:
-			print(_('dispatch-conf: Error copying %(curconf)s to %(archive)s: %(reason)s; fatal') % \
-				{"curconf": curconf, "archive": archive, "reason": str(why)}, file=sys.stderr)
+		_archive_copy(curconf_st, curconf, archive)
 
 	if os.path.lexists(archive + ',v'):
 		os.system(RCS_LOCK + ' ' + archive)
@@ -214,20 +233,7 @@ def rcs_archive(archive, curconf, newconf, mrgconf):
 		if has_branch:
 			os.rename(archive, archive + '.dist')
 
-		# Remove destination file in order to ensure that the following
-		# symlink or copy2 call won't fail (see bug #535850).
-		try:
-			os.unlink(archive)
-		except OSError:
-			pass
-		try:
-			if stat.S_ISLNK(mystat.st_mode):
-				os.symlink(os.readlink(newconf), archive)
-			else:
-				shutil.copy2(newconf, archive)
-		except(IOError, os.error) as why:
-			print(_('dispatch-conf: Error copying %(newconf)s to %(archive)s: %(reason)s; fatal') % \
-				{"newconf": newconf, "archive": archive, "reason": str(why)}, file=sys.stderr)
+		_archive_copy(mystat, newconf, archive)
 
 		if has_branch:
 			if mrgconf and os.path.isfile(archive) and \
@@ -276,20 +282,7 @@ def file_archive(archive, curconf, newconf, mrgconf):
 	if curconf_st is not None and \
 		(stat.S_ISREG(curconf_st.st_mode) or
 		stat.S_ISLNK(curconf_st.st_mode)):
-		# Remove destination file in order to ensure that the following
-		# symlink or copy2 call won't fail (see bug #535850).
-		try:
-			os.unlink(archive)
-		except OSError:
-			pass
-		try:
-			if stat.S_ISLNK(curconf_st.st_mode):
-				os.symlink(os.readlink(curconf), archive)
-			else:
-				shutil.copy2(curconf, archive)
-		except(IOError, os.error) as why:
-			print(_('dispatch-conf: Error copying %(curconf)s to %(archive)s: %(reason)s; fatal') % \
-				{"curconf": curconf, "archive": archive, "reason": str(why)}, file=sys.stderr)
+		_archive_copy(curconf_st, curconf, archive)
 
 	mystat = None
 	if newconf:
@@ -303,20 +296,7 @@ def file_archive(archive, curconf, newconf, mrgconf):
 		stat.S_ISLNK(mystat.st_mode)):
 		# Save off new config file in the archive dir with .dist.new suffix
 		newconf_archive = archive + '.dist.new'
-		# Remove destination file in order to ensure that the following
-		# symlink or copy2 call won't fail (see bug #535850).
-		try:
-			os.unlink(newconf_archive)
-		except OSError:
-			pass
-		try:
-			if stat.S_ISLNK(mystat.st_mode):
-				os.symlink(os.readlink(newconf), newconf_archive)
-			else:
-				shutil.copy2(newconf, newconf_archive)
-		except(IOError, os.error) as why:
-			print(_('dispatch-conf: Error copying %(newconf)s to %(archive)s: %(reason)s; fatal') % \
-				{"newconf": newconf, "archive": archive + '.dist.new', "reason": str(why)}, file=sys.stderr)
+		_archive_copy(mystat, newconf, newconf_archive)
 
 		ret = 0
 		if mrgconf and os.path.isfile(curconf) and \
-- 
2.0.5

Re: [gentoo-portage-dev] [PATCH] dispatch_conf: factor out _archive_copy

[Brian Dolbec]

On Sun, 11 Jan 2015 21:16:55 -0800
Zac Medico <zmedico@gentoo.org> wrote:

> This eliminates 4 instances of duplicate code from the rcs_archive and
> file_archive functions.
> 
> Suggested-by: Brian Dolbec <dolsen@gentoo.org>
> ---

Looks much better, 

Thank you, merge both patches please
-- 
Brian Dolbec <dolsen>