You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Yann Ylavic <yl...@gmail.com> on 2014/01/26 21:50:17 UTC
Re: svn commit: r1561347 - /apr/apr/trunk/shmem/unix/shm.c
On Sat, Jan 25, 2014 at 6:57 PM, <ji...@apache.org> wrote:
> Author: jim
> Date: Sat Jan 25 17:57:25 2014
> New Revision: 1561347
>
> URL: http://svn.apache.org/r1561347
> Log:
> Revert mistaken c/p
>
> Modified:
> apr/apr/trunk/shmem/unix/shm.c
>
> Modified: apr/apr/trunk/shmem/unix/shm.c
> URL:
> http://svn.apache.org/viewvc/apr/apr/trunk/shmem/unix/shm.c?rev=1561347&r1=1561346&r2=1561347&view=diff
>
> ==============================================================================
> --- apr/apr/trunk/shmem/unix/shm.c (original)
> +++ apr/apr/trunk/shmem/unix/shm.c Sat Jan 25 17:57:25 2014
> @@ -325,7 +325,7 @@ APR_DECLARE(apr_status_t) apr_shm_create
> shm_unlink(shm_name); /* we're failing, remove the object */
> return status;
> }
> - new_m->base = mmap(NULL, new_m->realsize, PROT_READ | PROT_WRITE,
> + new_m->base = mmap(NULL, reqsize, PROT_READ | PROT_WRITE,
> MAP_SHARED, tmpfd, 0);
>
> /* FIXME: check for errors */
>
>
>
Didn't you revert a fix rather?
The following code is executed (below) for both APR_USE_SHMEM_MMAP_TMP and
APR_USE_SHMEM_MMAP_SHM :
/* store the real size in the metadata */
*(apr_size_t*)(new_m->base) = new_m->realsize;
/* metadata isn't usable */
new_m->usable = (char *)new_m->base +
APR_ALIGN_DEFAULT(sizeof(apr_size_t));
By using reqsize only for mmap()ing, the usable space is < reqsize, and the
user would be better not to access the trailing
APR_ALIGN_DEFAULT(sizeof(apr_size_t)) bytes...
Also, since the APR_USE_SHMEM_MMAP_TMP and APR_USE_SHMEM_MMAP_SHM codes
would be almost the same now, the following patch could be applied IMHO :
Index: shmem/unix/shm.c
===================================================================
--- shmem/unix/shm.c (revision 1561542)
+++ shmem/unix/shm.c (working copy)
@@ -301,24 +301,7 @@ APR_DECLARE(apr_status_t) apr_shm_create(apr_shm_t
apr_file_remove(new_m->filename, new_m->pool);
return status;
}
-
- status = apr_file_trunc(file, new_m->realsize);
- if (status != APR_SUCCESS && status != APR_ESPIPE) {
- apr_file_close(file); /* ignore errors, we're failing */
- apr_file_remove(new_m->filename, new_m->pool);
- return status;
- }
-
- new_m->base = mmap(NULL, new_m->realsize, PROT_READ | PROT_WRITE,
- MAP_SHARED, tmpfd, 0);
- /* FIXME: check for errors */
-
- status = apr_file_close(file);
- if (status != APR_SUCCESS) {
- return status;
- }
-#endif /* APR_USE_SHMEM_MMAP_TMP */
-#if APR_USE_SHMEM_MMAP_SHM
+#elif APR_USE_SHMEM_MMAP_SHM
/* FIXME: SysV uses 0600... should we? */
tmpfd = shm_open(shm_name, O_RDWR | O_CREAT | O_EXCL, 0644);
if (tmpfd == -1) {
@@ -331,6 +314,7 @@ APR_DECLARE(apr_status_t) apr_shm_create(apr_shm_t
if (status != APR_SUCCESS) {
return status;
}
+#endif /* APR_USE_SHMEM_MMAP_SHM */
status = apr_file_trunc(file, new_m->realsize);
if (status != APR_SUCCESS && status != APR_ESPIPE) {
@@ -347,7 +331,6 @@ APR_DECLARE(apr_status_t) apr_shm_create(apr_shm_t
if (status != APR_SUCCESS) {
return status;
}
-#endif /* APR_USE_SHMEM_MMAP_SHM */
/* store the real size in the metadata */
*(apr_size_t*)(new_m->base) = new_m->realsize;
[END]
Regards,
Yann.
Re: svn commit: r1561347 - /apr/apr/trunk/shmem/unix/shm.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
Yeah, I'm doing testing against that... it most
cases the caller already aligns the data, but it
does seem that apr-shm making sure does make sense,
and, as you see, it would be good to be consistent
with the other impl's. Right now I'm trying to find
a test case where using new_m->realsize makes a
difference...
On Jan 26, 2014, at 4:06 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Sun, Jan 26, 2014 at 9:54 PM, Yann Ylavic <yl...@gmail.com> wrote:
> @@ -338,7 +322,7 @@ APR_DECLARE(apr_status_t) apr_shm_create(apr_shm_t
> return status;
> }
> /* TODO: should we use new_m->realsize instead of reqsize ?? */
>
> It seems you already figured out this, I didn't see the comment (another commit)...
> Sorry for the noise (it should be fixed however IMHO).
>
Re: svn commit: r1561347 - /apr/apr/trunk/shmem/unix/shm.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Sun, Jan 26, 2014 at 9:54 PM, Yann Ylavic <yl...@gmail.com> wrote:
> @@ -338,7 +322,7 @@ APR_DECLARE(apr_status_t) apr_shm_create(apr_shm_t
> return status;
> }
> /* TODO: should we use new_m->realsize instead of reqsize ?? */
>
It seems you already figured out this, I didn't see the comment (another
commit)...
Sorry for the noise (it should be fixed however IMHO).
Re: svn commit: r1561347 - /apr/apr/trunk/shmem/unix/shm.c
Posted by Yann Ylavic <yl...@gmail.com>.
Oups, that's my bad c/p now, here is the good patch :
Index: shmem/unix/shm.c
===================================================================
--- shmem/unix/shm.c (revision 1561542)
+++ shmem/unix/shm.c (working copy)
@@ -301,24 +301,7 @@ APR_DECLARE(apr_status_t) apr_shm_create(apr_shm_t
apr_file_remove(new_m->filename, new_m->pool);
return status;
}
-
- status = apr_file_trunc(file, new_m->realsize);
- if (status != APR_SUCCESS && status != APR_ESPIPE) {
- apr_file_close(file); /* ignore errors, we're failing */
- apr_file_remove(new_m->filename, new_m->pool);
- return status;
- }
-
- new_m->base = mmap(NULL, new_m->realsize, PROT_READ | PROT_WRITE,
- MAP_SHARED, tmpfd, 0);
- /* FIXME: check for errors */
-
- status = apr_file_close(file);
- if (status != APR_SUCCESS) {
- return status;
- }
-#endif /* APR_USE_SHMEM_MMAP_TMP */
-#if APR_USE_SHMEM_MMAP_SHM
+#elif APR_USE_SHMEM_MMAP_SHM
/* FIXME: SysV uses 0600... should we? */
tmpfd = shm_open(shm_name, O_RDWR | O_CREAT | O_EXCL, 0644);
if (tmpfd == -1) {
@@ -331,6 +314,7 @@ APR_DECLARE(apr_status_t) apr_shm_create(apr_shm_t
if (status != APR_SUCCESS) {
return status;
}
+#endif /* APR_USE_SHMEM_MMAP_SHM */
status = apr_file_trunc(file, new_m->realsize);
if (status != APR_SUCCESS && status != APR_ESPIPE) {
@@ -338,7 +322,7 @@ APR_DECLARE(apr_status_t) apr_shm_create(apr_shm_t
return status;
}
/* TODO: should we use new_m->realsize instead of reqsize ?? */
- new_m->base = mmap(NULL, reqsize, PROT_READ | PROT_WRITE,
+ new_m->base = mmap(NULL, new_m->realsize, PROT_READ | PROT_WRITE,
MAP_SHARED, tmpfd, 0);
/* FIXME: check for errors */
@@ -347,7 +331,6 @@ APR_DECLARE(apr_status_t) apr_shm_create(apr_shm_t
if (status != APR_SUCCESS) {
return status;
}
-#endif /* APR_USE_SHMEM_MMAP_SHM */
/* store the real size in the metadata */
*(apr_size_t*)(new_m->base) = new_m->realsize;
[END]
On Sun, Jan 26, 2014 at 9:50 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Sat, Jan 25, 2014 at 6:57 PM, <ji...@apache.org> wrote:
>
>> Author: jim
>> Date: Sat Jan 25 17:57:25 2014
>> New Revision: 1561347
>>
>> URL: http://svn.apache.org/r1561347
>> Log:
>> Revert mistaken c/p
>>
>> Modified:
>> apr/apr/trunk/shmem/unix/shm.c
>>
>> Modified: apr/apr/trunk/shmem/unix/shm.c
>> URL:
>> http://svn.apache.org/viewvc/apr/apr/trunk/shmem/unix/shm.c?rev=1561347&r1=1561346&r2=1561347&view=diff
>>
>> ==============================================================================
>> --- apr/apr/trunk/shmem/unix/shm.c (original)
>> +++ apr/apr/trunk/shmem/unix/shm.c Sat Jan 25 17:57:25 2014
>> @@ -325,7 +325,7 @@ APR_DECLARE(apr_status_t) apr_shm_create
>> shm_unlink(shm_name); /* we're failing, remove the object */
>> return status;
>> }
>> - new_m->base = mmap(NULL, new_m->realsize, PROT_READ | PROT_WRITE,
>> + new_m->base = mmap(NULL, reqsize, PROT_READ | PROT_WRITE,
>> MAP_SHARED, tmpfd, 0);
>>
>> /* FIXME: check for errors */
>>
>>
>>
> Didn't you revert a fix rather?
>
> The following code is executed (below) for both APR_USE_SHMEM_MMAP_TMP and
> APR_USE_SHMEM_MMAP_SHM :
>
> /* store the real size in the metadata */
> *(apr_size_t*)(new_m->base) = new_m->realsize;
> /* metadata isn't usable */
> new_m->usable = (char *)new_m->base +
> APR_ALIGN_DEFAULT(sizeof(apr_size_t));
>
> By using reqsize only for mmap()ing, the usable space is < reqsize, and
> the user would be better not to access the trailing
> APR_ALIGN_DEFAULT(sizeof(apr_size_t)) bytes...
>
> Also, since the APR_USE_SHMEM_MMAP_TMP and APR_USE_SHMEM_MMAP_SHM codes
> would be almost the same now, the following patch could be applied IMHO :
>
> Index: shmem/unix/shm.c
> ===================================================================
> --- shmem/unix/shm.c (revision 1561542)
> +++ shmem/unix/shm.c (working copy)
> @@ -301,24 +301,7 @@ APR_DECLARE(apr_status_t) apr_shm_create(apr_shm_t
> apr_file_remove(new_m->filename, new_m->pool);
> return status;
> }
> -
> - status = apr_file_trunc(file, new_m->realsize);
> - if (status != APR_SUCCESS && status != APR_ESPIPE) {
> - apr_file_close(file); /* ignore errors, we're failing */
> - apr_file_remove(new_m->filename, new_m->pool);
> - return status;
> - }
>
> -
> - new_m->base = mmap(NULL, new_m->realsize, PROT_READ | PROT_WRITE,
> - MAP_SHARED, tmpfd, 0);
> - /* FIXME: check for errors */
> -
> - status = apr_file_close(file);
> - if (status != APR_SUCCESS) {
> - return status;
> - }
> -#endif /* APR_USE_SHMEM_MMAP_TMP */
> -#if APR_USE_SHMEM_MMAP_SHM
> +#elif APR_USE_SHMEM_MMAP_SHM
> /* FIXME: SysV uses 0600... should we? */
> tmpfd = shm_open(shm_name, O_RDWR | O_CREAT | O_EXCL, 0644);
> if (tmpfd == -1) {
> @@ -331,6 +314,7 @@ APR_DECLARE(apr_status_t) apr_shm_create(apr_shm_t
> if (status != APR_SUCCESS) {
> return status;
> }
> +#endif /* APR_USE_SHMEM_MMAP_SHM */
>
> status = apr_file_trunc(file, new_m->realsize);
> if (status != APR_SUCCESS && status != APR_ESPIPE) {
> @@ -347,7 +331,6 @@ APR_DECLARE(apr_status_t) apr_shm_create(apr_shm_t
> if (status != APR_SUCCESS) {
> return status;
> }
> -#endif /* APR_USE_SHMEM_MMAP_SHM */
>
> /* store the real size in the metadata */
> *(apr_size_t*)(new_m->base) = new_m->realsize;
> [END]
>
> Regards,
> Yann.
>
>