[sos-devel] [PATCH] plugins/__init__.py: Skip copying of broken symlink

[Kamalesh Babulal]

On 05/26/2015 03:26 PM, Bryn M. Reeves wrote:
> On Tue, May 26, 2015 at 01:27:43PM +0530, Kamalesh Babulal wrote:
>> Check if the srcpath exist before creating the symlink in
>> __copy_symlink(), so we could avoid trying to link from broken
>> (not existing) source.
> Please make sure to explain problems in enough detail that another
> user can re-create the problem and test your fix. The symlink code
> is designed to work correctly with broken symlinks (we want to
> include them in the report as they carry diagnostic value - i.e. a
> symlink is broken on the target system).
>
> The patch breaks this as it skips all symlinks where the link
> target is non-existent.

Thank you for the review.  Will follow it from next posting and append 
the bug reporting style to patch.

>> This patch fixes the following issue:
>> File "/usr/lib/python2.7/site-packages/sos/sosreport.py", line 1172, in collect plug.collect()
>> File "/usr/lib/python2.7/site-packages/sos/plugins/__init__.py", line 630, in collect
>> self._collect_copy_specs()
>> File "/usr/lib/python2.7/site-packages/sos/plugins/__init__.py", line 600, in _collect_copy_specs
>> self._do_copy_path(path)
>> File "/usr/lib/python2.7/site-packages/sos/plugins/__init__.py", line 319, in _do_copy_path
>> self._copy_symlink(srcpath)
>> File "/usr/lib/python2.7/site-packages/sos/plugins/__init__.py", line 271, in _copy_symlink
>> self._do_copy_path(absdest)
>> File "/usr/lib/python2.7/site-packages/sos/plugins/__init__.py", line 319, in _do_copy_path
>> self._copy_symlink(srcpath)
>> File "/usr/lib/python2.7/site-packages/sos/plugins/__init__.py", line 261, in _copy_symlink
>> self.archive.add_link(reldest, srcpath)
>> File "/usr/lib/python2.7/site-packages/sos/archive.py", line 198, in add_link
>> os.symlink(source, dest)
>> OSError: [Errno 17]
> Are you sure you don't have a symlink that points to itself here?
>
> That should be the only explanation for:
>
>    _copy_symlink() -> _do_copy_path() -> _copy_symlink()
>
> The patch should test that linkdest == srcpath (using canonical paths as
> appropriate), create the link appropriately (it should anyway) and then
> skip collection of the target since it is already present.
>

Agree, can we  can compare the absolute path of both srcpath/destination 
before copying and skip
if they are same, like in the updated patch below. The symlink is 
created before, so we do not skip creating
the link, like what I did in the previous patch (I will post the second 
version with right formating and explanation)

diff --git a/sos/plugins/__init__.py b/sos/plugins/__init__.py
index 2a944ed..6bb6016 100644
--- a/sos/plugins/__init__.py
+++ b/sos/plugins/__init__.py
@@ -268,12 +268,17 @@ class Plugin(object):
          # to absolute paths to pass to _do_copy_path.
          self._log_debug("normalized link target '%s' as '%s'"
                          % (linkdest, absdest))
-        self._do_copy_path(absdest)

-        self.copied_files.append({'srcpath': srcpath,
-                                  'dstpath': srcpath,
-                                  'symlink': "yes",
-                                  'pointsto': linkdest})
+        # skip the symlink pointing to itself
+        if (absdest != srcpath):
+            self._do_copy_path(absdest)
+
+            self.copied_files.append({'srcpath': srcpath,
+                                      'dstpath': srcpath,
+                                      'symlink': "yes",
+                                      'pointsto': linkdest})
+        else:
+            self._log_debug("link '%s' is a self link, skipping..." % 
linkdest)


Thanks,
Kamalesh.