Re: [PATCH] Some minor CLD test program fixes

[Jeff Garzik <jeff@garzik.org>]

On 11/27/2009 06:20 PM, cmccabe@alumni.cmu.edu 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.


> 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.

	Jeff