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

calculate file and directory size #197

Merged
merged 4 commits into from
Jan 10, 2020
Merged

calculate file and directory size #197

merged 4 commits into from
Jan 10, 2020

Conversation

Karsten1987
Copy link
Contributor

connects to ros2/rosbag2#248
Signed-off-by: Karsten Knese karsten@openrobotics.org

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

There's a compilation issue in here on Windows, but that is minor to fix.

What are we trying to accomplish with this? I ask because I'm kind of concerned about this piece of functionality. There are a lot of corner cases here: what about symlinks? How about /dev style files? What about the two different kinds of symlinks on Windows? What happens if one file in the directory is very large, but has its permissions set so the current user can't read it? Also, it seems pretty easy to Denial Of Service the application by creating a directory with many little files. Maybe we can figure out a different way to do this.

src/filesystem.c Outdated Show resolved Hide resolved
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
Copy link
Contributor Author

Karsten1987 commented Jan 8, 2020

This code is currently already being used within rosbag2. As per top-level issue description, there are currently three places within ROS2 (rosbag2, rcpputils,rcutils) where filesystem helper functions are implemented.
rosbag2 can solely rely on the other two packages except that the two calculate functions are currently unavailable in rcutils.

What about the two different kinds of symlinks on Windows? What happens if one file in the directory is very large, but has its permissions set so the current user can't read it? Also, it seems pretty easy to Denial Of Service the application by creating a directory with many little files. Maybe we can figure out a different way to do this.

Is there a safe & cross-platform way of getting the file size as well as directory size? I am currently not aware of any other.

@piraka9011
Copy link
Contributor

Some of this was proposed in rcpputils as well here.

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
src/filesystem.c Outdated
size_t dir_size = 0;

if (!rcutils_is_directory(directory_path)) {
fprintf(stderr, "Path is no directory: %s\n", directory_path);
Copy link
Contributor

@piraka9011 piraka9011 Jan 9, 2020

Choose a reason for hiding this comment

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

Typo: not a

src/filesystem.c Outdated
rcutils_get_file_size(const char * file_path)
{
if (!rcutils_is_file(file_path)) {
fprintf(stderr, "Path is no file: %s\n", file_path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: not a

Copy link
Contributor

@piraka9011 piraka9011 left a comment

Choose a reason for hiding this comment

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

Some nits but this LGTM 👍

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
Copy link
Contributor Author

@clalancette please let me know if you have any further comments on this. CI turns out to be green and both PRs are approved. I just wanted to make sure we can easily address your concerns here in this PR. If it turns out to be a more complex change, I'd propose we open a follow up ticket/PR to this.

@Karsten1987
Copy link
Contributor Author

@clalancette I am going ahead and merge. Please feel free to come up with a follow up issue for addressing your concerns.

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.

3 participants