Subject: bug#15173: closed (Re: bug#15173: [cp] --link overrides dereference settings)

From: [email-masked] (GNU bug Tracking System)
Date: Thu, 07 Nov 2013 23:00:04 +0000

Your bug report

#15173: [cp] --link overrides dereference settings

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
15173: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=15173
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#15173: [cp] --link overrides dereference settings
From: Bernhard Voelker <[email-masked]>
To: Gian Piero Carrubba <[email-masked]>
Cc: Eric Blake <[email-masked]>,Pádraig Brady <[email-masked]>, Jim Meyering <[email-masked]>, [email-masked]
Date: Thu, 07 Nov 2013 23:59:28 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

On 11/07/2013 03:09 PM, Gian Piero Carrubba wrote:
> * [Thu, Nov 07, 2013 at 10:14:05AM +0000] Pádraig Brady:
>> I.E. IMHO your current patch is fine and good.
> 
> Fair enough. In this case please also update the documentation.

Good idea, thanks.
I pushed it in your name (together with Padraig's notes regarding
NEWS and the test case):
http://git.sv.gnu.org/cgit/coreutils.git/commit/?id=bf6bf52d

I'm hereby marking the bug as done.

Thanks again.

Have a nice day,
Berny

Subject: [cp] --link overrides dereference settings
From: Gian Piero Carrubba <[email-masked]>
To: [email-masked]
Date: Fri, 23 Aug 2013 23:40:34 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

Please keep me in Cc: as I'm not subscribed.

I failed to find references to discussions about this (intended?) 
behaviour, so I'm filing this report. Please forgive me if I've missed 
something elementary.

$ echo testfile > file; ln -s file link; opts="-l -Ll -Hl";
for o in $opts; do cp $o link cp${o}; done;
ls -li

total 4
3358742 lrwxrwxrwx 4 gpiero gpiero 4 Aug 23 22:27 cp-Hl -> file
3358742 lrwxrwxrwx 4 gpiero gpiero 4 Aug 23 22:27 cp-Ll -> file
3358742 lrwxrwxrwx 4 gpiero gpiero 4 Aug 23 22:27 cp-l -> file
3358741 -rw-r--r-- 1 gpiero gpiero 9 Aug 23 22:27 file
3358742 lrwxrwxrwx 4 gpiero gpiero 4 Aug 23 22:27 link -> file

I would have expected both 'cp-Hl' and 'cp-Ll' to be hardlinked to 
'file', not to 'link'.

$ cp --version | head -n1
cp (GNU coreutils) 8.21

$ uname -a
Linux caimano 3.10-2-amd64 #1 SMP Debian 3.10.5-1 (2013-08-07) x86_64 GNU/Linux

$ readlink -f /lib/x86_64-linux-gnu/libc.so.6                                                               
/lib/x86_64-linux-gnu/libc-2.17.so

A tentative draft patch is attached. It still lacks unit tests and a way 
to determine if the implementation of linkat() supports flags.
But most of all, please note that I'm not fully aware of the 
implications (including portability issues) of such a change.

AFAICS, at least in one case the patch changes the behaviour of cp:

$ echo testfile > file; ln -s file link;
cp -l link cp-l.old; ../src/cp -l link cp-l.new;
ls -li

total 8
8912975 -rw-r--r-- 2 gpiero gpiero 9 Aug 23 22:58 cp-l.new
8913010 lrwxrwxrwx 2 gpiero gpiero 4 Aug 23 22:58 cp-l.old -> file
8912975 -rw-r--r-- 2 gpiero gpiero 9 Aug 23 22:58 file
8913010 lrwxrwxrwx 2 gpiero gpiero 4 Aug 23 22:58 link -> file

This is caused by the following code:

$ tail -n+1135 src/cp.c | head -n8
   if (x.dereference == DEREF_UNDEFINED)
     {
       if (x.recursive)
         /* This is compatible with FreeBSD.  */
         x.dereference = DEREF_NEVER;
       else
         x.dereference = DEREF_ALWAYS;
     }

Not sure why this snippet is there[0], but it seems to me that it leads 
to inconsistent behaviour:

$ echo testfile > file; ln -s file link;
cp link cp; cp -r link cp-r;
ls -li

total 8
3358743 -rw-r--r-- 1 gpiero gpiero 9 Aug 23 23:06 cp
3358744 lrwxrwxrwx 1 gpiero gpiero 4 Aug 23 23:06 cp-r -> file
3358741 -rw-r--r-- 1 gpiero gpiero 9 Aug 23 23:06 file
3358742 lrwxrwxrwx 1 gpiero gpiero 4 Aug 23 23:06 link -> file

I think it is counterintuitive that '--recursive' had effects on the 
referentiation of the link.[1]

Anyway, given that x.dereference is set to DEREF_ALWAYS, I think the 
modified behaviour of `cp -l` could be considered correct.

Thanks,
Gian Piero.

[0] I suspect this is for backward compatibility. In principle, I think 
DEREF_UNDEF should always set DEREF_NEVER, but this change would modify 
a common, and well spread, behaviour.

[1] The same stands for '--link', as demonstrated in the first example, 
when 'cp-l' has been created as a hardlink to the symlink (even if for 
different reasons: in that case DEREF_ALWAYS was in place, but linkat() 
didn't follow the symlink).
Fri Aug 23 20:29:12 CEST 2013  Gian Piero Carrubba <[email-masked]>
  * DRAFT
diff -rN -u old-coreutils/src/copy.c new-coreutils/src/copy.c
--- old-coreutils/src/copy.c	2013-08-23 22:16:15.938940800 +0200
+++ new-coreutils/src/copy.c	2013-08-23 22:16:15.942940823 +0200
@@ -1566,10 +1566,15 @@
    be created as a symbolic link to SRC_NAME.  */
 static bool
 create_hard_link (char const *src_name, char const *dst_name,
-                  bool replace, bool verbose)
+                  bool replace, bool verbose, bool dereference)
 {
-  /* We want to guarantee that symlinks are not followed.  */
-  bool link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, 0) != 0);
+  int flags;
+  if (dereference)
+    flags = AT_SYMLINK_FOLLOW;
+  else
+    flags = 0; /* We want to guarantee that symlinks are not followed.  */
+
+  bool link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, flags) != 0);
 
   /* If the link failed because of an existing destination,
      remove that file and then call link again.  */
@@ -1582,7 +1587,7 @@
         }
       if (verbose)
         printf (_("removed %s\n"), quote (dst_name));
-      link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, 0) != 0);
+      link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, flags) != 0);
     }
 
   if (link_failed)
@@ -1748,7 +1753,10 @@
                       /* Note we currently replace DST_NAME unconditionally,
                          even if it was a newer separate file.  */
                       if (! create_hard_link (earlier_file, dst_name, true,
-                                              x->verbose))
+                                              x->verbose,
+                                              x->dereference == DEREF_ALWAYS
+                                              || (x->dereference == DEREF_COMMAND_LINE_ARGUMENTS
+                                                  && command_line_arg)))
                         {
                           goto un_backup;
                         }
@@ -2078,7 +2086,10 @@
         }
       else
         {
-          if (! create_hard_link (earlier_file, dst_name, true, x->verbose))
+          if (! create_hard_link (earlier_file, dst_name, true, x->verbose,
+                                  x->dereference == DEREF_ALWAYS
+                                  || (x->dereference == DEREF_COMMAND_LINE_ARGUMENTS
+                                      && command_line_arg)))
             goto un_backup;
 
           return true;
@@ -2389,7 +2400,10 @@
            && !(LINK_FOLLOWS_SYMLINKS && S_ISLNK (src_mode)
                 && x->dereference == DEREF_NEVER))
     {
-      if (! create_hard_link (src_name, dst_name, false, false))
+      if (! create_hard_link (src_name, dst_name, false, false,
+                              x->dereference == DEREF_ALWAYS
+                              || (x->dereference == DEREF_COMMAND_LINE_ARGUMENTS
+                                  && command_line_arg)))
         goto un_backup;
     }
   else if (S_ISREG (src_mode)