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 2018/05/29 19:53:27 UTC
Re: svn commit: r1831871 -
/httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c
On 05/18/2018 07:05 PM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Fri May 18 17:05:18 2018
> New Revision: 1831871
>
> URL: http://svn.apache.org/viewvc?rev=1831871&view=rev
> Log:
> mod_slotmem_shm: follow up to r1831869 (check persistent files).
>
> Since persistent files are also reused on stop/start, we must ensure that
> they match the same descriptor when reused on the next startup, so add it
> to integrity metadata.
>
> Also, the descriptor being the first field in the SHM, we don't need to
> copy on the stack it in several places, and can handle it as a pointer.
>
>
> Modified:
> httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c
>
> Modified: httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c?rev=1831871&r1=1831870&r2=1831871&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c (original)
> +++ httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c Fri May 18 17:05:18 2018
> @@ -200,37 +212,66 @@ static apr_status_t restore_slotmem(void
> rv = apr_file_open(&fp, storename, APR_READ | APR_WRITE, APR_OS_DEFAULT,
> pool);
> if (rv == APR_SUCCESS) {
> - rv = apr_file_read(fp, ptr, &nbytes);
> - if ((rv == APR_SUCCESS || rv == APR_EOF) && nbytes == size) {
> + rv = apr_file_read_full(fp, ptr, dsize, NULL);
> + if (rv == APR_SUCCESS || rv == APR_EOF) {
> rv = APR_SUCCESS; /* for successful return @ EOF */
> /*
> * if at EOF, don't bother checking md5
> * - backwards compatibility
> * */
> if (apr_file_eof(fp) != APR_EOF) {
> - apr_size_t ds = APR_MD5_DIGESTSIZE;
> - rv = apr_file_read(fp, digest, &ds);
> - if ((rv == APR_SUCCESS || rv == APR_EOF) &&
> - ds == APR_MD5_DIGESTSIZE) {
> - rv = APR_SUCCESS;
> - apr_md5(digest2, ptr, nbytes);
> + rv = apr_file_read_full(fp, digest, APR_MD5_DIGESTSIZE, NULL);
> + if (rv == APR_SUCCESS || rv == APR_EOF) {
.....
> + else if (rv == APR_SUCCESS || rv == APR_EOF) {
> + rv = APR_INCOMPLETE;
Hm, how can the condition in else if ever be true? We only get to else if rv is neither APR_SUCCESS or APR_EOF.
Regards
Rüdiger
Re: svn commit: r1831871 -
/httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c
Posted by Christophe Jaillet <ch...@wanadoo.fr>.
Le 29/05/2018 à 22:48, Yann Ylavic a écrit :
> On Tue, May 29, 2018 at 10:35 PM, Christophe Jaillet
> <ch...@wanadoo.fr> wrote:
>>
>> Also, httpd-2.4.x-balancer_slotmem.patch looks incomplete to me (but it is
>> maybe done on purpose).
>
> This is on purpose, or more exactly because trunk changes were
> restarted from 2.4.29's code, while 2.4.33 has already some of them.
> There is no diff between mod_slotmem_shm.c in trunk and 2.4.x once the
> backport proposal is applied...
>
>>
>>
>> The above (spurious) code is not included in the backport proposal.
>
> Was already in 2.4.33, but now with the update to the backport
> proposal it's also fixed.
>
>>
>> Also, some code included in r1831871 is also missing. For example, search
>> for APLOGNO(02551) or APLOGNO(02553) for example. Message and location have
>> change in r1831871, but not in the backport proposal.
>
> Same here, already in 2.4.33.
>
>>
>> Moreover, if the semantic of this message has changed, shouldn't we assign a
>> new number?
>
> The semantic didn't change (in 2.4.33 already), the new code adds new
> fields to the persisted file, so as much conditions but still with the
> same APLOGNOs.
>
> Hope that clarifies things, and thanks for the review Christophe!
>
>
> Regards,
> Yann.
>
Yep, crystal clear.
I hadn't followed all the do/undo/redo steps.
Sorry for the noise.
CJ
Re: svn commit: r1831871 - /httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, May 29, 2018 at 10:35 PM, Christophe Jaillet
<ch...@wanadoo.fr> wrote:
>
> Also, httpd-2.4.x-balancer_slotmem.patch looks incomplete to me (but it is
> maybe done on purpose).
This is on purpose, or more exactly because trunk changes were
restarted from 2.4.29's code, while 2.4.33 has already some of them.
There is no diff between mod_slotmem_shm.c in trunk and 2.4.x once the
backport proposal is applied...
>
>
> The above (spurious) code is not included in the backport proposal.
Was already in 2.4.33, but now with the update to the backport
proposal it's also fixed.
>
> Also, some code included in r1831871 is also missing. For example, search
> for APLOGNO(02551) or APLOGNO(02553) for example. Message and location have
> change in r1831871, but not in the backport proposal.
Same here, already in 2.4.33.
>
> Moreover, if the semantic of this message has changed, shouldn't we assign a
> new number?
The semantic didn't change (in 2.4.33 already), the new code adds new
fields to the persisted file, so as much conditions but still with the
same APLOGNOs.
Hope that clarifies things, and thanks for the review Christophe!
Regards,
Yann.
Re: svn commit: r1831871 -
/httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c
Posted by Christophe Jaillet <ch...@wanadoo.fr>.
Le 29/05/2018 à 21:53, Ruediger Pluem a écrit :
>
>> + rv = apr_file_read_full(fp, digest, APR_MD5_DIGESTSIZE, NULL);
>> + if (rv == APR_SUCCESS || rv == APR_EOF) {
>
> .....
>
>> + else if (rv == APR_SUCCESS || rv == APR_EOF) {
>> + rv = APR_INCOMPLETE;
>
> Hm, how can the condition in else if ever be true? We only get to else if rv is neither APR_SUCCESS or APR_EOF.
>
> Regards
>
> Rüdiger
>
>
Also, httpd-2.4.x-balancer_slotmem.patch looks incomplete to me (but it
is maybe done on purpose).
The above (spurious) code is not included in the backport proposal.
Also, some code included in r1831871 is also missing. For example,
search for APLOGNO(02551) or APLOGNO(02553) for example. Message and
location have change in r1831871, but not in the backport proposal.
Is it done on purpose?
Moreover, if the semantic of this message has changed, shouldn't we
assign a new number?
Just my 2c.
CJ
Re: svn commit: r1831871 - /httpd/httpd/trunk/modules/slotmem/mod_slotmem_shm.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, May 29, 2018 at 9:53 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
> .....
>
>> + else if (rv == APR_SUCCESS || rv == APR_EOF) {
>> + rv = APR_INCOMPLETE;
>
> Hm, how can the condition in else if ever be true? We only get to else if rv is neither APR_SUCCESS or APR_EOF.
Thanks Rüdiger, bad copy/paste all along :/
Fixed in r1832479 and added to backport proposal.
Regards,
Yann.