What's the point to keep mmaped objectfile opened?

What's the point to keep mmaped objectfile opened?

[rixed]

[-- Attachment #1: Type: text/plain, Size: 634 bytes --]

I'm a little worried by the amount of file descriptors left opened
after the objcode is mmaped.
I saw this "note to self" from objcodes.c:

/* FIXME: we leak ourselves and the file descriptor. but then again so does                      
   dlopen(). */

So apparently the author was well aware of the problem but decided to
ignore it, despite the fd being used nowhere as far as I can see
(also, I don't understand the remark about dlopen leaking a fd).

Anyway, the attached patch (which closes the fd and stores -1 inplace
of it in the cell) seams to works and seams to fix the issue (ie. no more
pending opened fd for .go files)


[-- Attachment #2: guile-close-fd-on-mmap.patch --]
[-- Type: text/x-diff, Size: 719 bytes --]

diff --git a/libguile/objcodes.c b/libguile/objcodes.c
index 536094f..9fa8b00 100644
--- a/libguile/objcodes.c
+++ b/libguile/objcodes.c
@@ -203,12 +203,11 @@ make_objcode_from_file (int fd)
                                     scm_from_size_t (total_len)));
       }
 
-    /* FIXME: we leak ourselves and the file descriptor. but then again so does
-       dlopen(). */
+    (void) close (fd);
     return scm_permanent_object
       (scm_double_cell (SCM_MAKE_OBJCODE_TAG (SCM_OBJCODE_TYPE_MMAP, 0),
                         (scm_t_bits)(addr + strlen (SCM_OBJCODE_COOKIE)),
-                        SCM_UNPACK (scm_from_int (fd)), 0));
+                        SCM_UNPACK (scm_from_int (-1)), 0));
   }
 #else
   {

Re: What's the point to keep mmaped objectfile opened?

[Andy Wingo]

On Tue 13 Dec 2011 11:15, rixed@happyleptic.org writes:

> I'm a little worried by the amount of file descriptors left opened
> after the objcode is mmaped.
> I saw this "note to self" from objcodes.c:
>
> /* FIXME: we leak ourselves and the file descriptor. but then again so does                      
>    dlopen(). */
>
> So apparently the author was well aware of the problem but decided to
> ignore it, despite the fd being used nowhere as far as I can see
> (also, I don't understand the remark about dlopen leaking a fd).
>
> Anyway, the attached patch (which closes the fd and stores -1 inplace
> of it in the cell) seams to works and seams to fix the issue (ie. no more
> pending opened fd for .go files)

If you close the fd, isn't the memory unmapped?  Isn't that a bad thing?

Maybe I was just under a big misconception here :)

Andy
-- 
http://wingolog.org/

Re: What's the point to keep mmaped objectfile opened?

[rixed]

-[ Tue, Dec 13, 2011 at 01:42:35PM +0100, Andy Wingo ]----
> If you close the fd, isn't the memory unmapped?  Isn't that a bad thing?
> 
> Maybe I was just under a big misconception here :)

That's a common misconception. The mapping of VM space to disk storage
is unrelated to the file descriptor set (or any program would have at
least a file descriptor opened for itself, since any program is mapped
into memory).
Compare for instance cat /proc/1/maps with ls -l /proc/1/fd.
As I said, the attached patch, which do closes the fd, actually works
(ie make check and my personal program works). So the memory is clearly
not unmapped :-)

Re: What's the point to keep mmaped objectfile opened?

[Andy Wingo]

On Tue 13 Dec 2011 14:43, rixed@happyleptic.org writes:

> -[ Tue, Dec 13, 2011 at 01:42:35PM +0100, Andy Wingo ]----
>> If you close the fd, isn't the memory unmapped?  Isn't that a bad thing?
>> 
>> Maybe I was just under a big misconception here :)
>
> That's a common misconception. The mapping of VM space to disk storage
> is unrelated to the file descriptor set (or any program would have at
> least a file descriptor opened for itself, since any program is mapped
> into memory).
> Compare for instance cat /proc/1/maps with ls -l /proc/1/fd.
> As I said, the attached patch, which do closes the fd, actually works
> (ie make check and my personal program works). So the memory is clearly
> not unmapped :-)

I applied your patch.  Thanks very much for the patch, and for the
education :-)

Cheers,

Andy
-- 
http://wingolog.org/