wolkykim/qdecoder

CRLF and FastCGI compatibility

Closed this issue · 9 comments

nyov commented

(cont. from #2)
regarding a3277e9,
the CRLF fix works, I wasn't sure if it's really necessary after every header, but it seems to be the right thing to do, as the RFC defines it.

regarding 58e6191,
fwrite() doesn't fix the download so far, but your use of fdopen looks close to my own try, so I'll try to make it work and let you know.

nyov commented

Found a fix to the problem.
First I found that fileno(stdin), fileno(stdout), fileno(stderr) all return -1 in the FCGI wrapped environment.
The result is that fdopen(fileno(stdout)) here always fails. Writing to the filepointers actually works, being wrapped as FCGI_std*.

So the quick fix that works, is this:

diff --git a/src/internal.c b/src/internal.c
index 772c924..b58a34d 100644
--- a/src/internal.c
+++ b/src/internal.c
@@ -282,11 +282,6 @@ off_t _q_filesize(const char *filepath)
 off_t _q_iosend(int outfd, int infd, off_t nbytes) {
     if (nbytes == 0) return 0;

-#ifdef ENABLE_FASTCGI
-    FILE* outfp = fdopen(outfd, "w");
-    if (outfp == NULL) return -1;
-#endif
-
     unsigned char buf[QIOSEND_CHUNK_SIZE];
     off_t total = 0; // total size sent
     while (total < nbytes) {
@@ -301,7 +296,7 @@ off_t _q_iosend(int outfd, int infd, off_t nbytes) {

         // write
 #ifdef ENABLE_FASTCGI
-        size_t wsize = fwrite(buf, rsize, 1, outfp);
+        size_t wsize = fwrite(buf, 1, rsize, stdout);
 #else
         ssize_t wsize = write(outfd, buf, rsize);
 #endif

Not so nice, but maybe it makes sense to rewrite this function for passing file pointers?
Since read/write are POSIX, might even make it more portable :D

maybe we can change _q_iosend to take FILE* argument.

I made this patch. What do you think? And could you test if this works?
1aab627

I gave you the full permission for you to be able to push your change directly to the repo.
When the change is small and if you feel pretty comfortable with your change, you could go ahead push it directly. Also it might be a good idea keep doing pull request just for a while just to get to know the style each other.

Also please update README.md file in the package to include your name under the contributor section(located at the very end)

nyov commented

the patched _q_iosend in 1aab627 works great.

I don't need the kudos, but I see you already added me. I'll just fix the typo there :)

nyov commented

I see you really gave me that push access, I'm honored. Of course I had to test it, because I never did that before ;)
But I don't really need that level of access, I won't push to your master branch without a pull request again, because then it can be discussed and possibly updated, and everyone is on the same page before a merge.

And I wanted to get the FastCGI code working, but I don't know if I can keep working on the project, since I have no use-case right now, sorry to say.
Besides, my C skills are pretty rusty :)

Hi,

Please feel free about it. We all make contributions whenever you want to.
No pressure there at all. That's it. :)
Would you like to include your name rather than nyoy on the readme file?

On Tue, May 20, 2014 at 2:31 AM, nyov notifications@github.com wrote:

I see you really gave me that push access, I'm honored. Of course I had to
test it, because I never did that before ;)
But I don't really need that level of access, I won't push to your master
branch without a pull request again, because then it can be discussed and
possibly updated, and everyone is on the same page before a merge.

And I wanted to get the FastCGI code working, but I don't know if I can
keep working on the project, since I have no use-case right now, sorry to
say.
Besides, my C skills are pretty rusty :)

Reply to this email directly or view it on GitHubhttps://github.com//issues/4#issuecomment-43604558
.

Great to hear it works. I'm closing this issue then.