Re: [PATCH] use xread where we are not checking for EAGAIN/EINTR
- From
Andy Whitcroft <apw@shadowen.org>
- Date
- Jan 8, 2007, 15:56 UTC
- Message-ID
- <45A2699F.5060100@shadowen.org>
- In-Reply-To
- <459E4270.9090307@shadowen.org>
Andy Whitcroft wrote:
Show 32 quoted lines
> 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
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))