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:54:39 GMT
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 <ylavic.dev@gmail.com> wrote:

> 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