You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@perl.apache.org by Geoffrey Young <ge...@modperlcookbook.org> on 2004/01/23 20:41:10 UTC
apr_brigade_pflatten
here's APR::Brigade::pflatten(), with tests
--Geoff
Re: apr_brigade_pflatten
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
>> the problem with all of these is that it's breaking away from the 1:1
>> mapping of the original apr_brigade_(p)flatten APIs. if we're going
>> to do
>> that to make them more perlish, I'd much rather see
>>
>> my $data = $bb->flatten(30); # calls apr_brigade_flatten
>> my $data = $bb->flatten($p); # calls apr_brigade_pflatten
>>
>> and throw away the length component, since it's very rarely important
>> anyway, has the possibility of being wrong (at least that's my reading of
>> the API), and can be computed (more accurately) if required.
>
>
> Sounds good to me. It makes the API so much simpler to use.
>
> I just thought that since it's similar to read() it could behave like one.
>
> I find it strange that you need the pool to allocate the unknown len,
> but you don't need one to allocate a certain amount of bytes. To make
> the API consistent I'd suggest it to be:
>
> my $data = $bb->flatten($p, [$len]);
>
> i.e. always pass $p, and use apr_pcalloc to allocate $len bytes if that
> optional argument is passed.
done. take a look when you get the chance just to make sure I did the
allocation properly.
--Geoff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: apr_brigade_pflatten
Posted by Stas Bekman <st...@stason.org>.
Geoffrey Young wrote:
>>I'd rather see:
>>
>>my ($data, $len) = foo(...);
>>
>>so much more perlish. but I guess it's too late in the game.
>>
>>The only inconvenience is that you can't use that in the while loop:
>>
>> while (defined my $len = foo($data, ...) {}
>
>
>>there is no problem here. flatten should accept how much data to read as
>>an argument and return how much it actually read as a return value.
>>That's exactly how perl's read() works. and that value is optional.
>
>
> the problem with all of these is that it's breaking away from the 1:1
> mapping of the original apr_brigade_(p)flatten APIs. if we're going to do
> that to make them more perlish, I'd much rather see
>
> my $data = $bb->flatten(30); # calls apr_brigade_flatten
> my $data = $bb->flatten($p); # calls apr_brigade_pflatten
>
> and throw away the length component, since it's very rarely important
> anyway, has the possibility of being wrong (at least that's my reading of
> the API), and can be computed (more accurately) if required.
Sounds good to me. It makes the API so much simpler to use.
I just thought that since it's similar to read() it could behave like one.
I find it strange that you need the pool to allocate the unknown len, but you
don't need one to allocate a certain amount of bytes. To make the API
consistent I'd suggest it to be:
my $data = $bb->flatten($p, [$len]);
i.e. always pass $p, and use apr_pcalloc to allocate $len bytes if that
optional argument is passed.
>>Now that you mention that, we could really have one interface instead of
>>2. If you pass the optional read length value it'll use flatten if you
>>don't it'll use pflatten. make it use the pool object in both but ignore
>>in flatten. So you get a 1:1 mapping to perl's read (we may even support
>>the offset argument later on) ;)
>>
>>Hell, why not write PerlIO::BB ;)
>
>
> I'm already working on APR::TieBrigade, which lets you manipulate a brigade
> like a perl $fh :)
Cool. Though this is probably something that is not quite needed at the moment.
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: apr_brigade_pflatten
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
> I'd rather see:
>
> my ($data, $len) = foo(...);
>
> so much more perlish. but I guess it's too late in the game.
>
> The only inconvenience is that you can't use that in the while loop:
>
> while (defined my $len = foo($data, ...) {}
> there is no problem here. flatten should accept how much data to read as
> an argument and return how much it actually read as a return value.
> That's exactly how perl's read() works. and that value is optional.
the problem with all of these is that it's breaking away from the 1:1
mapping of the original apr_brigade_(p)flatten APIs. if we're going to do
that to make them more perlish, I'd much rather see
my $data = $bb->flatten(30); # calls apr_brigade_flatten
my $data = $bb->flatten($p); # calls apr_brigade_pflatten
and throw away the length component, since it's very rarely important
anyway, has the possibility of being wrong (at least that's my reading of
the API), and can be computed (more accurately) if required.
>
> Now that you mention that, we could really have one interface instead of
> 2. If you pass the optional read length value it'll use flatten if you
> don't it'll use pflatten. make it use the pool object in both but ignore
> in flatten. So you get a 1:1 mapping to perl's read (we may even support
> the offset argument later on) ;)
>
> Hell, why not write PerlIO::BB ;)
I'm already working on APR::TieBrigade, which lets you manipulate a brigade
like a perl $fh :)
--Geoff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: apr_brigade_pflatten
Posted by Stas Bekman <st...@stason.org>.
Geoffrey Young wrote:
>>I don't like this populate-the-argument C interface.
>
>
> me neither.
I'd rather see:
my ($data, $len) = foo(...);
so much more perlish. but I guess it's too late in the game.
The only inconvenience is that you can't use that in the while loop:
while (defined my $len = foo($data, ...) {}
>>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.
>
>
> that probably makes more sense. but it makes it dissimilar from flatten()
> which returns both the data and length as well.
there is no problem here. flatten should accept how much data to read as an
argument and return how much it actually read as a return value. That's
exactly how perl's read() works. and that value is optional.
Now that you mention that, we could really have one interface instead of 2. If
you pass the optional read length value it'll use flatten if you don't it'll
use pflatten. make it use the pool object in both but ignore in flatten. So
you get a 1:1 mapping to perl's read (we may even support the offset argument
later on) ;)
Hell, why not write PerlIO::BB ;)
>>with your implementation $rc is useless
>>anyway.
>
>
> well, not useless - if it's not APR::SUCCESS then you know not to use the
> returned information
Yes, yes, but you convey that exact information in returning undef vs. defined
length.
>>It should return -1 on error and 0 or more as a number of bytes
>>populated in $data.
>
>
> I prefer undef to -1 in a perl interface.
Oops, you are right, then we need to fix the read as well. For some reason I
thought perl's read() was returning -1.
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: apr_brigade_pflatten
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
> I don't like this populate-the-argument C interface.
me neither.
> 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.
that probably makes more sense. but it makes it dissimilar from flatten()
which returns both the data and length as well.
> with your implementation $rc is useless
> anyway.
well, not useless - if it's not APR::SUCCESS then you know not to use the
returned information
> It should return -1 on error and 0 or more as a number of bytes
> populated in $data.
I prefer undef to -1 in a perl interface.
> returning the length as I've suggested will remove the need for this
> questionable snippet. I'd croak otherwise and not silently continue.
as we've already discussed, I may have a problem with croak. but I'm
thinking on it.
>
> 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.
yup.
--Geoff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: apr_brigade_pflatten
Posted by Stas Bekman <st...@stason.org>.
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:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org