Geoffrey Young wrote:
here's APR::Brigade::pflatten(), with tests

nice! a few comments below.


------------------------------------------------------------------------

Index: t/response/TestAPR/flatten.pm
[...]
+    # pflatten() populates $length.  make sure that if users
+    # get flatten() (which requires a length) and pflatten()
+    # confused that we don't segfault
+    {
+        my $rc = $bb->pflatten(my $data, 30, $r->pool);

I don't like this populate-the-argument C interface. And we already do that in several places. But for 2 arguments? That's too much and inconsistent with other similar interfaces. I think pflatten should be similar to $r->read(), i.e. populate the buffer $data, but return the number of bytes it has read. with your implementation $rc is useless anyway. It should return -1 on error and 0 or more as a number of bytes populated in $data.


Index: xs/APR/Brigade/APR__Brigade.h
===================================================================
RCS file: /home/cvspublic/modperl-2.0/xs/APR/Brigade/APR__Brigade.h,v
retrieving revision 1.5
diff -u -r1.5 APR__Brigade.h
--- xs/APR/Brigade/APR__Brigade.h 23 Jan 2004 15:26:55 -0000 1.5
+++ xs/APR/Brigade/APR__Brigade.h 23 Jan 2004 19:40:18 -0000
@@ -103,3 +103,28 @@
return status;
}
+
+static MP_INLINE
+apr_status_t mpxs_apr_brigade_pflatten(pTHX_ apr_bucket_brigade *bb,
+ SV *sv_buf, SV *sv_len,
+ apr_pool_t *pool)
+{
+ apr_status_t status;
+ apr_size_t length;
+ char *buffer;
+
+ status = apr_brigade_pflatten(bb, &buffer, &length, pool);
+
+ /* XXX what if the pool goes out of scope? */
+ sv_setpvn(sv_buf, buffer, length);
+ + /* we shouldn't need this check per the API, but just in
+ * case people get the flatten() and pflatten() arguments
+ * confused...
+ */
+ if (!SvREADONLY(sv_len)) {
+ sv_setiv(sv_len, length);
+ }

returning the length as I've suggested will remove the need for this questionable snippet. I'd croak otherwise and not silently continue.


There is one more thing to handle: the case when the brigade is empty. When this happens we need to put undef in the perl buffer. See the RequestIO read implementation. This is needed in both flatten functions.

__________________________________________________________________
Stas Bekman            JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/     mod_perl Guide ---> http://perl.apache.org
mailto:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org   http://ticketmaster.com


--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]



Reply via email to