fix data-loss issue with replays w/o checksums

[vassilit]

Closes: #672

See bug details in #672 (comment).

This is just a bugfix, but I feel we could fix a bit more, although I'm not sure if that wouldn't introduce a regression. I haven't checked all the consumers of file->digest.

In replay.c we do:

    file->digest = rm_digest_new(RM_DIGEST_EXT, 0);
[...]
    /* Fake the checksum using RM_DIGEST_EXT */
    JsonNode *cksum_node = json_object_get_member(object, "checksum");
    if(cksum_node != NULL) {
        const char *cksum = json_object_get_string_member(object, "checksum");
        if(cksum != NULL) {
            rm_digest_update(file->digest, (unsigned char *)cksum, strlen(cksum));
        }
    }

I would do instead:

    file->digest = NULL;
[...]
    /* Fake the checksum using RM_DIGEST_EXT */
    JsonNode *cksum_node = json_object_get_member(object, "checksum");
    if(cksum_node != NULL) {
        const char *cksum = json_object_get_string_member(object, "checksum");
        if(cksum != NULL) {
            file->digest = rm_digest_new(RM_DIGEST_EXT, 0);
            rm_digest_update(file->digest, (unsigned char *)cksum, strlen(cksum));
        }
    }

And reintroduce checks for file->digest == NULL ; that would avoid pointless allocations.
Another way to be safe would be to check for file->inode to confirm that they belong to the same group.
Are hardlinks the only situation when rmlint does not generate checksums ?

PS: @sahib if you still follow this repository, the bird theme is nice. It's a change from the overly formal naming in code of other projects :)

[RayOei]

I would suggest to move this to its own issue as a future improvement, so it doesn't get forgotten?

[RayOei]

@vassilit I haven't had the time, yet, to dive into this. Are there, besides the case described in #672, other scenarios you would want to have checked, looking at the tag?

[vassilit]

@vassilit I haven't had the time, yet, to dive into this. Are there, besides the case described in #672, other scenarios you would want to have checked, looking at the tag?

Just that it does not do silly thing, but as it passes all tests, I guess we're safe.

[RayOei]

I have tried several extended cases. All were fine 👍

I did bump into an issue with symbolic/soft links but can't find tests to explain how the behavior is expected to be.
If you take the error case and use soft instead of hard link the duplicate file is removed but the soft link is not. If you have many of those you are left with a lot dead links. That doesn't seem expected.

I note it here but I assume it needs its own issue? Unless this is intended?

[vassilit]

If you take the error case and use soft instead of hard link the duplicate file is removed but the soft link is not.

I will test it a bit later, in the meantime I do not merge this fix.

[vassilit]

I did bump into an issue with symbolic/soft links but can't find tests to explain how the behavior is expected to be. If you take the error case and use soft instead of hard link the duplicate file is removed but the soft link is not. If you have many of those you are left with a lot dead links. That doesn't seem expected.

Can you details the commands that you used to generate this case ? I'm not sure I understand.

[RayOei]

Create x number of duplicate files and define soft link (the error case uses hard links).
In this example: 4 files of which 2 duplicates with all 4 having a soft link

#!/bin/sh
echo "4 files - softlinks - 2 duplicates"
mkdir test_softlink_4_2
cd test_softlink_4_2

echo "test" > file1
echo "test" > file2
echo "testtesttest" > file3
echo "testtesttest" > file4

ln -s file1 file_link1
ln -s file2 file_link2
ln -s file3 file_link3
ln -s file4 file_link4

Run rmlint
It identifies correctly the two duplicate files

# Duplicate(s):
    ls '<..>/test_softlink_4_2/file1'
    rm '<..>/test_softlink_4_2/file2'
    ls '<..>/test_softlink_4_2/file3'
    rm '<..>/test_softlink_4_2/file4'

Run ./rmlint.sh -d

[  0%] Keeping:  <..>/test_softlink_4_2/file1
[ 25%] Deleting: <..>/test_softlink_4_2/file2
[ 50%] Keeping:  <..>/test_softlink_4_2/file3
[ 75%] Deleting: <..>/test_softlink_4_2/file4
[100%] Done!

Final result

-rw-rw-r-- 1 ray ray    5 mrt 11 15:17 file1
-rw-rw-r-- 1 ray ray   13 mrt 11 15:17 file3
lrwxrwxrwx 1 ray ray    5 mrt 11 15:17 file_link1 -> file1
lrwxrwxrwx 1 ray ray    5 mrt 11 15:17 file_link2 -> file2 ==> dead link
lrwxrwxrwx 1 ray ray    5 mrt 11 15:17 file_link3 -> file3
lrwxrwxrwx 1 ray ray    5 mrt 11 15:17 file_link4 -> file4 ==> dead link

I would expect the soft links of the deleted files to be removed as well.

[fermino]

Would a subsequent pass detect the broken links?

[RayOei]

It does.. but that was a bit the base of my question. Should you expect to run it again to do some additional cleaning or would it be more logical that the first clean also cleans the soft links? I couldn't find conclusive information on how this is supposed to be treated. It is not a big issue but I can image that some users after running the generated script assume things have been cleared.

[vassilit]

It does.. but that was a bit the base of my question. Should you expect to run it again to do some additional cleaning or would it be more logical that the first clean also cleans the soft links?

That's an interesting point, but unrelated to this bug that concerns replays exclusively.

We could do two passes and check for dangling links but I'm not sure if that would be a bug fix or a feature.

[RayOei]

I'm not sure if that would be a bug fix or a feature

Me neither :grin
It looks like it is long time behavior... so..
But personally I would (like to) see that as an improvement, either way. I will create an issue with the information above.