Bug 647231

Petr, this is you code, would you look on this?

Sure, seems the problem has been introduced by the following
commit: 

http://git.fedorahosted.org/git/?p=beakerlib.git;a=commitdiff;h=ab63f048

Where "readlink -f" is used to get the full path of the file to be
backed up. It expands to the real file name instead to keep the
symlink. I'll revert the change, which purpose seems to be only
"get rid of that long ugly regexp" (which worked better).

Created attachment 456421 [details]
Patch: backing up symlinks + test suite adjustment

Adding patch. Includes a small adjustment to the assertRun() to be
able to expect multiple values (as rlRun() does).

 * fixed symlink backup
 * added test coverage for the bug
 * tiny test suite adjustment [multiple expected values]

Please, review.

Thank you, Petr, for digging into this. Yes, it seems that replacing the readlink command with that ugly regexp solves the issue :-)

Great, thanks for verifying! Any objections to the subtle test
suite enhancement from anybody else? Otherwise I'll push.

(In reply to comment #5)
>Any objections to the subtle test
> suite enhancement from anybody else? Otherwise I'll push.

> -    if [ "$status" -eq "$expected" ]; then
> +    if [[ "$status" =~ ^$expected$ ]]; then

no objections

Thanks. Pushed to git:

http://git.fedorahosted.org/git/?p=beakerlib.git;a=commitdiff;h=63a93da

I have experienced the issue too. It would be cool if it is resolved.

Please note that symlink support is crucial in RHEL7. Adjusting bug summary

increasing priority/severity, I have several tests failing on RHEL7 because of deleted items in /var/run, rlFileRestore breaks the environment for other tests.

Pasting reproducer. Imagine /var/run instead of /tmp/run and you see why we are f****d up on RHEL7.


# mkdir -p /run/a /run/b
# ln -s /run /tmp/run
You have mail in /var/spool/mail/root
# ls -l /tmp/run
lrwxrwxrwx 1 root root 4 Jul 19 04:10 /tmp/run -> /run
# ls -l /tmp/run/
total 16
drwxr-xr-x 2 root root 4096 Jul 19 04:00 a
drwxr-xr-x 2 root root 4096 Jul 19 04:00 b
# . /usr/lib/beakerlib/beakerlib.sh
# rlJournalStart
# rlFileBackup /tmp/run/a 
# rmdir /tmp/run/a
# rlFileRestore
# ls -l /tmp/run
total 8
drwxr-xr-x 2 root root 4096 Jul 19 04:00 a
# ls -l /run
total 8
drwxr-xr-x 2 root root 4096 Jul 19 04:00 b

No I can see that the proposed patch is applied in beakerlib-1.5-1.el5 but the rlFileRestore is broken anyway

Following change fixed it for me, but I didn't do any detailed testing.

--- /usr/share/beakerlib/infrastructure.sh	2012-05-15 11:45:38.000000000 -0400
+++ /usr/share/beakerlib/infrastructure.sh.new	2012-07-19 04:33:42.000000000 -0400
@@ -302,6 +302,8 @@
     for file in "$@"; do
         # convert relative path to absolute, remove trailing slash
         file=$(echo "$file" | sed "s|^\([^/]\)|$PWD/\1|" | sed "s|/$||")
+	 # follow symlinks
+	 file=$( readlink -n -f $file )
         path=$(dirname "$file")
 
         # bail out if the file does not exist

Previous fix breaks the behavior expected in #c0. We had a discussion with Petr and proper symlink handling seems to be a challenge (backup of symlinks in the directory tree and also backup of their targets). Anyway, this is a better quick fix for "symlink in parent" issue, preserving the symlink backup behavior. This should be enough to fix bad behavior on RHEL7 and should not break anything. :-) Could you please review it ASAP?

--- /usr/share/beakerlib/infrastructure.sh	2012-05-15 11:45:38.000000000 -0400
+++ /usr/share/beakerlib/infrastructure.sh.new	2012-07-19 06:02:19.000000000 -0400
@@ -302,7 +302,10 @@
     for file in "$@"; do
         # convert relative path to absolute, remove trailing slash
         file=$(echo "$file" | sed "s|^\([^/]\)|$PWD/\1|" | sed "s|/$||")
+        # follow symlinks
         path=$(dirname "$file")
+        path=$(readlink -n -f $path)
+        file=$path/$(basename $file)
 
         # bail out if the file does not exist
         if ! [ -e "$file" ]; then



check that we do not rewrite symlink in the parent

# . /usr/lib/beakerlib/beakerlib.sh
# ls -l /tmp/run
lrwxrwxrwx 1 root root 4 Jul 19 04:31 /tmp/run -> /run
# ls -l /tmp/run/
total 16
drwxr-xr-x 2 root root 4096 Jul 19 04:31 a
drwxr-xr-x 2 root root 4096 Jul 19 04:30 b
# rlJournalStart
# rlFileBackup /tmp/run/a
# rmdir /tmp/run/a
# rlFileRestore
# ls -l /tmp/run
lrwxrwxrwx 1 root root 4 Jul 19 04:31 /tmp/run -> /run
# ls -l /tmp/run/
total 16
drwxr-xr-x 2 root root 4096 Jul 19 04:31 a
drwxr-xr-x 2 root root 4096 Jul 19 04:30 b


check that we are able to backup symlink itself

# rlJournalStart
# ls -l /tmp/run
lrwxrwxrwx 1 root root 4 Jul 19 04:31 /tmp/run -> /run
# ls -l /tmp/run/
total 16
drwxr-xr-x 2 root root 4096 Jul 19 04:31 a
drwxr-xr-x 2 root root 4096 Jul 19 04:30 b
# rlFileBackup /tmp/run
# rm /tmp/run
rm: remove symbolic link `/tmp/run'? y
# rlFileRestore 
# ls -l /tmp/run/
total 16
drwxr-xr-x 2 root root 4096 Jul 19 04:31 a
drwxr-xr-x 2 root root 4096 Jul 19 04:30 b
# ls -l /tmp/run
lrwxrwxrwx 1 root root 4 Jul 19 06:07 /tmp/run -> /run


This is just to illustrate the no-follow symlink backup problem:

# rlJournalStart
# ls -l /tmp/test
total 8
lrwxrwxrwx 1 root root 4 Jul 19 06:12 run -> /run
-rw-r--r-- 1 root root 0 Jul 19 06:11 x
# ls -l /tmp/test/
total 8
lrwxrwxrwx 1 root root 4 Jul 19 06:12 run -> /run
-rw-r--r-- 1 root root 0 Jul 19 06:11 x
# cat /tmp/test/run/a/y
1
# rlFileBackup /tmp/test
# echo 2 > /tmp/test/run/a/y
# rm -rf /tmp/test
# rlFileRestore 
# ls -l /tmp/test
total 8
lrwxrwxrwx 1 root root 4 Jul 19 06:15 run -> /run
-rw-r--r-- 1 root root 0 Jul 19 06:11 x
# ls -l /tmp/test/
total 8
lrwxrwxrwx 1 root root 4 Jul 19 06:15 run -> /run
-rw-r--r-- 1 root root 0 Jul 19 06:11 x
# cat /tmp/test/run/a/y
2
This is bad! I want the file to be restored with content 1!!!

Karel, can you please included a test for the fix as well? Thanks

Sure, I will prepare a git patch with both.

Created attachment 599125 [details]
proposed fix for issue with symlink in parent dir, including test case

Attached proposed patch. I hope this is my last comment in this bug today. :-)

The tests FAIL for me after applying:

 [ INFO ] Running test_rlFileBackup_SymlinkInParent
 [ PASS ] Preparing tmp directory
 [ PASS ] Preparing target directory
 [ PASS ] Preparing linked directory
 [ FAIL ] Backing up the link/file1
 [ PASS ] Removing link/file1
 [ FAIL ] Restoring the file1
 [ PASS ] Testing that link points to target
 [ PASS ] Testing that link/file1 was restored
 [ PASS ] Testing that link/file2 is still present
 [ PASS ] Clean up

(In reply to comment #20)
> The tests FAIL for me after applying:

Please disregard. rlBackup* & friends apparently need root. Mea culpa.

Created attachment 599321 [details]
proposed fix for issue with symlink in parent dir, including test case (updated)

Apparently other test cases are ignoring rlFileBackup/Restore errors if the test is executed as non-root. I have updated the patch accordingly.

http://git.fedorahosted.org/git?p=beakerlib.git;a=commit;h=2de1becf573531d449701491d505abf6c9474dce

*** Bug 839716 has been marked as a duplicate of this bug. ***

Thanks for the fix, Karel.

beakerlib-1.6-1 is already in fedora

This bug appears to have been reported against 'rawhide' during the Fedora 19 development cycle.
Changing version to '19'.

(As we did not run this process for some time, it could affect also pre-Fedora 19 development
cycle bugs. We are very sorry. It will help us with cleanup during Fedora 19 End Of Life. Thank you.)

More information and reason for this action is here:
https://fedoraproject.org/wiki/BugZappers/HouseKeeping/Fedora19