Created attachment 159435 [details] [review]
GUnixInputStream, GUnixOutputStream: clarify the problems with regular files

funny, we were just talking about this yesterday on IRC.

Perhaps we should have some way of creating a GFileInput/OutputStream
from an fd? Or else GUnix*Stream could be clever and do thread tricks
with file-based fds? (Can you tell if a file descriptor refers to a file?)

Attaching the patch so someone else can verify that I got the details
correct.

Well, poll() only tells you whether data is available for reading.  Whether you get blocked if you read is a different story :).  Can aio_* help?

As for detecting file fd, sure, fstat() provides:

           S_ISREG(m)  is it a regular file?

           S_ISDIR(m)  directory?

           S_ISCHR(m)  character device?

           S_ISBLK(m)  block device?

           S_ISFIFO(m) FIFO (named pipe)?

           S_ISLNK(m)  symbolic link? (Not in POSIX.1-1996.)

           S_ISSOCK(m) socket? (Not in POSIX.1-1996.)

(In reply to comment #2)
> Can aio_* help?

I thought aio was (a) useless, (b) not terribly portable, (c) not designed with libraries in mind (ie, it requires you to arbitrarily claim a particular signal number for yourself and there's no way to coordinate that with anyone else)?

The "do async reads by reading in a thread" code is in ginputstream.c (and likewise with writing and goutputstream.c), so all we'd have to do is just fall back to the default implementation if the fd is a file.  What would the test be? S_ISFIFO or S_ISSOCK supports poll "correctly", everything else doesn't?

actually, it's not quite that simple: gunixinputstream needs to poll the cancellable fd and the input fd at the same time in order to be cancellable properly. (glocalfileinputstream doesn't worry about that, presumably under the assumption that read() won't block *too* long.)

(In reply to comment #3)

> I thought aio was (a) useless, (b) not terribly portable, (c) not designed with
> libraries in mind (ie, it requires you to arbitrarily claim a particular signal
> number for yourself and there's no way to coordinate that with anyone else)?

I see.


> The "do async reads by reading in a thread" code is in ginputstream.c (and
> likewise with writing and goutputstream.c), so all we'd have to do is just fall
> back to the default implementation if the fd is a file.  What would the test
> be? S_ISFIFO or S_ISSOCK supports poll "correctly", everything else doesn't?

S_ISCHR also looks safe to me.  I'd say invert it around, assume IS_REG and IS_DIR may block, anything else is fine.

Should resolve IS_LINK?


(In reply to comment #4)
> (glocalfileinputstream doesn't worry about that, presumably under the
> assumption that read() won't block *too* long.)

Fix that then?

Err, nevermind the part about symlinks.  We don't need to follow them or anything.  fstat() can't return those.

Created attachment 164340 [details] [review]
GUnixInputStream, GUnixOutputStream: support ordinary files better

If the fd is not a pipe or socket, fall back to using threads to do
async I/O rather than poll, since poll doesn't work the way you want
for ordinary files.

---

I had this patch lying around on my hard drive, but I'm not sure now why
I hadn't attached it. The gio tests still pass with it...

The lseek hack is something we'd come up with on #nautilus a while back
when discussing GFileDescriptorBased stuff

Review of attachment 164340 [details] [review]:

::: gio/gunixinputstream.c
@@ +55,1 @@
  *

Referring to GLocalFileInputStream is not useful, since that class is not part of the public api and doesn't show up in the docs.

@@ +507,3 @@
+      return;
+    }
+

Not sure if this is really good enough; if you look at ginputstream.c, there are places where it does different things depending on class->read_async, e.g:

 if (class->read_async == g_input_stream_real_read_async)
    {
      /* Read is thread-using async fallback.
       * Make skip use threads too, so that we can use a possible sync skip
       * implementation. */

(In reply to comment #8)
> Not sure if this is really good enough; if you look at ginputstream.c, there
> are places where it does different things depending on class->read_async, e.g:

Ah. OK, in GInputStream, that's the only such check, and in GOutputStream, the only checks involve flush/flush_async, which we don't override, so we're ok.

For the skip/read thing, the current state of the patch will result in skip_async() always falling back to read_async() (and skip() always falling back to read()) while ideally we would use lseek() if !priv->is_pipe_or_socket.

OK, pushed this with the existing skip/skip_async implementations;
there was already a note about possibly optimizing them in the future
anyway.

Attachment 164340 [details] pushed as 9b4cc6e - GUnixInputStream, GUnixOutputStream: support ordinary files better