apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: svn commit: r1561347 - /apr/apr/trunk/shmem/unix/shm.c
Date Sun, 26 Jan 2014 20:50:17 GMT
On Sat, Jan 25, 2014 at 6:57 PM, <jim@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.


Mime
View raw message