You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jeff Trawick <tr...@gmail.com> on 2009/10/03 22:12:50 UTC
Re: svn commit: r821307 - in /httpd/httpd/trunk: include/scoreboard.h
modules/experimental/mod_noloris.c server/scoreboard.c
On Sat, Oct 3, 2009 at 3:54 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
> On 03.10.2009 14:54, jim@apache.org wrote:
> > Author: jim
> > Date: Sat Oct 3 12:54:35 2009
> > New Revision: 821307
> >
> > URL: http://svn.apache.org/viewvc?rev=821307&view=rev
> > Log:
> > Provide new ap_update_child_status_from_conn() mostly
> > for use with mod_noloris.c Add some logic protection, for
> > NULL ref, which shoulda be there in any case.
> >
> > Modified:
> > httpd/httpd/trunk/include/scoreboard.h
> > httpd/httpd/trunk/modules/experimental/mod_noloris.c
> > httpd/httpd/trunk/server/scoreboard.c
> >
>
>
> > Modified: httpd/httpd/trunk/server/scoreboard.c
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/scoreboard.c?rev=821307&r1=821306&r2=821307&view=diff
> >
> ==============================================================================
> > --- httpd/httpd/trunk/server/scoreboard.c (original)
> > +++ httpd/httpd/trunk/server/scoreboard.c Sat Oct 3 12:54:35 2009
> > @@ -490,6 +492,19 @@
> > status, r);
> > }
> >
> > +AP_DECLARE(int) ap_update_child_status_from_conn(ap_sb_handle_t *sbh,
> int status,
> > + conn_rec *c)
> > +{
> > + if (!sbh)
> > + return -1;
> > +
> > + request_rec fake_rec;
> > + fake_rec.connection = c;
>
> Shouldn't we set fake_rec.per_dir_config and fake_rec.server to NULL to
> play safe?
>
I think you're right, but it is still hard to follow. I think a bit of
refactoring is in order (attached, untexted).
Re: svn commit: r821307 - in /httpd/httpd/trunk: include/scoreboard.h
modules/experimental/mod_noloris.c server/scoreboard.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Sat, Oct 3, 2009 at 4:21 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 10/03/2009 10:12 PM, Jeff Trawick wrote:
> > On Sat, Oct 3, 2009 at 3:54 PM, Ruediger Pluem <rp...@apache.org>
> wrote:
> >
> >> On 03.10.2009 14:54, jim@apache.org wrote:
> >>> Author: jim
> >>> Date: Sat Oct 3 12:54:35 2009
> >>> New Revision: 821307
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=821307&view=rev
> >>> Log:
> >>> Provide new ap_update_child_status_from_conn() mostly
> >>> for use with mod_noloris.c Add some logic protection, for
> >>> NULL ref, which shoulda be there in any case.
> >>>
> >>> Modified:
> >>> httpd/httpd/trunk/include/scoreboard.h
> >>> httpd/httpd/trunk/modules/experimental/mod_noloris.c
> >>> httpd/httpd/trunk/server/scoreboard.c
> >>>
> >>
> >>> Modified: httpd/httpd/trunk/server/scoreboard.c
> >>> URL:
> >>
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/scoreboard.c?rev=821307&r1=821306&r2=821307&view=diff
> >>
> ==============================================================================
> >>> --- httpd/httpd/trunk/server/scoreboard.c (original)
> >>> +++ httpd/httpd/trunk/server/scoreboard.c Sat Oct 3 12:54:35 2009
> >>> @@ -490,6 +492,19 @@
> >>> status, r);
> >>> }
> >>>
> >>> +AP_DECLARE(int) ap_update_child_status_from_conn(ap_sb_handle_t *sbh,
> >> int status,
> >>> + conn_rec *c)
> >>> +{
> >>> + if (!sbh)
> >>> + return -1;
> >>> +
> >>> + request_rec fake_rec;
> >>> + fake_rec.connection = c;
> >> Shouldn't we set fake_rec.per_dir_config and fake_rec.server to NULL to
> >> play safe?
> >>
> >
> > I think you're right, but it is still hard to follow. I think a bit of
> > refactoring is in order (attached, untexted).
>
> +1. Looks better and cleaner. Maybe
>
> @@ -468,7 +465,6 @@
> ws->conn_bytes = 0;
> }
> if (r) {
> - conn_rec *c = r->connection;
>
> + if (!c) {
> + c = r->connection;
> + }
> apr_cpystrn(ws->client, ap_get_remote_host(c,
> r->per_dir_config,
> REMOTE_NOLOOKUP, NULL), sizeof(ws->client));
> copy_request(ws->request, sizeof(ws->request), r);
>
> to avoid someone calling it with setting r but forgetting to set c.
> But maybe I am just too cautious here :-).
>
I think you are; as it is scoreboard-internal, it should go kaboom if not
called properly ;)
Re: svn commit: r821307 - in /httpd/httpd/trunk: include/scoreboard.h
modules/experimental/mod_noloris.c server/scoreboard.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/03/2009 10:12 PM, Jeff Trawick wrote:
> On Sat, Oct 3, 2009 at 3:54 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
>> On 03.10.2009 14:54, jim@apache.org wrote:
>>> Author: jim
>>> Date: Sat Oct 3 12:54:35 2009
>>> New Revision: 821307
>>>
>>> URL: http://svn.apache.org/viewvc?rev=821307&view=rev
>>> Log:
>>> Provide new ap_update_child_status_from_conn() mostly
>>> for use with mod_noloris.c Add some logic protection, for
>>> NULL ref, which shoulda be there in any case.
>>>
>>> Modified:
>>> httpd/httpd/trunk/include/scoreboard.h
>>> httpd/httpd/trunk/modules/experimental/mod_noloris.c
>>> httpd/httpd/trunk/server/scoreboard.c
>>>
>>
>>> Modified: httpd/httpd/trunk/server/scoreboard.c
>>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/scoreboard.c?rev=821307&r1=821306&r2=821307&view=diff
>> ==============================================================================
>>> --- httpd/httpd/trunk/server/scoreboard.c (original)
>>> +++ httpd/httpd/trunk/server/scoreboard.c Sat Oct 3 12:54:35 2009
>>> @@ -490,6 +492,19 @@
>>> status, r);
>>> }
>>>
>>> +AP_DECLARE(int) ap_update_child_status_from_conn(ap_sb_handle_t *sbh,
>> int status,
>>> + conn_rec *c)
>>> +{
>>> + if (!sbh)
>>> + return -1;
>>> +
>>> + request_rec fake_rec;
>>> + fake_rec.connection = c;
>> Shouldn't we set fake_rec.per_dir_config and fake_rec.server to NULL to
>> play safe?
>>
>
> I think you're right, but it is still hard to follow. I think a bit of
> refactoring is in order (attached, untexted).
+1. Looks better and cleaner. Maybe
@@ -468,7 +465,6 @@
ws->conn_bytes = 0;
}
if (r) {
- conn_rec *c = r->connection;
+ if (!c) {
+ c = r->connection;
+ }
apr_cpystrn(ws->client, ap_get_remote_host(c, r->per_dir_config,
REMOTE_NOLOOKUP, NULL), sizeof(ws->client));
copy_request(ws->request, sizeof(ws->request), r);
to avoid someone calling it with setting r but forgetting to set c.
But maybe I am just too cautious here :-).
Regards
Rüdiger