From 03c3be7125154a21241fe9fe3d74a7395cae8ad5 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Jul 26 2016 20:20:54 +0000 Subject: [PATCH 1/5] stub --- diff --git a/koji/util.py b/koji/util.py index 80e511f..9a73c82 100644 --- a/koji/util.py +++ b/koji/util.py @@ -26,6 +26,7 @@ import os import os.path import re import resource +import shutil import stat import sys import time @@ -316,6 +317,13 @@ def rmtree(path): continue os.rmdir(dirpath) + +def safe_move(src, dst): + """Wrapper around shutil.move with additional safety""" + + pass + + def _relpath(path, start=getattr(os.path, 'curdir', '.')): """Backport of os.path.relpath for python<2.6""" From 720c6479594a9a703db1de6c3bc2fafa3d20775f Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Jul 26 2016 20:20:54 +0000 Subject: [PATCH 2/5] just wrap shutil.move for now --- diff --git a/koji/util.py b/koji/util.py index 9a73c82..461ccce 100644 --- a/koji/util.py +++ b/koji/util.py @@ -318,10 +318,20 @@ def rmtree(path): os.rmdir(dirpath) -def safe_move(src, dst): - """Wrapper around shutil.move with additional safety""" +def safer_move(src, dst): + """Rename if possible, copy+rm otherwise - pass + Behavior is similar to shutil.move + + Unlike move, src is /always/ moved from src to dst. If dst is an existing + directory, then an error is raised. + """ + if os.path.exists(dst): + raise koji.GenericError, "Destination exists: %s" % dst + elif os.path.islink(dst): + raise koji.GenericError, "Destination is a symlink: %s" % dst + # TODO - use locking to do a better job of catching races + shutil.move(src, dst) def _relpath(path, start=getattr(os.path, 'curdir', '.')): From 8bd4b49822f01fecdef653ce14e8cbb98cc9244a Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Jul 26 2016 20:20:54 +0000 Subject: [PATCH 3/5] use safer_move on hub --- diff --git a/hub/kojihub.py b/hub/kojihub.py index c37c445..1e9d759 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -41,6 +41,7 @@ from koji.util import md5_constructor from koji.util import sha1_constructor from koji.util import dslice from koji.util import multi_fnmatch +from koji.util import safer_move import os import re import rpm @@ -5147,7 +5148,7 @@ def import_build_log(fn, buildinfo, subdir=None): raise koji.GenericError("Error importing build log. %s already exists." % final_path) if os.path.islink(fn) or not os.path.isfile(fn): raise koji.GenericError("Error importing build log. %s is not a regular file." % fn) - os.rename(fn,final_path) + safer_move(fn, final_path) os.symlink(final_path,fn) def import_rpm_file(fn,buildinfo,rpminfo): @@ -5603,7 +5604,7 @@ def import_old_image(old, name, version): raise koji.GenericError("Error importing build log. %s already exists." % final_path) if os.path.islink(logsrc) or not os.path.isfile(logsrc): raise koji.GenericError("Error importing build log. %s is not a regular file." % logsrc) - os.rename(logsrc, final_path) + safer_move(logsrc, final_path) os.symlink(final_path, logsrc) return binfo @@ -5774,7 +5775,7 @@ def _import_archive_file(filepath, destdir): if os.path.islink(filepath) or not os.path.isfile(filepath): raise koji.GenericError("Error importing archive file, %s is not a regular file" % filepath) koji.ensuredir(destdir) - os.rename(filepath, final_path) + safer_move(filepath, final_path) os.symlink(final_path, filepath) def _generate_maven_metadata(mavendir): @@ -7891,7 +7892,7 @@ def importImageInternal(task_id, build_id, imgdata): raise koji.GenericError("Error importing build log. %s already exists." % final_path) if os.path.islink(logsrc) or not os.path.isfile(logsrc): raise koji.GenericError("Error importing build log. %s is not a regular file." % logsrc) - os.rename(logsrc, final_path) + safer_move(logsrc, final_path) os.symlink(final_path, logsrc) # record all of the RPMs installed in the image(s) @@ -10977,7 +10978,7 @@ class HostExports(object): for relpath in [srpm] + rpms: fn = "%s/%s" % (uploadpath,relpath) dest = "%s/%s" % (dir,os.path.basename(fn)) - os.rename(fn,dest) + safer_move(fn, dest) os.symlink(dest,fn) if logs: for key, files in logs.iteritems(): @@ -10989,7 +10990,7 @@ class HostExports(object): for relpath in files: fn = "%s/%s" % (uploadpath,relpath) dest = "%s/%s" % (logdir,os.path.basename(fn)) - os.rename(fn,dest) + safer_move(fn, dest) os.symlink(dest,fn) def moveMavenBuildToScratch(self, task_id, results, rpm_results): @@ -11012,7 +11013,7 @@ class HostExports(object): src = os.path.join(koji.pathinfo.task(results['task_id']), relpath) dest = os.path.join(destdir, relpath) koji.ensuredir(os.path.dirname(dest)) - os.rename(src, dest) + safer_move(src, dest) os.symlink(dest, src) if rpm_results: for relpath in [rpm_results['srpm']] + rpm_results['rpms'] + \ @@ -11021,7 +11022,7 @@ class HostExports(object): relpath) dest = os.path.join(destdir, 'rpms', relpath) koji.ensuredir(os.path.dirname(dest)) - os.rename(src, dest) + safer_move(src, dest) os.symlink(dest, src) def moveWinBuildToScratch(self, task_id, results, rpm_results): @@ -11039,7 +11040,7 @@ class HostExports(object): filename = os.path.join(koji.pathinfo.task(results['task_id']), relpath) dest = os.path.join(destdir, relpath) koji.ensuredir(os.path.dirname(dest)) - os.rename(filename, dest) + safer_move(filename, dest) os.symlink(dest, filename) if rpm_results: for relpath in [rpm_results['srpm']] + rpm_results['rpms'] + \ @@ -11048,7 +11049,7 @@ class HostExports(object): relpath) dest = os.path.join(destdir, 'rpms', relpath) koji.ensuredir(os.path.dirname(dest)) - os.rename(filename, dest) + safer_move(filename, dest) os.symlink(dest, filename) def moveImageBuildToScratch(self, task_id, results): @@ -11069,7 +11070,7 @@ class HostExports(object): dest = os.path.join(destdir, img) koji.ensuredir(destdir) logger.debug('renaming %s to %s' % (src, dest)) - os.rename(src, dest) + safer_move(src, dest) os.symlink(dest, src) if sub_results.has_key('rpmresults'): rpm_results = sub_results['rpmresults'] @@ -11079,7 +11080,7 @@ class HostExports(object): rpm_results['task_id']), relpath) dest = os.path.join(destdir, 'rpms', relpath) koji.ensuredir(os.path.dirname(dest)) - os.rename(src, dest) + safer_move(src, dest) os.symlink(dest, src) def initBuild(self,data): @@ -11694,7 +11695,7 @@ class HostExports(object): raise koji.GenericError, "File already in repo: %s" % dst #otherwise the desired hardlink already exists else: - os.link(filepath, dst) + safer_move(filepath, dst) def repoDone(self, repo_id, data, expire=False): """Move repo data into place, mark as ready, and expire earlier repos @@ -11724,7 +11725,7 @@ class HostExports(object): dst = "%s/%s" % (datadir, fn) if not os.path.exists(src): raise koji.GenericError, "uploaded file missing: %s" % src - os.link(src, dst) + safer_move(src, dst) os.unlink(src) if expire: repo_expire(repo_id) From 0e7c45a544aaacb78b6dc07c310b27cf41c51951 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Jul 26 2016 20:20:54 +0000 Subject: [PATCH 4/5] update raise syntax in safer_move() --- diff --git a/koji/util.py b/koji/util.py index 461ccce..7903979 100644 --- a/koji/util.py +++ b/koji/util.py @@ -327,9 +327,9 @@ def safer_move(src, dst): directory, then an error is raised. """ if os.path.exists(dst): - raise koji.GenericError, "Destination exists: %s" % dst + raise koji.GenericError("Destination exists: %s" % dst) elif os.path.islink(dst): - raise koji.GenericError, "Destination is a symlink: %s" % dst + raise koji.GenericError("Destination is a symlink: %s" % dst) # TODO - use locking to do a better job of catching races shutil.move(src, dst) From 952891ea2f3da28635eeacf45fc6d2f8384434ce Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Jul 26 2016 20:20:54 +0000 Subject: [PATCH 5/5] unit test for safer_move() --- diff --git a/tests/test_utils.py b/tests/test_utils.py index 6be5326..9f5b604 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,6 +1,8 @@ +import mock import unittest import koji +import koji.util class EnumTestCase(unittest.TestCase): @@ -31,3 +33,46 @@ class EnumTestCase(unittest.TestCase): """ Test slice access. """ test = koji.Enum(('one', 'two', 'three')) self.assertEquals(test[1:], ('two', 'three')) + + +class MiscFunctionTestCase(unittest.TestCase): + + @mock.patch('os.path.exists') + @mock.patch('os.path.islink') + @mock.patch('shutil.move') + def test_safer_move(self, move, islink, exists): + """Test safer_move function""" + src = '/FAKEPATH/SRC' + dst = '/FAKEPATH/DST' + + # good args + exists.return_value = False + islink.return_value = False + koji.util.safer_move(src, dst) + exists.assert_called_once_with(dst) + islink.assert_called_once_with(dst) + move.assert_called_once_with(src, dst) + + move.reset_mock() + islink.reset_mock() + exists.reset_mock() + + # existing dst + exists.return_value = True + with self.assertRaises(koji.GenericError): + koji.util.safer_move(src, dst) + exists.assert_called_once_with(dst) + move.assert_not_called() + + move.reset_mock() + islink.reset_mock() + exists.reset_mock() + + # symlink dst + exists.return_value = False + islink.return_value = True + with self.assertRaises(koji.GenericError): + koji.util.safer_move(src, dst) + exists.assert_called_once_with(dst) + islink.assert_called_once_with(dst) + move.assert_not_called()