[PATCH] Some minor CLD test program fixes

[PATCH] Some minor CLD test program fixes

[cmccabe]

From: cmccabe <cmccabe@alumni.cmu.edu>

Hi all! Here is a small patch to the CLD test programs...

When doing a raw read(2) in cld_readport(), resume after EINTR. Also resume if
we read less than the requested amount.

In the test programs, call g_thread_init() before cldc_init(). cldc_init has a
stern warning to "call cldc_init after g_thread_init, if present."
---
 lib/common.c           |   37 ++++++++++++++++++++++++++++++++++---
 test/it-works.c        |    1 +
 test/load-file-event.c |    1 +
 test/lock-file-event.c |    1 +
 test/save-file-event.c |    1 +
 5 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/lib/common.c b/lib/common.c
index 68f60f8..1b20980 100644
--- 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;
 
 	port = strtol(buf, NULL, 10);
 	if (port <= 0 || port >= 65636)
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();
 #if 0
-- 
1.6.2.5

Re: [PATCH] Some minor CLD test program fixes

[Pete Zaitcev]

On Fri, 27 Nov 2009 15:20:36 -0800
cmccabe@alumni.cmu.edu wrote:

> When doing a raw read(2) in cld_readport(), resume after EINTR.
> Also resume if we read less than the requested amount.

>  	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);

I don't think it's necessary when reading from a file.

> In the test programs, call g_thread_init() before cldc_init().
> cldc_init has a
> stern warning to "call cldc_init after g_thread_init, if present."

This may be a prudent thing to do, I just forgot about the
tests when I planned this.

-- Pete

Re: [PATCH] Some minor CLD test program fixes

[Jeff Garzik]

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

Re: [PATCH] Some minor CLD test program fixes

[Colin McCabe]

On Sat, Nov 28, 2009 at 1:23 AM, Jeff Garzik <jeff@garzik.org> wrote:
> 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.

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

Re: [PATCH] Some minor CLD test program fixes

[Jeff Garzik]

On 11/27/2009 06:20 PM, cmccabe@alumni.cmu.edu wrote:
> From: cmccabe<cmccabe@alumni.cmu.edu>
>
> Hi all! Here is a small patch to the CLD test programs...
>
> When doing a raw read(2) in cld_readport(), resume after EINTR. Also resume if
> we read less than the requested amount.
>
> In the test programs, call g_thread_init() before cldc_init(). cldc_init has a
> stern warning to "call cldc_init after g_thread_init, if present."
> ---
>   lib/common.c           |   37 ++++++++++++++++++++++++++++++++++---
>   test/it-works.c        |    1 +
>   test/load-file-event.c |    1 +
>   test/lock-file-event.c |    1 +
>   test/save-file-event.c |    1 +
>   5 files changed, 38 insertions(+), 3 deletions(-)

Oh, and two procedural notes:

1) As noted in doc/contributions.txt (and like the Linux kernel), all 
patches must include a Signed-off-by line.  This normally is the last 
line in the extended patch description, found in the email body.

2) Adding "From: ..." is rather pointless in this case, because the 
From: feature is used to denote an author other than yourself.  The 
default is taken from the email RFC822 headers, which appear to be fine 
in your case.  In the kernel, Andrew Morton uses this feature to 
indicate the original patch author, when he mails a huge swath of 
patches to Linus for application.

	Jeff

[PATCH v2 1/2] cld: in tests, call g_thread_init before cld_init

[Colin McCabe]

From: cmccabe <cmccabe@alumni.cmu.edu>

Signed-off-by: Colin McCabe <cmccabe@alumni.cmu.edu>

---
 test/it-works.c        |    1 +
 test/load-file-event.c |    1 +
 test/lock-file-event.c |    1 +
 test/save-file-event.c |    1 +
 4 files changed, 4 insertions(+), 0 deletions(-)

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();
 #if 0
-- 
1.6.2.5

Re: [PATCH v2 1/2] cld: in tests, call g_thread_init before cld_init

[Colin McCabe]

Oops... please ignore the From: line.

C.


On Fri, Nov 27, 2009 at 6:21 PM, Colin McCabe <cmccabe@alumni.cmu.edu> wrote:
> From: cmccabe <cmccabe@alumni.cmu.edu>
>
> Signed-off-by: Colin McCabe <cmccabe@alumni.cmu.edu>
>
> ---
>  test/it-works.c        |    1 +
>  test/load-file-event.c |    1 +
>  test/lock-file-event.c |    1 +
>  test/save-file-event.c |    1 +
>  4 files changed, 4 insertions(+), 0 deletions(-)
>
> 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();
>  #if 0
> --
> 1.6.2.5
>
>

Re: [PATCH v2 1/2] cld: in tests, call g_thread_init before cld_init

[Jeff Garzik]

On 11/27/2009 09:21 PM, Colin McCabe wrote:
> Signed-off-by: Colin McCabe<cmccabe@alumni.cmu.edu>
>
> ---
>   test/it-works.c        |    1 +
>   test/load-file-event.c |    1 +
>   test/lock-file-event.c |    1 +
>   test/save-file-event.c |    1 +
>   4 files changed, 4 insertions(+), 0 deletions(-)

applied