Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lkl tools: cpfromfs: fix root directory copy #300

Merged
merged 1 commit into from
Jan 19, 2017
Merged

lkl tools: cpfromfs: fix root directory copy #300

merged 1 commit into from
Jan 19, 2017

Conversation

tavip
Copy link
Member

@tavip tavip commented Jan 18, 2017

This fixes #295


This change is Reviewable

@@ -493,6 +493,9 @@ int copy_one(const char *src, const char *mpoint, const char *dst)
snprintf(dst_path, sizeof(dst_path), "%s", dst);
}

if (strcmp(src, "/") == 0)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth handling "//" and other logically equivalent paths here? I don't expect to use them directly, but they can occasionally arise when someone uses programmatically generated paths.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copumpkin Good point. I think the equivalent regular expression [./]* should cover all cases. .. already works fine so we don't need consider it. Anything else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copumpkin done

const char *c = src;

while (*c) {
if (*c != '.' && *c != '/')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI, this will match "../" and "/../../../....." and other nonsense like that too. That's why I phrased my original comment as a question, FWIW: not sure it's actually worth doing this accurately 😄

@copumpkin
Copy link

Sorry, haven't had a chance to test this yet, but the approach looks good code-wise!

Currently copying the disk image root is broken:

cptofs -P 1 -t ext4 -i test.img ~/linux /
cpfromfs -P 1 -t ext4 -i test.img / ~/linuxcopy

leads to the following in ~/linuxcopy:

/home/ec2-user/linuxcopy
└── 0000fe01
    ├── linux
        │   ├── arch

The problem is that basename("/mnt/0000fe01/") returns 0000fe01 where
we would expect /.

Fix that by making the root folder copy case special.

Reported-by: Dan Peebles <pumpkin@me.com>
Signed-off-by: Octavian Purdila <tavi@cs.pub.ro>
@tavip
Copy link
Member Author

tavip commented Jan 18, 2017

@copumpkin Pushed a new version, probably still has multiple holes in it :) but unless there is something really wrong with it, I think it is good enough.

@tavip tavip merged commit c874d36 into master Jan 19, 2017
@tavip tavip deleted the issue-294 branch January 19, 2017 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cpfromfs seems to inject unpredictable prefix into filesystem
2 participants