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 F3DBD138A1A for ; Mon, 12 Jan 2015 01:39:30 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 97466E0828; Mon, 12 Jan 2015 01:39:28 +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 A8B85E0823 for ; Mon, 12 Jan 2015 01:39:27 +0000 (UTC) Received: from [192.168.43.17] (99.sub-70-209-195.myvzw.com [70.209.195.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: zmedico) by smtp.gentoo.org (Postfix) with ESMTPSA id 94F05340634 for ; Mon, 12 Jan 2015 01:39:26 +0000 (UTC) Message-ID: <54B325CB.10100@gentoo.org> Date: Sun, 11 Jan 2015 17:39:23 -0800 From: Zac Medico User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 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 To: gentoo-portage-dev@lists.gentoo.org Subject: Re: [gentoo-portage-dev] [PATCH] dispatch-conf: avoid symlink "File exists" error (535850) References: <1420966190-7007-1-git-send-email-zmedico@gentoo.org> <20150111172135.13c39400.dolsen@gentoo.org> In-Reply-To: <20150111172135.13c39400.dolsen@gentoo.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-Archives-Salt: 6550963f-58d3-4fc5-b21e-16e50db63ad9 X-Archives-Hash: d0f5983fd61cc6e89b438170cb27d3e4 On 01/11/2015 05:21 PM, Brian Dolbec wrote: > 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. 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