[PATCH 1/1] mmap.2: improve example

[PATCH 1/1] mmap.2: improve example

[Rahul Bedarkar]

free resources on error paths and at the end after use.

Signed-off-by: Rahul Bedarkar <rahulbedarkar89-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 man2/mmap.2 | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/man2/mmap.2 b/man2/mmap.2
index 0f2f277..cede984 100644
--- a/man2/mmap.2
+++ b/man2/mmap.2
@@ -765,8 +765,10 @@ main(int argc, char *argv[])
     if (fd == \-1)
         handle_error("open");
 
-    if (fstat(fd, &sb) == \-1)           /* To obtain file size */
+    if (fstat(fd, &sb) == \-1) {          /* To obtain file size */
+        close(fd);
         handle_error("fstat");
+    }
 
     offset = atoi(argv[2]);
     pa_offset = offset & ~(sysconf(_SC_PAGE_SIZE) \- 1);
@@ -774,6 +776,7 @@ main(int argc, char *argv[])
 
     if (offset >= sb.st_size) {
         fprintf(stderr, "offset is past end of file\\n");
+        close(fd);
         exit(EXIT_FAILURE);
     }
 
@@ -789,18 +792,27 @@ main(int argc, char *argv[])
 
     addr = mmap(NULL, length + offset \- pa_offset, PROT_READ,
                 MAP_PRIVATE, fd, pa_offset);
-    if (addr == MAP_FAILED)
+    if (addr == MAP_FAILED) {
+        close(fd);
         handle_error("mmap");
+    }
 
     s = write(STDOUT_FILENO, addr + offset \- pa_offset, length);
     if (s != length) {
-        if (s == \-1)
+        if (s == \-1) {
+            munmap(addr, length + offset \- pa_offset);
+            close(fd);
             handle_error("write");
+        }
 
+        munmap(addr, length + offset \- pa_offset);
+        close(fd);
         fprintf(stderr, "partial write");
         exit(EXIT_FAILURE);
     }
 
+    munmap(addr, length + offset \- pa_offset);
+    close(fd);
     exit(EXIT_SUCCESS);
 }
 .fi
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] mmap.2: improve example

[Michael Kerrisk (man-pages)]

Hello Rahul,

On 06/02/2016 12:56 PM, Rahul Bedarkar wrote:
> free resources on error paths and at the end after use.
> 
> Signed-off-by: Rahul Bedarkar <rahulbedarkar89-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  man2/mmap.2 | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/man2/mmap.2 b/man2/mmap.2
> index 0f2f277..cede984 100644
> --- a/man2/mmap.2
> +++ b/man2/mmap.2
> @@ -765,8 +765,10 @@ main(int argc, char *argv[])
>      if (fd == \-1)
>          handle_error("open");
>  
> -    if (fstat(fd, &sb) == \-1)           /* To obtain file size */
> +    if (fstat(fd, &sb) == \-1) {          /* To obtain file size */
> +        close(fd);
>          handle_error("fstat");
> +    }
>  
>      offset = atoi(argv[2]);
>      pa_offset = offset & ~(sysconf(_SC_PAGE_SIZE) \- 1);
> @@ -774,6 +776,7 @@ main(int argc, char *argv[])
>  
>      if (offset >= sb.st_size) {
>          fprintf(stderr, "offset is past end of file\\n");
> +        close(fd);
>          exit(EXIT_FAILURE);
>      }
>  
> @@ -789,18 +792,27 @@ main(int argc, char *argv[])
>  
>      addr = mmap(NULL, length + offset \- pa_offset, PROT_READ,
>                  MAP_PRIVATE, fd, pa_offset);
> -    if (addr == MAP_FAILED)
> +    if (addr == MAP_FAILED) {
> +        close(fd);
>          handle_error("mmap");
> +    }
>  
>      s = write(STDOUT_FILENO, addr + offset \- pa_offset, length);
>      if (s != length) {
> -        if (s == \-1)
> +        if (s == \-1) {
> +            munmap(addr, length + offset \- pa_offset);
> +            close(fd);
>              handle_error("write");
> +        }
>  
> +        munmap(addr, length + offset \- pa_offset);
> +        close(fd);
>          fprintf(stderr, "partial write");
>          exit(EXIT_FAILURE);
>      }
>  
> +    munmap(addr, length + offset \- pa_offset);
> +    close(fd);
>      exit(EXIT_SUCCESS);
>  }
>  .fi

Given that handle_error() calls exit(), and that triggers close() 
on all FDs and munmap() on all mappings, I'm not sure this patch
improves things. Can you say some more on why you think it is 
needed?

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] mmap.2: improve example

[Rahul Bedarkar]

Hello Michael,

On Fri, Jun 3, 2016 at 4:59 AM, Michael Kerrisk (man-pages)
<mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hello Rahul,
>
>
> Given that handle_error() calls exit(), and that triggers close()
> on all FDs and munmap() on all mappings, I'm not sure this patch
> improves things. Can you say some more on why you think it is
> needed?

I think it is always better to free resources after use to avoid
memory leaks later. But doing that in such
simple example might be debatable.

May be at-least at the end we should call munmap() and close(). That
will cover munmap() usage as well.

Regards,

Rahul
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] mmap.2: improve example

[Michael Kerrisk (man-pages)]

On 06/03/2016 02:46 AM, Rahul Bedarkar wrote:
> Hello Michael,
> 
> On Fri, Jun 3, 2016 at 4:59 AM, Michael Kerrisk (man-pages)
> <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Hello Rahul,
>>
>>
>> Given that handle_error() calls exit(), and that triggers close()
>> on all FDs and munmap() on all mappings, I'm not sure this patch
>> improves things. Can you say some more on why you think it is
>> needed?
> 
> I think it is always better to free resources after use to avoid
> memory leaks later. But doing that in such
> simple example might be debatable.

Yep.

> May be at-least at the end we should call munmap() and close(). That
> will cover munmap() usage as well.

Yes, by way of example, I think it doesn't hurt to add the munmap() 
call especially. Added.

Thanks for the input.

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html