[systemd-devel] Some thoughts about loop_read/loop_write in util.c

[cee1]

2013/9/11 Lennart Poettering <lennart at poettering.net>:
>> loop_read/loop_write:
>> http://cgit.freedesktop.org/systemd/systemd/tree/src/shared/util.c#n2179
>>
>> In a scenario of pipes, loop_read on read side, if the write side is
>> closed, loop_read will return 0 if do_poll is false(let's assume no
>> data available to read). When do_poll is true, it will return:
>> 1) 0, if write side is closed while loop_read is just doing a read
>> 2) or -EIO when poll returns pollfd.revent with POLLHUP flag set
>>
>> The behavior is not very consistent.
>> IMHO, it's preferred loop_read follows read behavior as much as
>> possible -- returns 0 to indicate end of a file here, e.g. We can try
>> to read 0 bytes when pollfd.revents != POLLIN.
>
> EOF and temporarily not being able to read more data is something very
> different.
>
> It might make sense to return EAGAIN if POLLHUP is set though.
>
> (But note that POLLHUP has more complex semantics when combined with
> shutdown() and half-open connections...)
>
>> The same with loop_write.
>
> EOF doesn't exist for loop_write(), so this is even weirder....
Sorry for not make myself clear, I mean EPIPE.

What about the following patch? It simply do read/write again if poll
returns, and let read/write report error if something is wrong.

>From 3b83e839ebfc161565db76ce8d0e1dd4da1b0afc Mon Sep 17 00:00:00 2001
From: Chen Jie <chenj at lemote.com>
Date: Thu, 12 Sep 2013 09:21:41 +0800
Subject: [PATCH] util.c: not check pollfd.revent for loop_read/loop_write

If an error occurred, let read/write report it.
---
 src/shared/util.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/shared/util.c b/src/shared/util.c
index 1dde8af..e08ec44 100644
--- a/src/shared/util.c
+++ b/src/shared/util.c
@@ -2206,8 +2206,10 @@ ssize_t loop_read(int fd, void *buf, size_t
nbytes, bool do_poll) {
                                         return n > 0 ? n : -errno;
                                 }

+                                /*
                                 if (pollfd.revents != POLLIN)
                                         return n > 0 ? n : -EIO;
+                                 */

                                 continue;
                         }
@@ -2254,8 +2256,10 @@ ssize_t loop_write(int fd, const void *buf,
size_t nbytes, bool do_poll) {
                                         return n > 0 ? n : -errno;
                                 }

+                                /*
                                 if (pollfd.revents != POLLOUT)
                                         return n > 0 ? n : -EIO;
+                                 */

                                 continue;
                         }


-- 
Regards,

- cee1