ERL-327 On zero-size files attempt to read until EOF #1524
Conversation
f24a563
to
e83c11a
Compare
| datap = d->c.read_file.binp->orig_bytes + actual_size; | ||
|
|
||
| /* Read until we hit EOF (read less than requested) */ | ||
| read_result = read((int) d->fd, datap, FILE_SEGMENT_READ); |
There was a problem hiding this comment.
You should use the efile_read(errInfo, flags, fd, buf, count, pBytesRead) wrapper instead of plain read(fd, p, size) since the former for example handles both retry on EINTR as well as uses the right call on Windows.
There was a problem hiding this comment.
Done. Will check it and submit soon.
| /* [ERL-327] Some special files like /proc/... have reported size 0 */ | ||
| void read_file_zero_size(struct t_data* d) { | ||
| size_t actual_size = 0; | ||
| while (1) { |
There was a problem hiding this comment.
I personally prefer the for (;;) construct instead of while (1) since a compiler once warned me that 1 was a constant expression.
| static void invoke_read_file(void *data) | ||
| { | ||
| struct t_data *d = (struct t_data *) data; | ||
| size_t read_size; | ||
| int chop; | ||
| DTRACE_INVOKE_SETUP(FILE_READ_FILE); | ||
|
|
There was a problem hiding this comment.
Whitespace changes makes the diff harder to read
There was a problem hiding this comment.
I will fix this in a separate commit. Then squash before merge.
| EFILE_MODE_READ, &fd, &size))) { | ||
| goto done; | ||
| } | ||
| d->fd = fd; | ||
| d->c.read_file.size = (int) size; | ||
| if (size < 0 || size != d->c.read_file.size || | ||
| ! (d->c.read_file.binp = | ||
| ! (d->c.read_file.binp = | ||
| driver_alloc_binary(d->c.read_file.size))) { |
There was a problem hiding this comment.
It should be possible that for the special case size == 0 pre-allocate one chunk and then in read_file_zero_size(d) not have to start with reallocating it from size 0 to one chunk
There was a problem hiding this comment.
Done. Will check it and submit soon.
| size_t actual_size = 0; | ||
| while (1) { | ||
| ssize_t read_result; | ||
| char* datap = 0; |
There was a problem hiding this comment.
It should be possible to use d->c.read_file.orig_bytes and d->c.read_file.offset instead of actual_size and datap to reduce the number of variables and to make the code more similar to the non-0-size code (less places where the type could get wrong)
|
Thank you for the updates! One more small nitpick that I am sorry I missed the first time: we like use tests a'la |
|
I see you also changed the chunk size to 64 kB. Great! We talked about 256 kB being on the fat side internally, but I did not remember having said that to you. We can talk more about the final chunk size later... This kind of change has been up for discussion many times before and always been rejected. I do not remember the exact reasons, nor the exact proposals, and can not find any meeting protocols about it. So the final decision will have to wait until everybody with an opinion gets back from their vacations. |
|
The chunk size only affects zero-file algorithm as my common sense told me 256kb is too much for possibly a ~3kb read. I did not modify the original |
| @@ -1290,6 +1340,11 @@ static void invoke_read_file(void *data) | |||
| } | |||
| /* Invariant: d->c.read_file.size >= d->c.read_file.offset */ | |||
|
|
|||
| if (d->c.read_file.size != 0) { | |||
There was a problem hiding this comment.
97e33f9 accidentally inverted this condition, it should be d->c.read_file.size == 0
read_file_zero_size sets d->again to 0 and doesn't close the file on its own, so jumping to chop_done will leak. goto close; instead?
There was a problem hiding this comment.
Indeed! I am committing a fix now (it all will be squashed anyway, so doesn't matter in the end)
C really needs some RAII and resource lifetimes 😄 (which it has in form of __attribute__((cleanup(my_function_name))) but that is Clang/GCC specific and isn't a generic C89)
|
Now we are just missing some test cases. It would be simple to test if there is a |
|
|
||
| %% Fail if none of mentioned files exist in /proc, did we just get | ||
| %% a normal /proc directory without any special files? | ||
| ?assertNotEqual([], Inputs1), |
There was a problem hiding this comment.
None of the /proc/ files in "Inputs" are available on freebsd or solaris.
On freebsd I suggest you use /proc/curproc/cmdline.
On solaris all of the files in /proc/ seem to have the correct filesize, so not sure what we can do there. I suggest skipping them.
|
I squashed this and merged it to master. Thanks! |
When read_file is invoked on a file with zero size, it will attempt to read
256kb64kb blocks until EOF then stop and return the result. Assuming most of files in/proc/are few kb in length, this typically should result in 1 allocation of256kb64kb and 1 following reallocation down to the real size.