You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@perl.apache.org by Stas Bekman <st...@stason.org> on 2004/01/27 23:57:53 UTC
Re: cvs commit: modperl-2.0/xs/tables/current/ModPerl FunctionTable.pm
geoff@apache.org wrote:
[...]
> - # test that other length variants - such as constants and
> - # subroutine returns - don't segfault
> + # syntax: flatten($p, 0) is equivalent to flatten($p)
> {
> - my $rc = $bb->flatten(my $data, 300000);
> + my $data = $bb->flatten($pool, 0);
> +
> + ok t_cmp(200000,
> + length($data),
> + 'APR::Brigade::flatten() returned all the data');
why? I don't find it intuitive at all. If you say 0 you should get an empty
string back (not undef and not the whole brigade).
Not only not intuitive, but wrong. Consider this case: some function returns
the wanted amount of chars to slurp, let's say it decides that it doesn't want
any more chars and sets $wanted to 0 and you passed it to
$bb->flatten($pool, $wanted); now instead of getting an empty string, you get
the whole brigade. You could check that $wanted is 0 and not call flatten, but
hey why making it more error-prone to users.
I think what you could use undef. These two:
my $data = $bb->flatten($pool, undef);
my $data = $bb->flatten($pool);
should probably be the same. But may be we don't need this extra complication
at all.
> Index: APR__Brigade.h
[...]
> +SV *mpxs_APR__Brigade_flatten(pTHX_ apr_bucket_brigade *bb,
> + apr_pool_t *pool, SV *sv_len)
> {
> +
> + /* XXX we're deviating from the API here to try and make
> + * the API more Perlish - nobody likes the idea of two
> + * "in/out" arguments. and we generally don't ever need
> + * the length anyway...
> + */
why the XXX comment? You aren't happy about it?
> apr_status_t status;
> - apr_size_t len = mp_xs_sv2_apr_size_t(sv_len);
> + char *buffer;
> + apr_size_t length;
> +
> + if (SvTRUE(sv_len)) {
I see, now I understand why you had this 0 case special. You need to
manipulate the perl stack here like other '...' functions do. In which case
the last argument is optional.
> + return newSVpvn(buffer, length);
this is a bad idea. You should install the buffer into the SV not copy it.
If in server_root_relative it was sort of ok, because usually the string is
short there, here you kill the performance and double the memory usage (for a
potentially huge string!). I guess to be consistent we need to drop the
server_root_relative copying as well and just hope that people will learn to
use the right pools if they are used in many APIs.
I'll write on the pool issue in a separate email.
__________________________________________________________________
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: cvs commit: modperl-2.0/xs/tables/current/ModPerl FunctionTable.pm
Posted by Stas Bekman <st...@stason.org>.
Geoffrey Young wrote:
>>You are correct. I didn't realize that pflatten was a cheat (plus scary
>>comment in apr_brigade_pflatten: 'XXX This is dangerous beyond
>>belief..'). I thought it didn't call apr_brigade_length, but did flatten
>>at one pass. I should have looked at the source. Sorry about that.
>
>
> np. I guess we both need to learn to shoot less from the hip... or lighten
> up a bit :)
Well, if you did look at apr_brigade_pflatten and you knew that it does a
double pass already, why didn't you say so in reponse to my "inefficiency"
comment? Remember, I wasn't working on this code and was just giving comments
to your patch.
>>>apr_brigade_pflatten should now get a different modifier than ~ in
>>>apr_functions.map.
>>
>>
>>why? it's implemented, no?
>
>
> kinda. I guess my thought is that if it has a ~ then it means that it's
> implemented proper - there's no pflatten() method anywhere now. but the
> maps are more of a guideline anyway for non-autogenerated stuff, so it's no
> biggie.
I can't see why do you suggest that apr_brigade_pflatten doesn't have a proper
implementation. $bb->flatten() is the proper implementation, no?
Dunno, may be we need a new marker that says that this function is implemented
but may be named differently?
> at any rate, at least we now have something that works.
;)
__________________________________________________________________
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: cvs commit: modperl-2.0/xs/tables/current/ModPerl FunctionTable.pm
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
> You are correct. I didn't realize that pflatten was a cheat (plus scary
> comment in apr_brigade_pflatten: 'XXX This is dangerous beyond
> belief..'). I thought it didn't call apr_brigade_length, but did flatten
> at one pass. I should have looked at the source. Sorry about that.
np. I guess we both need to learn to shoot less from the hip... or lighten
up a bit :)
> Yeah, I did do that initially, but thought that you won't like it, so
> I've kept it your way.
well, I didn't see a mpxs_usage_va variant that allowed for two object (bb
and pool) so I did them both manually. but for one I guess it doesn't matter.
>> apr_brigade_pflatten should now get a different modifier than ~ in
>> apr_functions.map.
>
>
> why? it's implemented, no?
kinda. I guess my thought is that if it has a ~ then it means that it's
implemented proper - there's no pflatten() method anywhere now. but the
maps are more of a guideline anyway for non-autogenerated stuff, so it's no
biggie.
at any rate, at least we now have something that works.
--Geoff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: cvs commit: modperl-2.0/xs/tables/current/ModPerl FunctionTable.pm
Posted by Stas Bekman <st...@stason.org>.
Geoffrey Young wrote:
>
> Stas Bekman wrote:
>
>>Doh! since my suggested change drops the use of apr_brigade_pflatten,
>>all this magic is no longer needed. just use the grow macro like all
>>other functions do and pass SvPVX to flatter. and whoah, no longer we
>>need the pool object! that's a goodness.
>
>
> which makes it look very much like my first implementation of
> apr_brigade_flatten only, which you objected to because it required looping
> through the brigade twice to get the entire brigade - once for
> apr_brigade_length, and once for apr_brigade_flatten.
You are correct. I didn't realize that pflatten was a cheat (plus scary
comment in apr_brigade_pflatten: 'XXX This is dangerous beyond belief..'). I
thought it didn't call apr_brigade_length, but did flatten at one pass. I
should have looked at the source. Sorry about that.
> but I guess it makes
> for a better API to give the entire brigade back if no length argument is
> given, and we did reorder the function a bit, so all is better in the end :)
;)
>>+ bb = mp_xs_sv2_APR__Brigade(*MARK);
>>
>>+ if (items > 1) {
>>+ /* APR::Brigade->flatten($p, $length); */
>>+ length = SvIV(*(MARK+1));
>
>
> without the pool argument, these can be wrapped up with mpxs_usage_va_2 + a new
>
> #define mpxs_xs_sv2_bb mpxs_xs_sv2_APR__Brigade
>
> which may not be any clearer, but at least it follows existing patterns.
Yeah, I did do that initially, but thought that you won't like it, so I've
kept it your way.
>>+ mpxs_APR__Brigade_flatten | | ...
>
>
> apr_brigade_pflatten should now get a different modifier than ~ in
> apr_functions.map.
why? it's implemented, no?
__________________________________________________________________
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: cvs commit: modperl-2.0/xs/tables/current/ModPerl FunctionTable.pm
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
Stas Bekman wrote:
> Doh! since my suggested change drops the use of apr_brigade_pflatten,
> all this magic is no longer needed. just use the grow macro like all
> other functions do and pass SvPVX to flatter. and whoah, no longer we
> need the pool object! that's a goodness.
which makes it look very much like my first implementation of
apr_brigade_flatten only, which you objected to because it required looping
through the brigade twice to get the entire brigade - once for
apr_brigade_length, and once for apr_brigade_flatten. but I guess it makes
for a better API to give the entire brigade back if no length argument is
given, and we did reorder the function a bit, so all is better in the end :)
> + bb = mp_xs_sv2_APR__Brigade(*MARK);
>
> + if (items > 1) {
> + /* APR::Brigade->flatten($p, $length); */
> + length = SvIV(*(MARK+1));
without the pool argument, these can be wrapped up with mpxs_usage_va_2 + a new
#define mpxs_xs_sv2_bb mpxs_xs_sv2_APR__Brigade
which may not be any clearer, but at least it follows existing patterns.
> + mpxs_APR__Brigade_flatten | | ...
apr_brigade_pflatten should now get a different modifier than ~ in
apr_functions.map.
other than that, it's fine with me, so go ahead.
--Geoff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: cvs commit: modperl-2.0/xs/tables/current/ModPerl FunctionTable.pm
Posted by Stas Bekman <st...@stason.org>.
Doh! since my suggested change drops the use of apr_brigade_pflatten, all this
magic is no longer needed. just use the grow macro like all other functions do
and pass SvPVX to flatter. and whoah, no longer we need the pool object!
that's a goodness.
Here's the reworked patch (against cvs):
Index: t/response/TestAPR/flatten.pm
===================================================================
RCS file: /home/cvs/modperl-2.0/t/response/TestAPR/flatten.pm,v
retrieving revision 1.2
diff -u -r1.2 flatten.pm
--- t/response/TestAPR/flatten.pm 27 Jan 2004 16:34:36 -0000 1.2
+++ t/response/TestAPR/flatten.pm 28 Jan 2004 23:22:06 -0000
@@ -36,50 +36,50 @@
$bb->length,
'APR::Brigade::length()');
- # syntax: always require a pool
- eval { $bb->flatten() };
+ # syntax: require a $bb
+ eval { APR::Brigade::flatten("") };
- ok t_cmp(qr/Usage: APR::Brigade::flatten/,
+ ok t_cmp(qr!expecting an APR::Brigade derived object!,
$@,
- 'APR::Brigade::flatten() requires a pool');
+ 'APR::Brigade::flatten() requires a brigade');
- # flatten($pool) will slurp up the entire brigade
+ # flatten() will slurp up the entire brigade
# equivalent to calling apr_brigade_pflatten
{
- my $data = $bb->flatten($pool);
+ my $data = $bb->flatten();
ok t_cmp(200000,
length($data),
- 'APR::Brigade::flatten() returned all the data');
+ '$bb->flatten() returned all the data');
# don't use t_cmp() here, else we get 200,000 characters
# to look at in verbose mode
- t_debug("APR::Brigade::flatten() data all 'x' characters");
+ t_debug("data all 'x' characters");
ok ($data !~ m/[^x]/);
}
- # syntax: flatten($p, 0) is equivalent to flatten($p)
+ # flatten(0) returns 0 bytes
{
- my $data = $bb->flatten($pool, 0);
+ my $data = $bb->flatten(0);
- ok t_cmp(200000,
+ t_debug('$bb->flatten(0) returns a defined value');
+ ok (defined $data);
+
+ ok t_cmp(0,
length($data),
- 'APR::Brigade::flatten() returned all the data');
-
- t_debug("APR::Brigade::flatten() data all 'x' characters");
- ok ($data !~ m/[^x]/);
+ '$bb->flatten(0) returned no data');
}
- # flatten($pool, $length) will return the first $length bytes
+ # flatten($length) will return the first $length bytes
# equivalent to calling apr_brigade_flatten
{
# small
- my $data = $bb->flatten($pool, 30);
+ my $data = $bb->flatten(30);
ok t_cmp(30,
length($data),
- 'APR::Brigade::flatten() returned all the data');
+ '$bb->flatten(30) returned 30 characters');
t_debug("APR::Brigade::flatten() data all 'x' characters");
ok ($data !~ m/[^x]/);
@@ -87,38 +87,38 @@
{
# large
- my $data = $bb->flatten($pool, 190000);
+ my $data = $bb->flatten(190000);
ok t_cmp(190000,
length($data),
- 'APR::Brigade::flatten() returned all the data');
+ '$bb->flatten(190000) returned 19000 characters');
- t_debug("APR::Brigade::flatten() data all 'x' characters");
+ t_debug("data all 'x' characters");
ok ($data !~ m/[^x]/);
}
{
# more than enough
- my $data = $bb->flatten($pool, 300000);
+ my $data = $bb->flatten(300000);
ok t_cmp(200000,
length($data),
- 'APR::Brigade::flatten() returned all the data');
+ '$bb->flatten(300000) returned all 200000 characters');
- t_debug("APR::Brigade::flatten() data all 'x' characters");
+ t_debug("data all 'x' characters");
ok ($data !~ m/[^x]/);
}
# fetch from a brigade with no data in it
{
- my $data = APR::Brigade->new($pool, $ba)->flatten($pool);
+ my $data = APR::Brigade->new($pool, $ba)->flatten();
- t_debug('an empty brigade returns a defined value');
+ t_debug('empty brigade returns a defined value');
ok (defined $data);
ok t_cmp(0,
length($data),
- 'an empty brigade returns data of 0 length');
+ 'empty brigade returns data of 0 length');
}
Apache::OK;
Index: xs/APR/Brigade/APR__Brigade.h
===================================================================
RCS file: /home/cvs/modperl-2.0/xs/APR/Brigade/APR__Brigade.h,v
retrieving revision 1.7
diff -u -r1.7 APR__Brigade.h
--- xs/APR/Brigade/APR__Brigade.h 28 Jan 2004 13:54:27 -0000 1.7
+++ xs/APR/Brigade/APR__Brigade.h 28 Jan 2004 23:22:06 -0000
@@ -97,46 +97,39 @@
}
static MP_INLINE
-SV *mpxs_APR__Brigade_flatten(pTHX_ apr_bucket_brigade *bb,
- apr_pool_t *pool, SV *sv_len)
+SV *mpxs_APR__Brigade_flatten(pTHX_ I32 items,
+ SV **MARK, SV **SP)
{
- /* XXX we're deviating from the API here to try and make
- * the API more Perlish - nobody likes the idea of two
- * "in/out" arguments. and we generally don't ever need
- * the length anyway...
- */
-
- apr_status_t status;
- char *buffer;
+ apr_bucket_brigade *bb;
apr_size_t length;
+ apr_status_t status;
+ SV *data = newSV(0);
- if (SvTRUE(sv_len)) {
- /* APR::Brigade->flatten($p, $length);
- * use apr_brigade_flatten to get the first $length bytes
- *
- * note that $length must be non-zero to get here
- */
-
- length = mp_xs_sv2_apr_size_t(sv_len);
-
- /* since we always require a pool, we can allocate from it */
- buffer = apr_pcalloc(pool, length);
-
- status = apr_brigade_flatten(bb, buffer, &length);
+ if (items < 1) {
+ Perl_croak(aTHX_ "usage: $bb->flatten([$length])");
+ }
+
+ bb = mp_xs_sv2_APR__Brigade(*MARK);
+ if (items > 1) {
+ /* APR::Brigade->flatten($p, $length); */
+ length = SvIV(*(MARK+1));
}
else {
- /* APR::Brigade->flatten($p);
- * use apr_brigade_pflatten to slurp the entire brigade
- *
- * note that it doesn't matter what we pass in for length
+ /* APR::Brigade->flatten($p); */
+ /* can't use pflatten, because we can't realloc() memory
+ * allocated by pflatten. and we need to append '\0' to it in
+ * SV. so we copy pflatten's guts here.
*/
-
- status = apr_brigade_pflatten(bb, &buffer, &length, pool);
-
+ apr_off_t actual;
+ apr_brigade_length(bb, 1, &actual);
+ length = (apr_size_t)actual;
}
+ mpxs_sv_grow(data, length);
+
+ status = apr_brigade_flatten(bb, SvPVX(data), &length);
if (status != APR_SUCCESS) {
/* XXX croak?
* note that reading from an empty brigade will return
@@ -145,5 +138,8 @@
return &PL_sv_undef;
}
- return newSVpvn(buffer, length);
+ mpxs_sv_cur_set(data, length);
+ SvTAINTED_on(data);
+
+ return data;
}
Index: xs/maps/apr_functions.map
===================================================================
RCS file: /home/cvs/modperl-2.0/xs/maps/apr_functions.map,v
retrieving revision 1.68
diff -u -r1.68 apr_functions.map
--- xs/maps/apr_functions.map 28 Jan 2004 13:54:35 -0000 1.68
+++ xs/maps/apr_functions.map 28 Jan 2004 23:22:06 -0000
@@ -98,7 +98,7 @@
mpxs_APR__Brigade_concat #APR_BRIGADE_CONCAT
mpxs_APR__Brigade_empty #APR_BRIGADE_EMPTY
mpxs_APR__Brigade_length | | bb, read_all=1
- mpxs_APR__Brigade_flatten | | bb, pool, sv_len=0
+ mpxs_APR__Brigade_flatten | | ...
mpxs_APR__Brigade_pool
MODULE=APR::Bucket
__________________________________________________________________
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: cvs commit: modperl-2.0/xs/tables/current/ModPerl FunctionTable.pm
Posted by Stas Bekman <st...@stason.org>.
Geoffrey Young wrote:
>>but this is the same as newSVpvn(buffer, length);. You still allocate
>>the buffer twice (second time in mpxs_sv_grow which calls SvGROW). I've
>>attached the adjusted patch (of only this file), which allocates memory
>>only once. we should make it into a macro.
>
>
> well, I was just doing it the way it's done elsewhere.
show me where?
> but thanks for
> cleaning it up :)
Yours was clean. It just wasn't efficient.
>>+ buffer = apr_palloc(pool, length+1); /* +1 for '\0' */
>>+ status = apr_brigade_flatten(bb, buffer, &length);
>>+ buffer[length] = '\0'; /* it is going to be in sv */
>>+
>
>
>>+ else {
>>+ SV *data = newSV(0);
>>+ /* as we have already allocated the buffer, we don't want to
>>+ * use newSVpvn as it'll copy the buffer second time. But we
>>+ * can't use sv_usepvn(), designed for this purpose, because
>>+ * it'll try to Renew to add \n, and buffer is allocated from
>>+ * the apr_pool, not malloc. so we make our own sv, taking
>>+ * care of the '\0' business */
>>+ SvUPGRADE(data, SVt_PV);
>>+ SvPVX(data) = buffer;
>>+ SvLEN(data) = length;
>>+ SvCUR(data) = length;
>>+ /* XXX: shouldn't this be SvPOK_only_UTF8, and in other
>>+ * places? add a test with some utf8 data */
>>+ (void)SvPOK_only_UTF8(data);
>
>
> if this is the way to do it, I guess it should be done this way everywhere
> else that we are growing the SV prior to population? for instance
> modperl_io_apache.c calls apr_brigade_flatten after growing the SV (in
> Apache__RequestIO.h). in fact, grep for mpxs_sv_grow - it looks like each
> time it could be rewritten allocate the buffer via APR then copy the result
> to the SV instead of populating SvPVX indirectly.
Not quite so, please take a look at the code again:
mpxs_Apache__RequestRec_read calls:
mpxs_sv_grow(bufsv, len+offset);
at this moment bufsv's PVX field is already of size 'len+offset'. Next:
total = modperl_request_read(aTHX_ r, SvPVX(bufsv)+offset, len);
it passes a pointer to an already allocated buffer, which is next passed to:
tmp = buffer;
...
apr_brigade_flatten(bb, tmp, &read);
in modperl_request_read(), which does *not* allocates any new memory. Your
case was different as you were using pflatten which was allocating the memory
internally, then you were copying it in to the sv.
If you find any other places where we duplicate memory please let me know. It
certainly should be fixed.
It easy to check - if SvPVX is passed to some function after sv_grow, then it
doesn't allocate anything new.
__________________________________________________________________
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: cvs commit: modperl-2.0/xs/tables/current/ModPerl FunctionTable.pm
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
> but this is the same as newSVpvn(buffer, length);. You still allocate
> the buffer twice (second time in mpxs_sv_grow which calls SvGROW). I've
> attached the adjusted patch (of only this file), which allocates memory
> only once. we should make it into a macro.
well, I was just doing it the way it's done elsewhere. but thanks for
cleaning it up :)
> + buffer = apr_palloc(pool, length+1); /* +1 for '\0' */
> + status = apr_brigade_flatten(bb, buffer, &length);
> + buffer[length] = '\0'; /* it is going to be in sv */
> +
> + else {
> + SV *data = newSV(0);
> + /* as we have already allocated the buffer, we don't want to
> + * use newSVpvn as it'll copy the buffer second time. But we
> + * can't use sv_usepvn(), designed for this purpose, because
> + * it'll try to Renew to add \n, and buffer is allocated from
> + * the apr_pool, not malloc. so we make our own sv, taking
> + * care of the '\0' business */
> + SvUPGRADE(data, SVt_PV);
> + SvPVX(data) = buffer;
> + SvLEN(data) = length;
> + SvCUR(data) = length;
> + /* XXX: shouldn't this be SvPOK_only_UTF8, and in other
> + * places? add a test with some utf8 data */
> + (void)SvPOK_only_UTF8(data);
if this is the way to do it, I guess it should be done this way everywhere
else that we are growing the SV prior to population? for instance
modperl_io_apache.c calls apr_brigade_flatten after growing the SV (in
Apache__RequestIO.h). in fact, grep for mpxs_sv_grow - it looks like each
time it could be rewritten allocate the buffer via APR then copy the result
to the SV instead of populating SvPVX indirectly.
--Geoff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: cvs commit: modperl-2.0/xs/tables/current/ModPerl FunctionTable.pm
Posted by Stas Bekman <st...@stason.org>.
Geoffrey Young wrote:
> ok, both of these issues are addressed in the attached patch.
looks good, a few efficiency fixes are due
pcalloc shouldn't be used, unless you really need to 0 all the bytes. palloc
is faster.
> - return newSVpvn(buffer, length);
> + mpxs_sv_grow(data, length);
> + SvPVX(data) = buffer;
> + mpxs_sv_cur_set(data, length);
but this is the same as newSVpvn(buffer, length);. You still allocate the
buffer twice (second time in mpxs_sv_grow which calls SvGROW). I've attached
the adjusted patch (of only this file), which allocates memory only once. we
should make it into a macro.
> + /* the brigade could be anything, so taint it */
> + SvTAINTED_on(data);
that's a good idea!
Here is the patch (against cvs). I've added comments inlined, so you
understand what and why:
Index: xs/APR/Brigade/APR__Brigade.h
===================================================================
RCS file: /home/cvs/modperl-2.0/xs/APR/Brigade/APR__Brigade.h,v
retrieving revision 1.7
diff -u -r1.7 APR__Brigade.h
--- xs/APR/Brigade/APR__Brigade.h 28 Jan 2004 13:54:27 -0000 1.7
+++ xs/APR/Brigade/APR__Brigade.h 28 Jan 2004 21:33:20 -0000
@@ -97,46 +97,42 @@
}
static MP_INLINE
-SV *mpxs_APR__Brigade_flatten(pTHX_ apr_bucket_brigade *bb,
- apr_pool_t *pool, SV *sv_len)
+SV *mpxs_APR__Brigade_flatten(pTHX_ I32 items,
+ SV **MARK, SV **SP)
{
- /* XXX we're deviating from the API here to try and make
- * the API more Perlish - nobody likes the idea of two
- * "in/out" arguments. and we generally don't ever need
- * the length anyway...
- */
-
+ apr_bucket_brigade *bb;
+ apr_pool_t *pool;
+ apr_size_t length;
apr_status_t status;
char *buffer;
- apr_size_t length;
-
- if (SvTRUE(sv_len)) {
- /* APR::Brigade->flatten($p, $length);
- * use apr_brigade_flatten to get the first $length bytes
- *
- * note that $length must be non-zero to get here
- */
-
- length = mp_xs_sv2_apr_size_t(sv_len);
-
- /* since we always require a pool, we can allocate from it */
- buffer = apr_pcalloc(pool, length);
-
- status = apr_brigade_flatten(bb, buffer, &length);
+ if (items < 2) {
+ Perl_croak(aTHX_ "usage: $bb->flatten($pool, [$length])");
+ }
+
+ bb = mp_xs_sv2_APR__Brigade(*MARK);
+ pool = mp_xs_sv2_APR__Pool(*(MARK+1));
+
+ if (items > 2) {
+ /* APR::Brigade->flatten($p, $length); */
+ length = SvIV(*(MARK+2));
}
else {
- /* APR::Brigade->flatten($p);
- * use apr_brigade_pflatten to slurp the entire brigade
- *
- * note that it doesn't matter what we pass in for length
+ /* APR::Brigade->flatten($p); */
+ /* can't use pflatten, because we can't realloc() memory
+ * allocated by pflatten. and we need to append '\0' to it.
+ * so we copy pflatten's guts here.
*/
-
- status = apr_brigade_pflatten(bb, &buffer, &length, pool);
-
+ apr_off_t actual;
+ apr_brigade_length(bb, 1, &actual);
+ length = (apr_size_t)actual;
}
+ buffer = apr_palloc(pool, length+1); /* +1 for '\0' */
+ status = apr_brigade_flatten(bb, buffer, &length);
+ buffer[length] = '\0'; /* it is going to be in sv */
+
if (status != APR_SUCCESS) {
/* XXX croak?
* note that reading from an empty brigade will return
@@ -144,6 +140,25 @@
*/
return &PL_sv_undef;
}
+ else {
+ SV *data = newSV(0);
+ /* as we have already allocated the buffer, we don't want to
+ * use newSVpvn as it'll copy the buffer second time. But we
+ * can't use sv_usepvn(), designed for this purpose, because
+ * it'll try to Renew to add \n, and buffer is allocated from
+ * the apr_pool, not malloc. so we make our own sv, taking
+ * care of the '\0' business */
+ SvUPGRADE(data, SVt_PV);
+ SvPVX(data) = buffer;
+ SvLEN(data) = length;
+ SvCUR(data) = length;
+ /* XXX: shouldn't this be SvPOK_only_UTF8, and in other
+ * places? add a test with some utf8 data */
+ (void)SvPOK_only_UTF8(data);
+
+ /* the brigade could be anything, so taint it */
+ SvTAINTED_on(data);
- return newSVpvn(buffer, length);
+ return data;
+ }
}
__________________________________________________________________
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: cvs commit: modperl-2.0/xs/tables/current/ModPerl FunctionTable.pm
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
>
> Not only not intuitive, but wrong. Consider this case: some function
> returns the wanted amount of chars to slurp, let's say it decides that
> it doesn't want any more chars and sets $wanted to 0 and you passed it
> to $bb->flatten($pool, $wanted); now instead of getting an empty string,
> you get the whole brigade. You could check that $wanted is 0 and not
> call flatten, but hey why making it more error-prone to users.
yes, you're right
>> + return newSVpvn(buffer, length);
>
>
> this is a bad idea. You should install the buffer into the SV not copy it.
ok, both of these issues are addressed in the attached patch.
--Geoff
Re: cvs commit: modperl-2.0/xs/tables/current/ModPerl FunctionTable.pm
Posted by Stas Bekman <st...@stason.org>.
Geoffrey Young wrote:
>>> + return newSVpvn(buffer, length);
>>
>>
>>this is a bad idea. You should install the buffer into the SV not copy it.
>
>
> what SV?
a new SV that you return.
__________________________________________________________________
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: cvs commit: modperl-2.0/xs/tables/current/ModPerl FunctionTable.pm
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
> I see, now I understand why you had this 0 case special. You need to
> manipulate the perl stack here like other '...' functions do. In which
> case the last argument is optional.
yeah, ok
>
>> + return newSVpvn(buffer, length);
>
>
> this is a bad idea. You should install the buffer into the SV not copy it.
what SV?
--Geoff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org