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.

Sign up for GitHub

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

Jump to bottom

copy: avoid useless comparison for non-directory entries #6656

Closed
wants to merge 1 commit into from
Closed

copy: avoid useless comparison for non-directory entries #6656

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/basic/copy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,14 +386,13 @@ static int fd_copy_directory(
continue;
}

if (buf.st_dev != original_device)
continue;

if (S_ISREG(buf.st_mode))
q = fd_copy_regular(dirfd(d), de->d_name, &buf, fdt, de->d_name, override_uid, override_gid, copy_flags);
else if (S_ISDIR(buf.st_mode))
else if (S_ISDIR(buf.st_mode)) {
if (buf.st_dev != original_device)
continue;
Copy link
Member

@evverx evverx Aug 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you. I'm not sure where this if should be placed. fd_copy_directory stops copying as soon as it detects a filesystem boundary, while cp --one-file-system creates an empty directory before stopping copying.

Copy link
Author

Choose a reason for hiding this comment

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

Right. I made my change to be minimal while keeping the current behaviour. But when you come over such a mount point, it's true that the underlying filesystem has at least an empty directory for the mount to be possible. So arguably the behaviour of cp --one-file-system is more logical. Shall I update my patch to behave like cp ?

Copy link
Member

Choose a reason for hiding this comment

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

@rhertzog, I think that the patch fixes the bug and should be updated, but I'd rather wait for someone else to confirm that skipping device nodes and bind-mounted files is not intentional.

Copy link
Author

Choose a reason for hiding this comment

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

The stat call on device nodes does not report the device number of the device itself but of the filesystem hosting the device node, so this one is fine. You are right though that this changes the behaviour for bind-mounted files which would be copied with my patch while they are skipped currently. That said copy_tree is only used by systemd-tmpfiles to setup the temporary files and by machined to copy files between host and containers. In both cases, I don't believe that we have any specific requirement to ignore bind mounted files.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry. Indeed, I was wrong about device nodes. I should have used mknod to check the behaviour of stat.

I don't believe that we have any specific requirement to ignore bind mounted files.

Neither do I.

q = fd_copy_directory(dirfd(d), de->d_name, &buf, fdt, de->d_name, original_device, override_uid, override_gid, copy_flags);
else if (S_ISLNK(buf.st_mode))
} else if (S_ISLNK(buf.st_mode))
q = fd_copy_symlink(dirfd(d), de->d_name, &buf, fdt, de->d_name, override_uid, override_gid, copy_flags);
else if (S_ISFIFO(buf.st_mode))
q = fd_copy_fifo(dirfd(d), de->d_name, &buf, fdt, de->d_name, override_uid, override_gid, copy_flags);
Expand Down