On Sat, Nov 28, 2009 at 1:23 AM, Jeff Garzik <jeff@xxxxxxxxxx> wrote:
> On 11/27/2009 06:20 PM, cmccabe@xxxxxxxxxxxxxx wrote:
>>
>> --- a/lib/common.c
>> +++ b/lib/common.c
>> @@ -56,6 +56,37 @@ const char *cld_errstr(enum cle_err_codes ecode)
>> return cld_errlist[ecode];
>> }
>>
>> +/** Read from a file descriptor, resuming after interruptions.
>> + *
>> + * @param fd The file descriptor
>> + * @param buf Output buffer
>> + * @param len How many bytes to read
>> + *
>> + * @return Negative error code on error, or the number of
>> bytes
>> + * that were read. This will only be less than the
>> + * requested length if the file is too short.
>> + */
>> +int safe_read(int fd, void *buf, int len)
>> +{
>> + int rem = len;
>> + while (rem) {
>> + int res = read(fd, buf, rem);
>> + if (res == 0) {
>> + return len - rem;
>> + }
>> + else if (res< 0) {
>> + int err = errno;
>> + if (err != EINTR)
>> + return -err;
>> + }
>> + else {
>> + rem -= res;
>> + buf += res;
>> + }
>> + }
>> + return len;
>> +}
>> +
>> /*
>> * Read a port number from a port file, return the value or negative
>> error.
>> */
>> @@ -69,13 +100,13 @@ int cld_readport(const char *fname)
>>
>> if ((fd = open(fname, O_RDONLY)) == -1)
>> return -errno;
>> - rc = read(fd, buf, LEN);
>> + memset(buf, 0, sizeof(buf));
>> + rc = safe_read(fd, buf, LEN);
>> close(fd);
>> if (rc< 0)
>> - return -errno;
>> + return rc;
>> if (rc == 0)
>> return -EPIPE;
>> - buf[rc] = 0;
>>
>
> Well, while technically correct, nobody really bothers with EINTR or loops
> when reading from a file, because all Unix/Linux/BSD kernels have
> historically given you exactly the amount you needed. Any stray EINTR is
> often something serious that indicates the program should abort -- Ctrl-C at
> terminal by an impatient user, or a kill signal from an admin.
>
> Also, for reading whole files into memory, GLib's g_file_get_contents()
> already exists for that. It does full error checking.
>
> I do wonder what cygwin does, and if a read(2) loop matters on Windows...?
> Eventually I would like to see these programs portable enough to run under
> mingw or cygwin.
Good to know.
The manpages list EINTR in some surprising places... for example, close(2).
Apparently Alan Cox commented that "while the standard permits" EINTR
on close, "sanity suggests otherwise". :)
>
>
>> diff --git a/test/it-works.c b/test/it-works.c
>> index bd2f965..0e932e7 100644
>> --- a/test/it-works.c
>> +++ b/test/it-works.c
>> @@ -107,6 +107,7 @@ static int init(void)
>>
>> int main (int argc, char *argv[])
>> {
>> + g_thread_init(NULL);
>> cldc_init();
>> event_init();
>> if (init())
>> diff --git a/test/load-file-event.c b/test/load-file-event.c
>> index 1620508..23ee4e0 100644
>> --- a/test/load-file-event.c
>> +++ b/test/load-file-event.c
>> @@ -243,6 +243,7 @@ int main(int argc, char *argv[])
>> }
>> #endif
>>
>> + g_thread_init(NULL);
>> cldc_init();
>> event_init();
>> #if 0
>> diff --git a/test/lock-file-event.c b/test/lock-file-event.c
>> index c69da66..6fc4f1a 100644
>> --- a/test/lock-file-event.c
>> +++ b/test/lock-file-event.c
>> @@ -247,6 +247,7 @@ static int init(void)
>>
>> int main (int argc, char *argv[])
>> {
>> + g_thread_init(NULL);
>> cldc_init();
>> event_init();
>> if (init())
>> diff --git a/test/save-file-event.c b/test/save-file-event.c
>> index 1b3418a..23f1781 100644
>> --- a/test/save-file-event.c
>> +++ b/test/save-file-event.c
>> @@ -249,6 +249,7 @@ int main(int argc, char *argv[])
>> }
>> #endif
>>
>> + g_thread_init(NULL);
>> cldc_init();
>> event_init();
>
> This is OK, but should be in a separate patch.
>
> Similar to Linux kernel changes, logically separate changes should be in
> distinct, separate patches.
>
> That allows bisection ('git bisect') to better isolate problematic changes,
> and helps reviewers. It also means I can immediately apply one patch, while
> requesting revisions of another patch. Introduces fewer "stalls" in the
> programming pipeline.
Ah, ok
Colin
>
> Jeff
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
