kazu-yamamoto / simple-sendfile

Cross platform library for the sendfile system call
BSD 3-Clause "New" or "Revised" License
15 stars 11 forks source link

use autoconf for correct bit-ness of off_t/sendfile() #18

Closed crosser closed 7 months ago

crosser commented 9 years ago

I have no 32bit systems to test it, so it's only checked on 64bit Linux (where none of the LARGEFILE-magic is necessary anyway). So please test before merging into mainline.

crosser commented 9 years ago

Note that it's forked at 7f64dc2, before the "use safe size according to CSize" commit!

crosser commented 9 years ago
  1. I was wrong that FFI will automatically expand the sendfile() libc macro and call the right function: sendfile() or sendfile64. This manual disproves my theory; I followed the recommendation and made a C file with trivial wrapper function.
  2. safeSize handling was wrong. sendfileloop's argument was of type CSize, and the maximum value was limited to roughly maximum CSize, which does not make sense. I changed the size argument of sendfileloop to Integer, and the logic to send in chunks no bigger than can fit in CSize type.
  3. I added tests with a file bigger than 4 Gb. The code is a bit dirty, sorry. But it works. The tests succeed on Linux32 with largefile support and on Linux64, they will fail on Linux32 without largefile support. I would not be able to check, so I did not even try. I let go hspec autodetection, it's not worth the effort when there is only a single spec file.

Anyway it works for me (tm) in the current form. Please see if you want to merge all or parts.

kazu-yamamoto commented 9 years ago

I was wrong that FFI will automatically expand the sendfile() libc macro and call the right function: sendfile() or sendfile64. This manual disproves my theory; I followed the recommendation and made a C file with trivial wrapper function.

Sorry, but it seems to me that this is over engineering. CSize and COff is set by autoconf in the base library. I don't know why simple-sendfile use autoconf again.

safeSize handling was wrong. sendfileloop's argument was of type CSize, and the maximum value was limited to roughly maximum CSize, which does not make sense. I changed the size argument of sendfileloop to Integer, and the logic to send in chunks no bigger than can fit in CSize type.

I don't understand why the current sefaSize handling is wrong. Moreover, the current code has less type conversion. So, I like the current code better.

I added tests with a file bigger than 4 Gb. The code is a bit dirty, sorry. But it works. The tests succeed on Linux32 with largefile support and on Linux64, they will fail on Linux32 without largefile support. I would not be able to check, so I did not even try. I let go hspec autodetection, it's not worth the effort when there is only a single spec file.

Let's add this tests. I guess that the current code passes the tests even on Linux32 without largefile suppot.

crosser commented 9 years ago

My approach has more parts; the upside is that the code is completely agnostic to the presence of different interface functions. I agree that the binding should not need to run autoconf, as I argued in the maillist thread that I cited in the wai ticket. I would argue that the FFI could and should be designed to automatically generate wrappers when necessary. As it stands now, you need a wrapper to let existing libc CPP magic to do the compile-time dispatch, and you need autoconf to provide the libc header with the environment to make this dispatch decision.

My argument is that making dispatch decision in the Haskell code is hackery (in a bad sense), it is trying to deal with the guts of the libc interface that libc has intentionally hidden away.

But it's your project, your decision.

I did not see your latest code; the variant that I criticized had the function argument of the type CSize, and it was checking if the value of the argument is small enough to fit in CSize, and if it was larger, run in multiple chunks. But after you have a value of the type CSize, it's too late to check if it fits in CSize. It was already silently truncated before. That's why the user had those failuers, when only part of the file was sent, but there was no error indication.

Pre-existing tests passed on Linux32 without largefile support. They also passed on Linux32 with largefile support, just not triggering the errors that where present in the implementation. For proper test, you need to use a file with size that does not fit in 32bit variable, and you need to send a chunk of it of the size bigger than fits in 32bit varuable. That is what my test is about.