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

[Brian Dolbec <dolsen@gentoo.org>]

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>