Created attachment 202339 [details] [review]
Add g_file_new_for_temp

I tried to create the function g_file_new_for_temp. I simply use g_file_open_tmp to create a file, close it, and then create GFile from its path. I guess it is not very safe but I don't see a way to make it so.

(documentation is heavily inspired from g_file_open_tmp and g_file_new_for_* functions)

Comment on attachment 202339 [details] [review]
Add g_file_new_for_temp

I just noticed a typo in the patch (g_unlink should take the path, not the fd)

I will create a new patch soon.

Comment on attachment 202339 [details] [review]
Add g_file_new_for_temp

Creating a file and then opening it is a wrapper for mktemp(), not mkstemp(), and creates security holes. g_file_make_temp() needs to actually create a GFile directly from the fd.

Created attachment 202413 [details] [review]
(rejected) Updated patch used in tests

Hi Dan, thanks for the review.

I understand that the approach was naive. I was simply writing it as a wrapper for a common way of accomplishing it that I see spread across a lot of gnome modules. Before I read your comment I had already updated the patch and altered gio/tests/file.c to use it. It replaces code that does pretty much the same thing there too. I'll attach the patch (and reject it as well) in case you want to have a look.

Do you have any hints how I might do this the right way? Should I create a GFileIOStream for the file and return that also? Like
GFile * g_file_new_for_temp (const gchar *, GFileIOStream **) ?

I am probably in above my head here though.

(In reply to comment #3)
> (From update of attachment 202339 [details] [review])
> Creating a file and then opening it is a wrapper for mktemp(), not mkstemp(),
> and creates security holes. g_file_make_temp() needs to actually create a GFile
> directly from the fd.

I thought a GFile was basically a convenient wrapper around a path. How do you create a GFile for an fd?

(In reply to comment #5)
> I thought a GFile was basically a convenient wrapper around a path. How do you
> create a GFile for an fd?

er, yeah, right. I guess what I meant was that g_file_new_temp() would create a GFile such that the first time you called a G*Stream-returning method on it, it would return a stream that wrapped the mkstemp fd, but after that it would behave just like a regular GLocalFile.

But maybe it would be simpler/better to just do:

  GFile *g_file_new_temp (const char  *template,
                          GIOStream  **iostream);

where iostream is an out parameter that returns a GLocalFileIOStream as though you had called g_file_create_readwrite() (with G_FILE_CREATE_PRIVATE) on the GFile, except that it is created directly from the fd returned by mkstemp.

I feel like it should be "g_file_new_temp", not "g_file_new_for_temp", because the "new_for" methods all mean "create a GFile representing (ie, 'for') some specific argument", which this new method does not.

Would this be total crack?

Get an fd with g_get_tmp_name (to allow setting mode 0666)

Create a GFile from name_used that is also returned from g_get_tmp_name

Use the fd in a function like:
GLocalFileIOStream  *
_g_local_file_io_stream_for_fd (int fd)    
  GLocalFileInputStream *input_stream;
  GLocalFileOutputStream *output_stream;
  GLocalFileIOStream *stream;
  
  output_stream = g_object_new (G_TYPE_LOCAL_FILE_OUTPUT_STREAM, NULL);
  output_stream->priv->fd = fd;

  input_stream = g_object_new (G_TYPE_LOCAL_FILE_INPUT_STREAM, NULL);
  input_stream->priv->fd = fd;  
  
  stream = g_object_new (G_TYPE_LOCAL_FILE_IO_STREAM, NULL);
  stream->output_stream = g_object_ref (output_stream);
  stream->input_stream = g_object_ref (input_stream);
  _g_local_file_output_stream_set_do_close (output_stream, FALSE);
  _g_local_file_input_stream_set_do_close (input_stream, FALSE);

  return stream;
}

and then do that with an api like you suggested? I could have a patch ready with that later today.

wait, that is crack. We do want 0600. I misread. Disregard that comment.

(In reply to comment #7)
>   output_stream = g_object_new (G_TYPE_LOCAL_FILE_OUTPUT_STREAM, NULL);
>   output_stream->priv->fd = fd;

Even ignoring the style issue of modifying someone else's private data, that won't compile, because GLocalFileOutputStreamPrivate is only defined inside glocalfileoutputstream.c.

You ned to add a _g_local_file_output_stream_new() that takes an fd (just like g_local_input_stream_new()), call that to create the output stream, and then pass that to _g_local_file_io_stream_new().

Created attachment 202781 [details] [review]
Add g_file_new_temp

Thank you for the pointers Dan. Would something along this line do? I suppose I should also do something more useful with the GError I pass around.

Comment on attachment 202781 [details] [review]
Add g_file_new_temp

ok, getting close...

>+GFile *
>+g_file_new_temp (const char     *template,
>+                 GFileIOStream **iostream)

This will need a gtk-doc comment of course. (You should be able to crib most of the content from the related gfileutils.c and gfile.c methods.) Also, you need to add g_file_new_temp to gio/gio.symbols and docs/reference/gio/gio-sections.txt (just after the other g_file_new* methods).

>+  if (fd != -1) {

>+  }
>+  else {

code style. should be

  if (fd != -1)
    {
      ...
    }
  else
    {
      ...
    }

>+    iface = G_FILE_GET_IFACE (file);
>+    *iostream = iface->open_readwrite_for_fd (fd, &error);

we don't want a public open_readwrite_for_fd() method on the interface; it's inconsistent with the rest of the interface and fds can cause tricky portability issues with Windows anyway. Just move your code from g_local_file_open_readwrite_for_fd() into g_file_new_temp() itself.

>+    g_warning ("Could not create temporary file: %s", error->message);

Don't do that. Just have g_file_new_temp() take a GError** and return the error to the caller.

Created attachment 203122 [details] [review]
Add g_file_new_for_temp

Thanks Dan. I updated the patch with your comments. It will probably need some work on the introspection annotations.

committed with a few doc fixes, and I also ended up renaming the
function to g_file_new_tmp() (rather than "temp") for consistency with
g_file_open_tmp() and g_get_tmp_dir().

(though unfortunately I forgot to change the commit message. oops)

Thanks for the patch.