Created attachment 215224 [details] [review]
GMappedFile: Add g_mapped_file_get_bytes()

There's already a bug about turning GMappedFile into GBytes entirely

Looks like https://bugzilla.gnome.org/show_bug.cgi?id=666854

I'm not so sure about the plan to deprecate all the non-constructor GMappedFile API, precisely because of what chpe says in https://bugzilla.gnome.org/show_bug.cgi?id=666854#c6

It's completely valid to make writable mapped file instances.

Review of attachment 215224 [details] [review]:

Other than that, I'm ok with this api; turning gmappedfile into a gbytes outright is probably not going to happen, so this is the next-best thing...

::: glib/gmappedfile.c
@@ +408,3 @@
+ * Creates a new #GBytes which references the data mapped from @file.
+ * It is invalid to use this function if @file is mapped writable,
+ * because a #GBytes should be immutable.

why is the writability a particular concern ? it is only a problem if you actually modify the data, no ? After all, we don't demand a memory block that you pass into a gbytes constructor to be readonly...

The bigger concern is this, imo (from the docs of MAP_PRIVATE):

  It is unspecified whether changes made to the file after the
  mmap() call are visible in the mapped region.

(In reply to comment #4)

> why is the writability a particular concern ? it is only a problem if you
> actually modify the data, no ? After all, we don't demand a memory block that
> you pass into a gbytes constructor to be readonly...

glib/gbytes.c:61: * The data pointed to by this bytes must not be modified. 

> The bigger concern is this, imo (from the docs of MAP_PRIVATE):
> 
>   It is unspecified whether changes made to the file after the
>   mmap() call are visible in the mapped region.

If you were doing that you were already hitting undefined behavior.  Wrapping it in a GBytes doesn't change it.

(In reply to comment #5)
> (In reply to comment #4)
> 
> > why is the writability a particular concern ? it is only a problem if you
> > actually modify the data, no ? After all, we don't demand a memory block that
> > you pass into a gbytes constructor to be readonly...
> 
> glib/gbytes.c:61: * The data pointed to by this bytes must not be modified. 

Yes, so why not put the equivalent sentence here:

  The mmapped contents of the file must not be modified after creating this bytes object.


> > The bigger concern is this, imo (from the docs of MAP_PRIVATE):
> > 
> >   It is unspecified whether changes made to the file after the
> >   mmap() call are visible in the mapped region.
> 
> If you were doing that you were already hitting undefined behavior.  Wrapping
> it in a GBytes doesn't change it.

True, could still mention it in the docs, though.

The following fix has been pushed:
056d39c GMappedFile: Add g_mapped_file_get_bytes()

Created attachment 221548 [details] [review]
GMappedFile: Add g_mapped_file_get_bytes()

This is yet another API that has a data/length/refcount combination
that one might often want to turn into a GBytes.