Package: coreutils;
Reported by: Gian Piero Carrubba <gpiero <at> rm-rf.it>
Date: Fri, 23 Aug 2013 21:55:02 UTC
Severity: normal
Tags: fixed
Merged with 23120
Done: Bernhard Voelker <mail <at> bernhard-voelker.de>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Bernhard Voelker <mail <at> bernhard-voelker.de> To: Pádraig Brady <P <at> draigBrady.com> Cc: Gian Piero Carrubba <gpiero <at> rm-rf.it>, 15173 <at> debbugs.gnu.org Subject: bug#15173: [cp] --link overrides dereference settings Date: Wed, 30 Oct 2013 13:13:58 +0100 (CET)
> On October 29, 2013 at 7:27 PM Pádraig Brady <P <at> draigBrady.com> wrote:
> On 10/20/2013 02:11 AM, Bernhard Voelker wrote:
> >> I would have expected both 'cp-Hl' and 'cp-Ll' to be hardlinked to
> >> 'file', not to 'link'.
> >
> > I'd also say that the above is an error ... or at least contradicts
> > to what is documented in the code (copy.c, line 2377):
> >
> > Yet cp, invoked with '--link --no-dereference',
> > should not follow the link.
>
> Yes I would think that `ln -L` and `cp --link -L` should
> be equivalent in this regard.
s/ln -L/ln -n/, I guess.
> > Instead of passing 'flags' to linkat (of which I don't know if it works
> > on all platforms), I'd rather dereference the symlink manually:
> >
> > char *src_name_new = canonicalize_filename_mode (src_name, CAN_EXISTING);
>
> Actually since linkat() has defined semantics,
> _and thanks to gnulib, guaranteed semantics_
> we should just leverage the gnulib magic here,
> and use linkat with flags.
Thanks for the clarification. I changed it back to Gian's
version.
> > This is the result:
> >
> > $ : > file; ln -s file link
> > $ for opt in "" -l -lH -lL -lP -r ; do cp $opt link cp$opt ; done
> > $ ls -ldogi link file cp*
>
> Nice basis for a test.
Yes, but it's not enough: I exercised also some combinations with
-lrP -lrH -lrL, and the whole stuff again for a symlink pointing
to a directory.
It got a bit late yesterday, and I couldn't get the things sorted
to present here. All Ican say now is that there are still corner
cases where cp now behaves differently (e.g. "cp -l link2dir dst").
I hope I can get back here with the results this evening.
> > 15335993 -rw-r--r-- 1 0 Oct 20 03:09 cp
> > 15335992 lrwxrwxrwx 3 4 Oct 20 03:08 cp-l -> file
> > 15335991 -rw-r--r-- 3 0 Oct 20 03:08 cp-lH
> > 15335991 -rw-r--r-- 3 0 Oct 20 03:08 cp-lL
> > 15335992 lrwxrwxrwx 3 4 Oct 20 03:08 cp-lP -> file
> > 15335994 lrwxrwxrwx 1 4 Oct 20 03:09 cp-r -> file
> > 15335991 -rw-r--r-- 3 0 Oct 20 03:08 file
> > 15335992 lrwxrwxrwx 3 4 Oct 20 03:08 link -> file
>
> The above look correct.
Great, thanks.
> For comparison on FreeBSD 9.1
>
> 1755429 -rw-r--r-- 1 pbrady pbrady - 0 Oct 29 12:16 cp
> 1755427 -rw-r--r-- 5 pbrady pbrady - 0 Oct 29 12:16 cp-l
> 1755427 -rw-r--r-- 5 pbrady pbrady - 0 Oct 29 12:16 cp-lH
> 1755427 -rw-r--r-- 5 pbrady pbrady - 0 Oct 29 12:16 cp-lL
> 1755427 -rw-r--r-- 5 pbrady pbrady - 0 Oct 29 12:16 cp-lP
> 1755430 -rw-r--r-- 1 pbrady pbrady - 0 Oct 29 12:16 cp-r
> 1755427 -rw-r--r-- 5 pbrady pbrady - 0 Oct 29 12:16 file
> 1755428 lrwxr-xr-x 1 pbrady pbrady - 4 Oct 29 12:16 link -> file
Interesting.
> > BTW: tests/cp/same-file.sh now fails with the patch.
>
> Yes better to consolidate the functionality,
> before doing the tests to avoid awkward test churn.
yes, regarding from what I got with the tests mentioned above,
I'm still not convinced we're at that point.
> >>From fca06077e11d6338b35029bc61fbee5dbfb5ab66 Mon Sep 17 00:00:00 2001
> > From: Gian Piero Carrubba <gpiero <at> rm-rf.it>
> > Date: Sun, 20 Oct 2013 03:04:41 +0200
> > Subject: [PATCH] cp: fix --link --dereference [DRAFT]
> >
> > * src/copy.c (create_hard_link): Add a bool 'dereference' parameter,
> > and canonicalize the given src_name when dereference is true.
> > (copy_internal): Add a value for the above new parameter in all
> > three calls to create_hard_link: true if the src_name should be
> > dereferenced before creating the hard link, false otherwise.
> > * src/cp.c (main): after parsing the options, if x.dereference is
> > still DEFEF_UNDEFINED and the --links option was specified, initialize
>
> s/--links/--link/
Fixed, thanks.
> > diff --git a/src/copy.c b/src/copy.c
> > index f66ab46..fa4d734 100644
> > --- a/src/copy.c
> > +++ b/src/copy.c
[...]
> > @@ -1748,7 +1757,10 @@ copy_internal (char const *src_name, char const
> > *dst_name,
> > /* 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 +2090,10 @@ copy_internal (char const *src_name, char const
> > *dst_name,
> > }
> > 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 +2404,10 @@ copy_internal (char const *src_name, char const
> > *dst_name,
> > && !(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)))
>
> Lots and copy and paste there.
> Maybe use a helper like:
>
> static inline bool _GL_ATTRIBUTE_PURE
> should_dereference (const struct cp_options *x, bool cli_arg)
> {
> return x->dereference == DEREF_ALWAYS
> || (x->dereference == DEREF_COMMAND_LINE_ARGUMENTS
> && cli_arg);
> }
Good idea!
Thanks again for looking into this. It's really quite complex.
I'll come up with the test (old vs. new) cases soon.
Have a nice day,
Berny
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.