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

Support reading "special files" that lie about their size #1064

Merged
merged 3 commits into from
Oct 12, 2023

Conversation

graebm
Copy link
Contributor

@graebm graebm commented Oct 12, 2023

Issue:
aws_byte_buf_init_from_file() didn't work with "special files", which don't accurately report their size. For example, on my Amazon Linux 2:

  • /proc/cpuinfo: size is 0, but contents are several KB of data.
  • /sys/devices/virtual/dmi/id/product_name: size is 4096, but contents are "c5.2xlarge"

Description of changes:

  • aws_byte_buf_init_from_file() no longer 100% trusts the reported size. Size is used as a hint, but it always reads now until EOF, growing the buffer if necessary.
  • New function aws_byte_buf_init_from_file_with_size_hint() lets users pass a hint, rather than querying the OS for it.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@bretambrose bretambrose left a comment

Choose a reason for hiding this comment

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

maybe with_size_hint() rather than "special" which isn't very meaningful

@graebm
Copy link
Contributor Author

graebm commented Oct 12, 2023

maybe with_size_hint() rather than "special" which isn't very meaningful

I also just encountered this term yesterday for the first time, but it came up in 2 different contexts! Apparently it's the official term: https://www.google.com/search?q=special+files

So I assumed it was something everyone but me already knew. But if you and me were both thrown by the term, then probably not.

So yeah, I'll change it

including two-days-ago me
@graebm graebm changed the title aws_byte_buf_init_from_special_file() Support reading "special files" that lie about their size Oct 12, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2023

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (d09b75e) 82.19% compared to head (7f8efed) 82.73%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1064      +/-   ##
==========================================
+ Coverage   82.19%   82.73%   +0.53%     
==========================================
  Files          52       52              
  Lines        5646     5664      +18     
==========================================
+ Hits         4641     4686      +45     
+ Misses       1005      978      -27     
Files Coverage Δ
source/file.c 77.11% <63.15%> (+14.11%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +51 to +52
bool use_file_size_as_hint,
size_t size_hint) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Debatable / Code Clarity: There are two different ways to use this function, which is a bit confusing. I suggest we change this function to

Suggested change
bool use_file_size_as_hint,
size_t size_hint) {
s_byte_buf_init_from_file(
struct aws_byte_buf *out_buf,
struct aws_allocator *alloc,
const char *filename,
size_t size_hint)

and let aws_byte_buf_init_from_file calculate size_hint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried writing it that way first, but it was more confusing IMHO.

You ended up with 3 functions that needed to deal with error-handling instead of just 1, and scrambled logic around instead of putting it all in 1 place.

@graebm graebm enabled auto-merge (squash) October 12, 2023 22:28
@graebm graebm merged commit 0baed28 into main Oct 12, 2023
51 checks passed
@graebm graebm deleted the read-weird-files branch October 12, 2023 22: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.

4 participants