You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2006/07/20 14:18:51 UTC
Re: svn commit: r423444 - in /httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem:
./ Makefile.in config5.m4 mod_plainmem.c mod_scoreboard.c mod_sharedmem.c
slotmem.h
Some comments inline.
Regards
Rüdiger
On 19.07.2006 14:18, jfclere@apache.org wrote:
> Author: jfclere
> Date: Wed Jul 19 05:18:10 2006
> New Revision: 423444
> --- httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_plainmem.c (added)
> +++ httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_plainmem.c Wed Jul 19 05:18:10 2006
> @@ -0,0 +1,145 @@
> +/* Copyright 1999-2006 The Apache Software Foundation or its licensors, as
> + * applicable.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
Please use the latest header approved by the board in all files.
> +}
> +static apr_status_t ap_slotmem_create(ap_slotmem_t **new, const char *name, apr_size_t item_size, int item_num, apr_pool_t *pool)
> +{
> + void *slotmem = NULL;
> + ap_slotmem_t *res;
> + ap_slotmem_t *next = globallistmem;
> + char *fname;
> + apr_status_t rv;
> +
> + fname = ap_server_root_relative(pool, name);
> +
> + /* first try to attach to existing slotmem */
> + if (next) {
> + for (;;) {
> + if (strcmp(next->name, fname) == 0) {
> + /* we already have it */
> + *new = next;
> + return APR_SUCCESS;
> + }
> + if (next->next)
Shouldn't this be (!next->next)?
> + break;
> + next = next->next;
> + }
> + }
> +
> + /* create the memory using the globalpool */
> + res = (ap_slotmem_t *) apr_pcalloc(globalpool, sizeof(ap_slotmem_t));
> + res->base = apr_pcalloc(globalpool, item_size * item_num);
> + if (!res->base)
> + return APR_ENOSHMAVAIL;
> + memset(res->base, 0, item_size * item_num);
Is this needed? You did a calloc.
> +
> + /* For the chained slotmem stuff */
> + res->name = apr_pstrdup(globalpool, fname);
> + res->size = item_size;
> + res->num = item_num;
> + res->next = NULL;
> + if (globallistmem==NULL)
> + globallistmem = res;
> + else
> + next->next = res;
> +
> + *new = res;
> + return APR_SUCCESS;
> +}
> +static apr_status_t ap_slotmem_mem(ap_slotmem_t *score, int id, void**mem)
> +{
> +
> + void *ptr = score->base + score->size * id;
> +
> + if (!score)
> + return APR_ENOSHMAVAIL;
Shouldn't this check happen before assigning a value to ptr?
>
> Added: httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_scoreboard.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_scoreboard.c?rev=423444&view=auto
> ==============================================================================
> --- httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_scoreboard.c (added)
> +++ httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_scoreboard.c Wed Jul 19 05:18:10 2006
> +static apr_status_t ap_slotmem_mem(ap_slotmem_t *score, int id, void**mem)
> +{
> + void *ptr;
> + if (!score)
> + return APR_ENOSHMAVAIL;
> +
> +#if PROXY_HAS_SCOREBOARD
> + if (ap_scoreboard_image)
> + ptr = (void *)ap_get_scoreboard_lb(id);
> +#else
> + return APR_ENOSHMAVAIL;
> +#endif
> +
> + if (!ptr)
> + return APR_ENOSHMAVAIL;
> + *mem = ptr;
> + ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL,
> + "ap_slotmem_mem: score %d", score);
Shouldn't this be APLOG_DEBUG?
>
> Added: httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_sharedmem.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_sharedmem.c?rev=423444&view=auto
> ==============================================================================
> --- httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_sharedmem.c (added)
> +++ httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_sharedmem.c Wed Jul 19 05:18:10 2006
>
> +static apr_status_t ap_slotmem_create(ap_slotmem_t **new, const char *name, apr_size_t item_size, int item_num, apr_pool_t *pool)
> +{
> + void *slotmem = NULL;
> + ap_slotmem_t *res;
> + ap_slotmem_t *next = globallistmem;
> + char *fname;
> + apr_status_t rv;
> +
> + fname = ap_server_root_relative(pool, name);
> +
> + /* first try to attach to existing slotmem */
> + if (next) {
> + for (;;) {
> + if (strcmp(next->name, fname) == 0) {
> + /* we already have it */
> + *new = next;
> + return APR_SUCCESS;
> + }
> + if (next->next)
Shouldn't this be (!next->next)?
> + break;
> + next = next->next;
> + }
> + }
> +
> + /* first try to attach to existing shared memory */
> + res = (ap_slotmem_t *) apr_pcalloc(globalpool, sizeof(ap_slotmem_t));
> + rv = apr_shm_attach(&res->shm, fname, globalpool);
> + if (rv == APR_SUCCESS) {
> + /* check size */
> + if (apr_shm_size_get(res->shm) != item_size * item_num) {
> + apr_shm_detach(res->shm);
> + res->shm = NULL;
> + return APR_EINVAL;
> + }
> + } else {
> + rv = apr_shm_create(&res->shm, item_size * item_num, fname, globalpool);
> + if (rv != APR_SUCCESS)
> + return rv;
> + memset(apr_shm_baseaddr_get(res->shm), 0, item_size * item_num);
> + }
> +
> + /* For the chained slotmem stuff */
> + res->name = apr_pstrdup(globalpool, fname);
> + res->base = apr_shm_baseaddr_get(res->shm);
> + res->size = item_size;
> + res->num = item_num;
> + res->next = NULL;
> + if (globallistmem==NULL)
> + globallistmem = res;
> + else
> + next->next = res;
> +
> + *new = res;
> + return APR_SUCCESS;
> +}
> +static apr_status_t ap_slotmem_mem(ap_slotmem_t *score, int id, void**mem)
> +{
> +
> + void *ptr = score->base + score->size * id;
> +
> + if (!score)
> + return APR_ENOSHMAVAIL;
Shouldn't this check happen before assigning a value to ptr?
> + if (id<0 || id>score->num)
> + return APR_ENOSHMAVAIL;
> + if (apr_shm_size_get(score->shm) != score->size * score->num)
> + return APR_ENOSHMAVAIL;
Is this check needed here again?
> +
> + if (!ptr)
> + return APR_ENOSHMAVAIL;
> + *mem = ptr;
> + return APR_SUCCESS;
> +}
> +
> +/* make sure the shared memory is cleaned */
> +static int initialize_cleanup(apr_pool_t *p, apr_pool_t *plog, apr_pool_t *ptemp, server_rec *s)
> +{
> + ap_slotmem_t *next = globallistmem;
> + while (next) { next = globallistmem;
> + next = next->next;
> + }
What is the purpose of this loop?
> + apr_pool_cleanup_register(p, &globallistmem, cleanup_slotmem, apr_pool_cleanup_null);
> + return OK;
> +}
> +
Re: svn commit: r423444 - in /httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem:
./ Makefile.in config5.m4 mod_plainmem.c mod_scoreboard.c mod_sharedmem.c
slotmem.h
Posted by Jean-frederic Clere <jf...@gmail.com>.
Brian Akins wrote:
> Jean-frederic Clere wrote:
>
>> Do you mean the proxy back-end connections?
>
>
> I am thinking of a more general purpose "slotmem" not particularly
> tied to proxy. Maybe have some "wrapper" functions that create a
> "slotmem" based on threads x procs and can be access using
> r->connection. (internally slotmem could use r->connection->id).
>
What was your idea? To have informations about the lastest requests?
The places where the slomem could be used usely are un the worker and
the process (servers and parent of the structure). should I go on
investigating this or there is no interest?
Another place were the slotmem could be used is in mod_jk and mod_proxy:
to store the sessionid to allow proxying back-end that aren't allow to
support a feature like the TC jvmroute. Comments there?
Cheers
Jean-Fredeirc
Re: svn commit: r423444 - in /httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem:
./ Makefile.in config5.m4 mod_plainmem.c mod_scoreboard.c mod_sharedmem.c
slotmem.h
Posted by Jean-frederic Clere <jf...@gmail.com>.
Brian Akins wrote:
> Jean-frederic Clere wrote:
>
>> Do you mean the proxy back-end connections?
>
>
> I am thinking of a more general purpose "slotmem" not particularly
> tied to proxy. Maybe have some "wrapper" functions that create a
> "slotmem" based on threads x procs and can be access using
> r->connection. (internally slotmem could use r->connection->id).
Yes the "slotmen" could be used for that.
Cheers
Jean-Frederic
Re: svn commit: r423444 - in /httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem:
./ Makefile.in config5.m4 mod_plainmem.c mod_scoreboard.c mod_sharedmem.c
slotmem.h
Posted by Brian Akins <br...@turner.com>.
Jean-frederic Clere wrote:
> Do you mean the proxy back-end connections?
I am thinking of a more general purpose "slotmem" not particularly tied
to proxy. Maybe have some "wrapper" functions that create a "slotmem"
based on threads x procs and can be access using r->connection.
(internally slotmem could use r->connection->id).
--
Brian Akins
Chief Operations Engineer
Turner Digital Media Technologies
Re: svn commit: r423444 - in /httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem:
./ Makefile.in config5.m4 mod_plainmem.c mod_scoreboard.c mod_sharedmem.c
slotmem.h
Posted by Jean-frederic Clere <jf...@gmail.com>.
Brian Akins wrote:
> Ruediger Pluem wrote:
>
>> > +static apr_status_t ap_slotmem_create(ap_slotmem_t **new, const
>> char *name, apr_size_t item_size, int item_num, apr_pool_t *pool)
>
>
> In my thought of a "slotmem" or "scoreboard" item_num is "max threads
> * max procs" just like the "normal" scoreboard. Or has this morphed
> into something completely different?
That is not really different: A piece of shared memory that should be
write only by one writer.
If item_num = "max threads * max procs" then it is just like the
"normal" scoreboard.
> I can see uses for this type as well. Would be nice to have a
> function somewhere to get the current connections "scoreboard slot"
> perhaps...
Do you mean the proxy back-end connections?
Cheers
Jean-Frederic
Re: svn commit: r423444 - in /httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem:
./ Makefile.in config5.m4 mod_plainmem.c mod_scoreboard.c mod_sharedmem.c
slotmem.h
Posted by Brian Akins <br...@turner.com>.
Ruediger Pluem wrote:
> > +static apr_status_t ap_slotmem_create(ap_slotmem_t **new, const char
> *name, apr_size_t item_size, int item_num, apr_pool_t *pool)
In my thought of a "slotmem" or "scoreboard" item_num is "max threads *
max procs" just like the "normal" scoreboard. Or has this morphed into
something completely different? I can see uses for this type as well.
Would be nice to have a function somewhere to get the current
connections "scoreboard slot" perhaps...
--
Brian Akins
Chief Operations Engineer
Turner Digital Media Technologies
Re: svn commit: r423444 - in /httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem:
./ Makefile.in config5.m4 mod_plainmem.c mod_scoreboard.c mod_sharedmem.c
slotmem.h
Posted by Jean-frederic Clere <jf...@gmail.com>.
Ruediger Pluem wrote:
>Some comments inline.
>
>Regards
>
>
Many thanks
Cheers
Jean-Frederic
>Rüdiger
>
>
>On 19.07.2006 14:18, jfclere@apache.org wrote:
>
>
>>Author: jfclere
>>Date: Wed Jul 19 05:18:10 2006
>>New Revision: 423444
>>
>>
>
>
>
>>--- httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_plainmem.c (added)
>>+++ httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_plainmem.c Wed Jul 19 05:18:10 2006
>>@@ -0,0 +1,145 @@
>>+/* Copyright 1999-2006 The Apache Software Foundation or its licensors, as
>>+ * applicable.
>>+ *
>>+ * Licensed under the Apache License, Version 2.0 (the "License");
>>+ * you may not use this file except in compliance with the License.
>>+ * You may obtain a copy of the License at
>>+ *
>>+ * http://www.apache.org/licenses/LICENSE-2.0
>>+ *
>>+ * Unless required by applicable law or agreed to in writing, software
>>+ * distributed under the License is distributed on an "AS IS" BASIS,
>>+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>>+ * See the License for the specific language governing permissions and
>>+ * limitations under the License.
>>
>>
>
>Please use the latest header approved by the board in all files.
>
>
Done.
>
>
>>+}
>>+static apr_status_t ap_slotmem_create(ap_slotmem_t **new, const char *name, apr_size_t item_size, int item_num, apr_pool_t *pool)
>>+{
>>+ void *slotmem = NULL;
>>+ ap_slotmem_t *res;
>>+ ap_slotmem_t *next = globallistmem;
>>+ char *fname;
>>+ apr_status_t rv;
>>+
>>+ fname = ap_server_root_relative(pool, name);
>>+
>>+ /* first try to attach to existing slotmem */
>>+ if (next) {
>>+ for (;;) {
>>+ if (strcmp(next->name, fname) == 0) {
>>+ /* we already have it */
>>+ *new = next;
>>+ return APR_SUCCESS;
>>+ }
>>+ if (next->next)
>>
>>
>
>Shouldn't this be (!next->next)?
>
>
Yep.
>
>
>>+ break;
>>+ next = next->next;
>>+ }
>>+ }
>>+
>>+ /* create the memory using the globalpool */
>>+ res = (ap_slotmem_t *) apr_pcalloc(globalpool, sizeof(ap_slotmem_t));
>>+ res->base = apr_pcalloc(globalpool, item_size * item_num);
>>+ if (!res->base)
>>+ return APR_ENOSHMAVAIL;
>>+ memset(res->base, 0, item_size * item_num);
>>
>>
>
>Is this needed? You did a calloc.
>
>
Argh too fast in reusing the shared memory code.
>
>
>>+
>>+ /* For the chained slotmem stuff */
>>+ res->name = apr_pstrdup(globalpool, fname);
>>+ res->size = item_size;
>>+ res->num = item_num;
>>+ res->next = NULL;
>>+ if (globallistmem==NULL)
>>+ globallistmem = res;
>>+ else
>>+ next->next = res;
>>+
>>+ *new = res;
>>+ return APR_SUCCESS;
>>+}
>>+static apr_status_t ap_slotmem_mem(ap_slotmem_t *score, int id, void**mem)
>>+{
>>+
>>+ void *ptr = score->base + score->size * id;
>>+
>>+ if (!score)
>>+ return APR_ENOSHMAVAIL;
>>
>>
>
>Shouldn't this check happen before assigning a value to ptr?
>
>
Well after all the checks.
>
>
>
>>Added: httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_scoreboard.c
>>URL: http://svn.apache.org/viewvc/httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_scoreboard.c?rev=423444&view=auto
>>==============================================================================
>>--- httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_scoreboard.c (added)
>>+++ httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_scoreboard.c Wed Jul 19 05:18:10 2006
>>
>>
>
>
>
>>+static apr_status_t ap_slotmem_mem(ap_slotmem_t *score, int id, void**mem)
>>+{
>>+ void *ptr;
>>+ if (!score)
>>+ return APR_ENOSHMAVAIL;
>>+
>>+#if PROXY_HAS_SCOREBOARD
>>+ if (ap_scoreboard_image)
>>+ ptr = (void *)ap_get_scoreboard_lb(id);
>>+#else
>>+ return APR_ENOSHMAVAIL;
>>+#endif
>>+
>>+ if (!ptr)
>>+ return APR_ENOSHMAVAIL;
>>+ *mem = ptr;
>>+ ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL,
>>+ "ap_slotmem_mem: score %d", score);
>>
>>
>
>Shouldn't this be APLOG_DEBUG?
>
>
I have removed the ap_log_error().
>
>
>
>>Added: httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_sharedmem.c
>>URL: http://svn.apache.org/viewvc/httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_sharedmem.c?rev=423444&view=auto
>>==============================================================================
>>--- httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_sharedmem.c (added)
>>+++ httpd/httpd/branches/httpd-proxy-scoreboard/modules/mem/mod_sharedmem.c Wed Jul 19 05:18:10 2006
>>
>>
>>
>
>
>
>
>>+static apr_status_t ap_slotmem_create(ap_slotmem_t **new, const char *name, apr_size_t item_size, int item_num, apr_pool_t *pool)
>>+{
>>+ void *slotmem = NULL;
>>+ ap_slotmem_t *res;
>>+ ap_slotmem_t *next = globallistmem;
>>+ char *fname;
>>+ apr_status_t rv;
>>+
>>+ fname = ap_server_root_relative(pool, name);
>>+
>>+ /* first try to attach to existing slotmem */
>>+ if (next) {
>>+ for (;;) {
>>+ if (strcmp(next->name, fname) == 0) {
>>+ /* we already have it */
>>+ *new = next;
>>+ return APR_SUCCESS;
>>+ }
>>+ if (next->next)
>>
>>
>
>Shouldn't this be (!next->next)?
>
>
Yep.
>
>
>>+ break;
>>+ next = next->next;
>>+ }
>>+ }
>>+
>>+ /* first try to attach to existing shared memory */
>>+ res = (ap_slotmem_t *) apr_pcalloc(globalpool, sizeof(ap_slotmem_t));
>>+ rv = apr_shm_attach(&res->shm, fname, globalpool);
>>+ if (rv == APR_SUCCESS) {
>>+ /* check size */
>>+ if (apr_shm_size_get(res->shm) != item_size * item_num) {
>>+ apr_shm_detach(res->shm);
>>+ res->shm = NULL;
>>+ return APR_EINVAL;
>>+ }
>>+ } else {
>>+ rv = apr_shm_create(&res->shm, item_size * item_num, fname, globalpool);
>>+ if (rv != APR_SUCCESS)
>>+ return rv;
>>+ memset(apr_shm_baseaddr_get(res->shm), 0, item_size * item_num);
>>+ }
>>+
>>+ /* For the chained slotmem stuff */
>>+ res->name = apr_pstrdup(globalpool, fname);
>>+ res->base = apr_shm_baseaddr_get(res->shm);
>>+ res->size = item_size;
>>+ res->num = item_num;
>>+ res->next = NULL;
>>+ if (globallistmem==NULL)
>>+ globallistmem = res;
>>+ else
>>+ next->next = res;
>>+
>>+ *new = res;
>>+ return APR_SUCCESS;
>>+}
>>+static apr_status_t //(ap_slotmem_t *score, int id, void**mem)
>>+{
>>+
>>+ void *ptr = score->base + score->size * id;
>>+
>>+ if (!score)
>>+ return APR_ENOSHMAVAIL;
>>
>>
>
>Shouldn't this check happen before assigning a value to ptr?
>
>
Yes.
>
>
>>+ if (id<0 || id>score->num)
>>+ return APR_ENOSHMAVAIL;
>>+ if (apr_shm_size_get(score->shm) != score->size * score->num)
>>+ return APR_ENOSHMAVAIL;
>>
>>
>
>Is this check needed here again?
>
>
No
>
>
>>+
>>+ if (!ptr)
>>+ return APR_ENOSHMAVAIL;
>>+ *mem = ptr;
>>+ return APR_SUCCESS;
>>+}
>>+
>>
>>
>
>
>
>
>>+/* make sure the shared memory is cleaned */
>>+static int initialize_cleanup(apr_pool_t *p, apr_pool_t *plog, apr_pool_t *ptemp, server_rec *s)
>>+{
>>+ ap_slotmem_t *next = globallistmem;
>>+ while (next) { next = globallistmem;
>>+ next = next->next;
>>+ }
>>
>>
>
>What is the purpose of this loop?
>
>
Oops that was debugging code.
>
>
>>+ apr_pool_cleanup_register(p, &globallistmem, cleanup_slotmem, apr_pool_cleanup_null);
>>+ return OK;
>>+}
>>+
>>
>>
>
>
>
>