Created attachment 187078 [details] [review]
GCancellable: Use g_unix_pipe_flags with FD_CLOEXEC

Comment on attachment 187078 [details] [review]
GCancellable: Use g_unix_pipe_flags with FD_CLOEXEC

yay assuming you tested against something

>+#include "glib.h"
>+#ifdef G_OS_UNIX
>+#include "glib-unix.h"

"No comment" :-)

Review of attachment 187078 [details] [review]:

::: gio/gcancellable.c
@@ +222,3 @@
        */
       set_fd_nonblocking (priv->cancel_pipe[0]);
       set_fd_nonblocking (priv->cancel_pipe[1]);

According to pipe2(2), you can pass O_NONBLOCK to pipe2(). Should g_unix_pipe_flags not support that ?

Review of attachment 187078 [details] [review]:

::: gio/gcancellable.c
@@ +222,3 @@
        */
       set_fd_nonblocking (priv->cancel_pipe[0]);
       set_fd_nonblocking (priv->cancel_pipe[1]);

According to pipe2(2), you can pass O_NONBLOCK to pipe2(). Should g_unix_pipe_flags not support that ?

(In reply to comment #4)
> Review of attachment 187078 [details] [review]:
> 
> ::: gio/gcancellable.c
> @@ +222,3 @@
>         */
>        set_fd_nonblocking (priv->cancel_pipe[0]);
>        set_fd_nonblocking (priv->cancel_pipe[1]);
> 
> According to pipe2(2), you can pass O_NONBLOCK to pipe2(). Should
> g_unix_pipe_flags not support that ?

We could, but the reason I didn't originally do this is:

* There are no race conditions related to it; the FD_CLOEXEC is the important one
* There are far fewer users of O_NONBLOCK with pipes
* I was a little worried about possible value collisions between the flags.  On Linux/glibc with fcntl(), setting close-on-exec and nonblock are two different operations; F_SETFD versus F_SETFL.  In theory O_NONBLOCK could have the same value as FD_CLOEXEC, though I admit in practice that's unlikely.

I guess if you were to add O_NONBLOCK, you'd have to use O_CLOEXEC instead of FD_CLOEXEC to avoid possible clashes...

oh, but you are right, the F_SETFD vs F_SETFL basically renders the point moot, I hadn't noticed that

Created attachment 187125 [details] [review]
g_unix_set_fd_nonblocking: New API to control file descriptor blocking state

And use it in relevant places in GLib.

Created attachment 187126 [details] [review]
GCancellable: Use g_unix_pipe_flags with FD_CLOEXEC

The old code was creating a pipe and setting FD_CLOEXEC
non-atomically.

This new API is more general and better - we can e.g. set only one end of the pipe nonblocking, as was used in gmain.c, and we also abstract unsetting the flag.

Review of attachment 187125 [details] [review]:

Looks like a nice cleanup

Review of attachment 187126 [details] [review]:

Looks good then, too

Attachment 187125 [details] pushed as ed37970 - g_unix_set_fd_nonblocking: New API to control file descriptor blocking state
Attachment 187126 [details] pushed as c078223 - GCancellable: Use g_unix_pipe_flags with FD_CLOEXEC