CAF::Path: add methods to manage links #225

jouvin · 2017-03-04T22:10:03Z

3 methods added to manage symlinks:

is_symlink(): returns true if the argument is a symlink
symlink(): creates a new symlink or updates an existing one. If the symlink path already exists and is not a symlink, returns an error.
hardlink(): same as symlink() for hardlinks.
has_hardlinks(): test if a file has hardlinks (several entries refering to the same inode)
is_hardlink(): compare to paths and check if they refer to the same inode
- Note that there it is impossible to say if a file is a hardlink in itself... A hardlink is just a normal file.

stdweird · 2017-03-05T08:50:12Z

src/main/perl/Path.pm

@@ -346,6 +346,54 @@ sub any_exists
    return $path && (-e $path || -l $path);
 }

+=item is_symlink
+
+Test if C<path> is a symlink.


pod should mention this reutrn true for broken symlink

stdweird · 2017-03-05T08:53:08Z

src/main/perl/Path.pm

+
+=cut
+
+sub make_symlink


like all other methods in CAF::Path, this has to support NoAction (incl unittests). i'd suggest to check the LC code and use that for now. we can replace LC later.

i would also rename it to just symlink. the result of this method should be a symlink, it's possible none should be created.

I looked at directory() and I don't see NoAction being handled (NoAction is handled only for temporary directories)... This may explain quattor/maven-tools#139... Or is NoAction handled by LC?

NoAction is handled in the LC call, the Noaction flag is passed in https://github.com/quattor/CAF/blob/master/src/main/perl/Path.pm#L252; only functionality not in LC requires explicit NoAction code.
best point to start is to look how symlinking is handled in LC.

This is what I was doing and I have the feeling that the method in CAF::Path can be almost as simple as calling LC::Check::symlink... Good news!

stdweird · 2017-03-05T08:55:36Z

src/main/perl/Path.pm

+{
+    my ($self, $target, $path) = @_;
+    my $status = SUCCESS;     # Assume success
+    unless ($path) {


i propose to keep the same style as the rest of CAF::Path, so don't check this. lets assume that people who use this know what the are doing.

stdweird · 2017-03-05T08:56:32Z

src/main/perl/Path.pm

+    unless ($target) {
+        $self->error("create_symlink(): 'target' argument missing");
+    }
+


you have too untaint path and target, look at eg the directory code (you can ignore the tempdir part)

stdweird · 2017-03-05T08:57:15Z

src/main/perl/Path.pm

+
+    if ( $self->is_symlink($path) ) {
+        $self->debug(1, "Removing symlink $path");
+        unlink ($path);


use CAF::Path methods to remove things.

stdweird · 2017-03-05T08:57:39Z

src/main/perl/Path.pm

+    }
+
+    if ( $self->is_symlink($path) ) {
+        $self->debug(1, "Removing symlink $path");


should only remove of something target is different

stdweird · 2017-03-05T08:58:19Z

src/main/perl/Path.pm

+        return;
+    }
+
+    $status = symlink $target, $path;


use LC method

and make (or try to make) the parent dir first

stdweird · 2017-03-05T08:59:14Z

src/main/perl/Path.pm

+    }
+
+    $status = symlink $target, $path;
+    $status = SUCCESS if $status;


success should be eturned when nothing has changed (eg link already existed and pointed to same target).
return CHANGED if a link was actually created

stdweird · 2017-03-05T09:02:34Z

src/main/perl/Path.pm

+        $self->debug(1, "Removing symlink $path");
+        unlink ($path);
+    } elsif ( $self->any_exists($path) ) {
+        $self->error("$path already exists and is not a symlink");


report failures with fail, eg. return $self->fail("$path already exists and is not a symlink");
in particular, CAF should try to minimise the reported errors. there might be a good reason that something fails, it's up to the component to promote a failure as an error (since any error means component failed).

stdweird · 2017-03-05T09:05:00Z

@jouvin please have a look at eg the directory code. try to use the wrapped LC calls or existing CAF::Path methods.
there has to be support for NoAction and there have to be unittests for NoAction

jouvin · 2017-03-05T10:56:48Z

@stdweird thanks for all the remarks, I'll try to improve the code later today...

jouvin · 2017-03-05T18:23:41Z

@stdweird @jrha @ned21 any opinion on the argument order (link path versus target)? Perl symlink is following ln command with target first, link second. LC::Check::symlink is following the order I had in mind initially: link first, target second. As CAF::Path::symlink is mainly a wrapper over LC::Check::symlink, I'm inclined to follow its order but I don't have a strong opinion. And I am happy to change if you think the Perl symlink order is less confusion/more appropriate...

jouvin · 2017-03-05T19:14:22Z

Reimplemented over LC::Check:link() as suggested by @stdweird. Some problems in tests, to be fixed later...

ned21 · 2017-03-05T21:13:00Z

@jouvin - tough question! Following LC::Check::symlink makes it easier to refactor code in the short term but long term following symlink() is better for everyone and less confusing. Since I think we want to take a long term view, I vote for following the Perl and UNIX symlink ordering.

jouvin · 2017-03-05T22:54:33Z

I think the code is now in pretty good shape. We may just need to adjust the argument order and in fact part of the refactoring we want to do after having these methods is replacing symlink by CAF::Path. For this @ned21 proposal is probably less confusing, I agree...

I am still struggling with the tests: @stdweird verify_exception() is failing and I don't really understand what it is supposed to do (and I have not seen any comment in the code to help with it!). Any explanation much appreciated! Once tests are working for symlink(), I still need to add tests for hardlink() and tests for NoAction but it should be easy.

stdweird

@jouvin i'm reading the unittests, my head is starting to spin . so i vote for perl/linux ln usage and variable nameing, so ln TARGET LINK_NAME (and i'd use $link_name instead of $path)

stdweird · 2017-03-06T07:27:59Z

src/main/perl/Path.pm

+sub _make_link
+{
+    my ($self, $path, $target, %opts) = @_;
+    $path = $self->_untaint_path($path, "symlink") || return;


should be symlink path as message, and why isn't the target untainted?

Mistake of mine! I thought I did it!

stdweird · 2017-03-06T07:28:41Z

src/main/perl/Path.pm

+
+    $self->_reset_exception_fail('symlink');
+
+    my $status = LC::Check::link($path, $target, %opts);


use the wrapper to call LC, eg https://github.com/quattor/CAF/blob/master/src/main/perl/Path.pm#L532

stdweird · 2017-03-06T07:30:14Z

src/main/perl/Path.pm

+        return ($status ? CHANGED : SUCCESS);
+    } else {
+        my $link_type = $opts{hard} ? "hardlink" : "symlink";
+        $self->fail("Failed to create $link_type $path (target=$target)");


return $self->fail()

stdweird · 2017-03-06T07:36:37Z

src/main/perl/Path.pm

+
+Create a hardlink C<path> whose target is C<target>.
+
+Returns an error if C<path> already exists and is not a 


Returns an error -> Returns undef on failure and sets the fail attribute

stdweird · 2017-03-06T07:38:20Z

src/main/perl/Path.pm

+
+Create a symlink C<path> whose target is C<target>.
+
+Returns an error if C<path> already exists and is not a 


same Returns error as above

stdweird · 2017-03-06T07:45:06Z

src/main/perl/Path.pm

+        $opts{nocheck} = 1;
+    }
+    $self->verbose("nocheck=$opts{nocheck}");
+    my $status = $self->_make_link($path, $target, %opts);


simply return $self->_make_links

stdweird · 2017-03-06T07:47:24Z

src/test/perl/path.t

@@ -108,9 +108,9 @@ sub verify_exception
    } else {
        ok(! $ec_check->error(), "Error reset after $msg");
    };
-    if ($noreset) {
+    if ($noreset && $mc->{fail}) {


please don't modify existing tests. using the LC wrapper is probably enough to make this pass.

I didn't really modify existing tests but made a commit explictly dedicated to this as I think the test is wrong... I don't like that you may want to test/print something undefined. It doesn't make sense.

it changes the logic of the test, from one condition to two. if $mc->{fail} is not defined, something is wrong and test will fail, as it is supposed to.

I understand that my problem was coming from the fact I was not using _safe_eval(). Nevertheless, I think we should protect in some way against an undef argument rather than adding a warning to the error...

BTW, I think there is a typo at the end of _safe_eval(). Should be chomp $res_text and verbose() should print $res_text, shouldn't it?

nice catch on the res_txt! (i'm waiting for quattor/maven-tools#130 to catch these sort of errors)

if you want to, you can make the tests more robust, but not by changing the logic like this.

I don't think it is changing the logic, at least it was not the goal! I need to reread what I did may be too quickly...

You are right that $noreset was not honored when $mc->{fail} was undefined. Should be fixed now and I really think that nothing is changed in the logic, just meaningless like() are not executed. @stdweird do you agree?

not really. now there's no catch-all else block anymore. in particular i don't see what you gain with this. what did the error message look like when $mc->{fail} is undefined?

@jouvin this still needs to be addressed. i would revert the changes and keep the old logic. it's in any case more important that it fails rather that the message is readable/human friendly

@jouvin ping

stdweird · 2017-03-06T07:54:01Z

src/test/perl/path.t

+makefile("$basetest/$target_file1");
+makefile($target_file2);
+my %opts;
+is($mc->symlink($dirlink, $target_directory), CHANGED, "directory symlink created");


add a symlink test $mc->is_symlink( for every symlink created

the main reason for this is so you can use the same/similar code block under noaction with minimal changes

Not sure to understand... Why is_symlink() is improving the ability to use the same code with NoAction?

the code block for NoAction will be the the smae, except where you now have/would have ok($mc->is_symlink,..), you wil have ok(!$mc->is_symlink; since noaction is not supposed to create actual symlink (unless keepsstate is set).

stdweird · 2017-03-06T07:55:41Z

src/test/perl/path.t

+is($mc->symlink($filelink, $target_file2), SUCCESS, "file symlink already exists: nothing done");
+is($mc->symlink($filelink, $target_file1), CHANGED, "file symlink already exists: nothing done");
+my $link_status = $mc->symlink($target_file2, $target_file1);
+ok(! $link_status, "existing file not replaced by a symlink");


it has to return undef, so use ok(!defined($link_status))

and the message should also have symlink failed failed, reason

stdweird · 2017-03-06T07:58:10Z

src/test/perl/path.t

+is($mc->symlink($filelink, $target_file1), CHANGED, "file symlink already exists: nothing done");
+my $link_status = $mc->symlink($target_file2, $target_file1);
+ok(! $link_status, "existing file not replaced by a symlink");
+is($mc->{fail}, "Failed to create symlink $target_file2 (target=$target_file1)");


add a message to the test symlink failed because existing file not replaced by a symlink sets correct fail attrribute

stdweird · 2017-03-06T08:05:30Z

src/test/perl/path.t

+is($mc->{fail}, "Failed to create symlink $target_file2 (target=$target_file1)");
+$opts{force} = 1;
+is($mc->symlink($target_file2, $target_file1, %opts), CHANGED, "existing file replaced by a symlink (force option set)");
+ok(! $mc->symlink("$basetest/$target_directory", $target_file1, %opts), "directory not replaced by a symlink (force option set)");


ok, i reread the pod, and this is implied, but it would be better to make it explicit in the pod (or use the not replacing of a directory as an exmaple in the pod).

same remark as above, here (and everywhere else) add an explicit test that nothing was created when it shouldn't have.

stdweird · 2017-03-06T08:08:21Z

src/test/perl/path.t

+$opts{force} = 1;
+is($mc->symlink($target_file2, $target_file1, %opts), CHANGED, "existing file replaced by a symlink (force option set)");
+ok(! $mc->symlink("$basetest/$target_directory", $target_file1, %opts), "directory not replaced by a symlink (force option set)");
+$opts{nocheck} = 0;


ahhh, this is confusing. so the pod is wrong?

stdweird · 2017-03-06T08:11:28Z

src/test/perl/path.t

+$opts{nocheck} = 0;
+ok(! $mc->symlink($brokenlink, "really_really_missing", %opts), "broken symlink not created (target existence enforced)");
+delete $opts{nocheck};
+is($mc->symlink($brokenlink, "really_missing"), CHANGED, "broken symlink created");


you have to test this, either use !-e $brokenlink && -l $brokenlink or a combination of CAF::Path methods

jouvin · 2017-03-07T22:36:16Z

@ned21 @jrha @stdweird Adding more unit tests for hardlinks (in particular hardlinks to symlinks, something allowed by ln command whatever the symlink target is), I found 2 bugs in in LC::Check::symlink. See quattor/LC#17.
Basically we have the choice between fixing LC::Check (easy fix, see quattor/LC#16) or preventing some (probably marginal) use cases in CAF::Path. What do you think?

jouvin · 2017-03-10T13:25:13Z

PR should be almost ok but I still have to figure out the problem with verify_exceptions(). Role of the noreset argument is unclear for me. @stdweird could you give me some info?

jouvin · 2017-03-11T18:38:23Z

I realized that we had a test-specific version of LC::Check (and Process and Files). It caused some tests to fail because the version of the module was not advertized but CAF::Path had a requirement on it. It is unclear for me why this was happening for certains tests only (seems to be those for modules using CAF::Path, except CAF::Path itself). Do we really need these test-specific versions of LC modules. Just removing them is unfortunately not working, so there are probably some subtilties...

jouvin · 2017-03-11T19:07:17Z

Ready for final review 😄

stdweird · 2017-03-13T20:40:17Z

src/main/perl/Path.pm

+is honored (overrides C<NoAction> if true). One important
+difference is the order of the arguments: C<CAF::Path:_make_link>
+and the methods based on it are following the Perl C<symlink>
+(and Shell C<ln>) argument order.


ln has nothing to do with Shell, it's part of base utils on a lot of Unix systems.

stdweird · 2017-03-13T20:40:40Z

src/main/perl/Path.pm

+and the methods based on it are following the Perl C<symlink>
+(and Shell C<ln>) argument order.
+
+This in internal method, not supposed to be called directly.


in -> is an

stdweird · 2017-03-13T20:41:55Z

src/main/perl/Path.pm

+If C<link_path> exists and is a file, it is updated. 
+C<target> must exist (C<check> flag available in symlink()
+is ignored for hardlinks) and it must reside in the same 
+file system as C<link_path>. If C<target_path> is a


file system -> filesystem

stdweird · 2017-03-13T20:49:11Z

src/test/perl/path.t


 ok(! $mc->directory_exists($basetestfile), "directory_exists false on created file");
 ok($mc->any_exists($basetestfile), "any_exists true on created file");
 ok($mc->file_exists($basetestfile), "file_exists true on created file");
+ok(! $mc->is_symlink($basetestfile), "is_symlink false on created file");
+ok(! $mc->has_hardlinks($basetestfile), "has_hardlinks false on created file");
+is($mc->is_hardlink($basetestfile, $basetestfile), 0, "is_hardlink false with a non-hardlinked file");


this is false because the files are the same. should change the message

stdweird · 2017-03-13T20:51:51Z

src/test/perl/path.t

+ok($mc->file_exists($target_file2) && ! $mc->is_symlink($target_file2), "File $target_file2 has not be replaced by a symlink");
+$opts{force} = 1;
+$CAF::Object::NoAction = 1;
+is($mc->symlink($target_file1, $target_file2, %opts), CHANGED, "existing file replaced by a symlink (force option set)");


adapt message. this is force=1, but also NoAction=1; so the file shouldn't be replaced.

stdweird · 2017-03-13T20:56:25Z

@jouvin more minor fixes. can you address them in a separate commit(s)? you can squash them before merging (it would make the review easier)

- Protect against uninitialized variables - Clarify messages

Fixes quattor#161

- Creates or updates a symlink or hardlink respectively - Wrapper over LC::Check::link()

- has_hardlinks(): returns the number of hardlinks of a file - is_hardlink(): test if two paths refer to the same file (inode) - CAF::Path requires LC::Check >= 1.22

- CAF::Path has a requirement on LC::Check version that causes the tests for other CAF modules using CAF::Path to fail.

CAF::Path: is_symlink() method added

b84520a

Fixes quattor#217

jouvin added the enhancement label Mar 4, 2017

jouvin added this to the 17.3 milestone Mar 4, 2017

jouvin mentioned this pull request Mar 4, 2017

Test::Quattor: mock the new symlink-related methods quattor/maven-tools#141

Closed

jouvin force-pushed the symlink branch 2 times, most recently from 27c4c58 to 9cdfd6f Compare March 4, 2017 22:26

stdweird reviewed Mar 5, 2017

View reviewed changes

jouvin force-pushed the symlink branch from 9cdfd6f to fd7599c Compare March 5, 2017 19:11

jouvin changed the title ~~CAF::Path: add methods to manage symlinks~~ CAF::Path: add methods to manage links Mar 5, 2017

jouvin force-pushed the symlink branch from fd7599c to aa1d72c Compare March 5, 2017 19:35

jouvin force-pushed the symlink branch 2 times, most recently from a21e6cc to aaac8fa Compare March 5, 2017 22:49

jouvin force-pushed the symlink branch 2 times, most recently from 9d86d33 to ea7aafd Compare March 6, 2017 07:14

stdweird reviewed Mar 6, 2017

View reviewed changes

jouvin force-pushed the symlink branch 4 times, most recently from 9e778fa to e5907a4 Compare March 7, 2017 22:23

This was referenced Mar 7, 2017

LC::Check: fix link() for hardlink creation quattor/LC#16

Merged

LC::Check::link 17.2: hardlink management is buggy quattor/LC#17

Closed

jouvin force-pushed the symlink branch from e5907a4 to 47d0477 Compare March 10, 2017 13:23

jouvin force-pushed the symlink branch from a3e134c to d19ba96 Compare March 11, 2017 17:39

jouvin force-pushed the symlink branch from 5a97953 to c1e3099 Compare March 11, 2017 18:49

jouvin force-pushed the symlink branch 2 times, most recently from bf48b16 to fb59f93 Compare March 12, 2017 15:05

jouvin mentioned this pull request Mar 12, 2017

Test::Quattor: mock CAF::Path link-related functions quattor/maven-tools#142

Merged

stdweird reviewed Mar 13, 2017

View reviewed changes

jouvin added 5 commits March 16, 2017 23:14

CAF::Path: fix verify_exceptions() in path.t

9392802

- Protect against uninitialized variables - Clarify messages

CAF::Path:: fix missing rmtree argument in path.t

dfc2521

Fixes quattor#161

CAF::Path: add symlink() and hardlink() method

0986669

- Creates or updates a symlink or hardlink respectively - Wrapper over LC::Check::link()

CAF::Path: add has_hardlinks() and is_hardlink() methods

6bb6200

- has_hardlinks(): returns the number of hardlinks of a file - is_hardlink(): test if two paths refer to the same file (inode) - CAF::Path requires LC::Check >= 1.22

Ensure that LC::Check used by unit tests advertize its version

a07ad67

- CAF::Path has a requirement on LC::Check version that causes the tests for other CAF modules using CAF::Path to fail.

jouvin force-pushed the symlink branch from 7b1507c to a07ad67 Compare March 16, 2017 22:23

stdweird approved these changes Mar 17, 2017

View reviewed changes

stdweird merged commit 9d804bc into quattor:master Mar 17, 2017


		$self->_reset_exception_fail('symlink');

		my $status = LC::Check::link($path, $target, %opts);


		Create a hardlink C<path> whose target is C<target>.

		Returns an error if C<path> already exists and is not a


		Create a symlink C<path> whose target is C<target>.

		Returns an error if C<path> already exists and is not a

CAF::Path: add methods to manage links #225

CAF::Path: add methods to manage links #225

Conversation

jouvin commented Mar 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jouvin Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stdweird Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stdweird commented Mar 5, 2017

jouvin commented Mar 5, 2017

jouvin commented Mar 5, 2017

jouvin commented Mar 5, 2017

ned21 commented Mar 5, 2017

jouvin commented Mar 5, 2017

stdweird left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jouvin Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jouvin commented Mar 7, 2017 • edited Loading

jouvin commented Mar 10, 2017

jouvin commented Mar 11, 2017

jouvin commented Mar 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stdweird commented Mar 13, 2017

jouvin commented Mar 4, 2017 •

edited

Loading

jouvin Mar 5, 2017 •

edited

Loading

stdweird Mar 5, 2017 •

edited

Loading

jouvin Mar 6, 2017 •

edited

Loading

jouvin commented Mar 7, 2017 •

edited

Loading