From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) by finch.gentoo.org (Postfix) with ESMTP id 846CD138A1A for ; Mon, 12 Jan 2015 01:21:53 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 606CAE0817; Mon, 12 Jan 2015 01:21:52 +0000 (UTC) Received: from smtp.gentoo.org (smtp.gentoo.org [140.211.166.183]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id C84CCE0810 for ; Mon, 12 Jan 2015 01:21:51 +0000 (UTC) Received: from big_daddy.dol-sen.ca (S010634bdfa9ecf80.vc.shawcable.net [96.49.31.57]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: dolsen) by smtp.gentoo.org (Postfix) with ESMTPSA id 85855340694 for ; Mon, 12 Jan 2015 01:21:50 +0000 (UTC) Date: Sun, 11 Jan 2015 17:21:35 -0800 From: Brian Dolbec To: gentoo-portage-dev@lists.gentoo.org Subject: Re: [gentoo-portage-dev] [PATCH] dispatch-conf: avoid symlink "File exists" error (535850) Message-ID: <20150111172135.13c39400.dolsen@gentoo.org> In-Reply-To: <1420966190-7007-1-git-send-email-zmedico@gentoo.org> References: <1420966190-7007-1-git-send-email-zmedico@gentoo.org> Organization: Gentoo Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-portage-dev@lists.gentoo.org Reply-to: gentoo-portage-dev@lists.gentoo.org MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Archives-Salt: 3d7c19bd-8c10-4565-899e-28036a971811 X-Archives-Hash: e9be01adf1f148af021bf76782de7731 On Sun, 11 Jan 2015 00:49:50 -0800 Zac Medico 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