[PATCH] imap_savebody()

php.internals

Michael Wallner

20 years ago
Hi, Currently there is no way to fetch a big message/attachment through ext/imap if memory_limit is enabled. I'd like to kindly ask for consideration of the following patch: http://dev.iworks.at/PATCHES/imap_savebody.patch.txt The patch introduces a new function imap_savebody(rsrc imap, string file, int msgno, string sect[, int flags]) which will save the specified body section to a file. Thanks,
-- Michael - < mike(@)php.net >

Sara Golemon

20 years ago
> #define GETS_FETCH_SIZE 2048000 >
That size doesn't seem a bit excessive to you? I thought the whole point of dumping it to a file was to avoid huge memory chunks. Not to mention the fact that reserving 2MB of stack space makes me a little twitchy...
> /* {{{ proto bool imap_savebody(resource stream_id, > string file, int msg_no, string section[, int options]) >
I'd make section option with the default being whatever it is that just gives you the whole body. Come to that it probably makes sense to be able to save the entire message (including headers). I'd also suggest either a second function, or an overloaded second parameter so that an existing stream could be used. This way a scripter could shuttle multiple messages off to a spool file, or open one for appending and add a message, or dump it to network socket (maybe they're implementing their own POP3=>IMAP gateway (god only knows why). -Sara

Ilia A.

20 years ago
Sara Golemon wrote:
>>#define GETS_FETCH_SIZE 2048000 >> > > That size doesn't seem a bit excessive to you? I thought the whole point of > dumping it to a file was to avoid huge memory chunks. Not to mention the > fact that reserving 2MB of stack space makes me a little twitchy...
Indeed, 2 megs of stack space seems a bit much, not to mention completely bypassed PHP's memory limit. I'd recommend allocating 1 megabyte buffer via emalloc(), a bit slower, but much "nicer" IMHO. Ilia

Wez Furlong

20 years ago
Use 8192 chunk size; anything bigger than that is a complete waste of memory. --Wez. On 9/22/05, Ilia Alshanetsky <ilia@prohost.org> wrote:

Andi Gutmans

20 years ago
Yep, I agree with that, and some past tests of mine showed that depending on the system, anything bigger than about 8-64K, will give marginal performance increase (and possibly decrease depending on memory allocation); so it's really not worth the hassle. It's also a size which we can allocate on the stack. Andi At 09:52 AM 9/22/2005, Wez Furlong wrote:

Michael Wallner

20 years ago
I wrote:
> Currently there is no way to fetch a big message/attachment through > ext/imap if memory_limit is enabled. I'd like to kindly ask for > consideration of the following patch: > > http://dev.iworks.at/PATCHES/imap_savebody.patch.txt
Thanks a lot for review; the patch has been updated with your suggestions. Additionally the whole thing is ifdef'd out in ZTS mode as the c-client gets function is a global variable. I tested the function with a 17M message saving to a file as well to an already opened stream. Thanks,
-- Michael - < mike(@)php.net >

Sara Golemon

20 years ago
> > http://dev.iworks.at/PATCHES/imap_savebody.patch.txt > > Thanks a lot for review; the patch has been updated with your suggestions. > Additionally the whole thing is ifdef'd out in ZTS mode as the c-client > gets function is a global variable. I tested the function with a 17M > message saving to a file as well to an already opened stream. >
Getting there, but you can make it work for ZTS mode. Just create a proxy for the gets function which is always set, then have the proxy do a TSRMLS_FETCH() and examine an extension global (e.g. IMAPG(gets_mode) or whatever) to determine whether it's in savebody() and should shuttle to your new gets function, or if it's not in savebody() and should dispatch to the "original" method. Come to that, might as well just make IMAPG(real_gets) as a dynamic function.... Not that I've looked at ext/imap enough to know what the role of gets is precisely. -Sara

Michael Wallner

20 years ago
Hi Sara Golemon, you wrote:
> Getting there, but you can make it work for ZTS mode. Just create a proxy > for the gets function which is always set, then have the proxy do a > TSRMLS_FETCH() and examine an extension global (e.g. IMAPG(gets_mode) or > whatever) to determine whether it's in savebody() and should shuttle to your > new gets function, or if it's not in savebody() and should dispatch to the > "original" method.
Thanks for your input, Sara. The new patch can be found at: http://dev.iworks.at/PATCHES/imap_savebody_TS.patch.txt Regards,
-- Michael - < mike(@)php.net >

Sara Golemon

20 years ago
Looks basicly alright (with a slight disagreement on how the read loop is implemented (below). In general you've got my vote (for what it's worth given my lack of experience with libc-client. unsigned long i, rest = size % GETS_FETCH_SIZE; char *buf = emalloc(GETS_FETCH_SIZE+1); for (i = GETS_FETCH_SIZE; i < size; i += GETS_FETCH_SIZE) { memset(buf, 0, GETS_FETCH_SIZE+1); f(stream, GETS_FETCH_SIZE, buf); php_stream_write_string(IMAPG(gets_stream), buf); } if (rest) { memset(buf, 0, GETS_FETCH_SIZE+1); f(stream, rest, buf); php_stream_write_string(IMAPG(gets_stream), buf); } efree(buf); return NULL; Here's how I'd write this block, it assumed that f returns the number of bytes actually read, but according to the proto for readfn_t it *should* do that. Feel free to ignore: char buf[GETS_FETCH_SIZE]; unsigned long ret = 1; while (ret > 0) { ret = f(stream, GETS_FETCH_SIZE, buf); php_stream_write_stringl(IMAPG(gets_stream), buf, ret); } return NULL;

Michael Wallner

20 years ago
Hi Sara Golemon, you wrote:
> Here's how I'd write this block, it assumed that f returns the number of > bytes actually read, but according to the proto for readfn_t it *should* > do that.
Well, unfortunately not. It seems that net_get_buffer() in mail.c is used as standard readfn_t which only returns NIL/T i.e. 0/1. But the patch's implementation is by no means mandatory, it's just about getting the data out with enabled memory_limit, so I actually wouldn't mind if it would get implemented in a totally different way ;) Regards,
-- Michael - < mike(@)php.net >

Wez Furlong

20 years ago
I'd recommend adopting Sara's suggestion; those memset()s you have there shouldn't be needed: char buf[GETS_FETCH_SIZE]; while (ret > 0) { if (!f(stream, sizeof(buf), buf)) break; php_stream_write_string(IMAPG(gets_stream), buf); } return NULL; --Wez. On 9/26/05, Michael Wallner <mike@php.net> wrote:

Sara Golemon

20 years ago
> Well, unfortunately not. It seems that net_get_buffer() in mail.c is > used as standard readfn_t which only returns NIL/T i.e. 0/1. >
Okay, take two: char buf[GETS_FETCH_SIZE]; while (size > 0) { in toread = (size > GETS_FETCH_SIZE) ? GETS_FETCH_SIZE : size); if (!f(stream, toread, buf)) break; php_stream_write_stringl(IMAPG(gets_stream), buf, toread); size -= toread; } return NULL;

Michael Wallner

20 years ago
Ok, here we go: http://dev.iworks.at/PATCHES/imap_savebody_TS.patch.txt Regards,
-- Michael - < mike(@)php.net >