Re: [PATCH] use xread where we are not checking for EAGAIN/EINTR

[Andy Whitcroft <apw@shadowen.org>]

[-- Attachment #1: Type: text/plain, Size: 1869 bytes --]

Andy Whitcroft wrote:
> Junio C Hamano wrote:
>> Andy Whitcroft <apw@shadowen.org> writes:
>>
>>>     We have an xread() wrapper to help us with those nasty
>>>     interrupt returns and yet we fail to use it consistently.
>>>     This patch updates those plain read()'s which do not
>>>     have any handling for errors, or which treat those errors
>>>     as user visible fatal errors.
>>>
>>>     This feels right to me, but perhaps there is some good
>>>     reason that things are done this way ... if so could
>>>     someone elighten me.
>> Thanks.
>>
>> I do not think any of the changes you did introduced new bugs,
>> but I think some of them are still wrong.  xread() protects us
>> from EINTR happening before any byte is read, but it can still
>> give a short read.  Many callers have a loop like this:
>>
>> 	do {
>>         	size = xread(...);
>>                 yet_to_go -= size;
>> 	} while (yet_to_go);
>>
>> but some are not (e.g. add_excludes_from_file_1() in dir.c
>> expects xread() does not return before reading full buffer).
> 
> Yes, that is true.  I was going to fix that in the next step with the
> writes.  But yes thats likely to involve them becoming 'read_in_full'
> style thing and in fact churn us more.
> 
> Ignore this one and I'll look to do it 'right'.

Ok, after much hacking about I think I've got something sensible sorted
out for this.  Following this email are four patches which convert the
various read/write/xread/xwrite users over to xread/read_in_full and
xwrite/write_in_full as appropriate.  Its pretty invasive as obviously
I/O is pretty common in an SCM.

next with this stack passes the builtin test suite.  I additionally used
the attached patch to induce severe short read/write semantics.  Before
this patch series git was _very_ unhappy, unable to even create a
repository.

This series is against next.

-apw


[-- Attachment #2: PATCH --]
[-- Type: text/plain, Size: 688 bytes --]

diff --git a/git-compat-util.h b/git-compat-util.h
index 55456da..931cbed 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -210,6 +210,7 @@ static inline void *xmmap(void *start, size_t length,
 static inline ssize_t xread(int fd, void *buf, size_t len)
 {
 	ssize_t nr;
+	if (len > 5) len = 5;
 	while (1) {
 		nr = read(fd, buf, len);
 		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
@@ -221,6 +222,7 @@ static inline ssize_t xread(int fd, void *buf, size_t len)
 static inline ssize_t xwrite(int fd, const void *buf, size_t len)
 {
 	ssize_t nr;
+	if (len > 5) len = 5;
 	while (1) {
 		nr = write(fd, buf, len);
 		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))