Make sure all the lines we read get freed (including the trailing blank)

Rakefile

@@ -14,9 +14,9 @@ TEST_OBJECTS = TEST_SOURCES.pathmap( "%{^tests,build/tests}X.o" )
 LIBS    = %w( pthread )
 CCFLAGS = %w(
              -D_GNU_SOURCE=1
-             -Wall 
+             -Wall
-             -Wextra 
+             -Wextra
-             -Werror-implicit-function-declaration 
+             -Werror-implicit-function-declaration
              -Wstrict-prototypes
              -Wno-missing-field-initializers
             ) # Added -Wno-missing-field-initializers to shut GCC up over {0} struct initialisers
@@ -93,7 +93,7 @@ rule 'build/flexnbd' => OBJECTS do |t|
 end
 
 
-file check("client") => 
+file check("client") =>
 %w{build/tests/check_client.o
   build/self_pipe.o
   build/nbdtypes.o
@@ -101,9 +101,9 @@ file check("client") =>
   build/readwrite.o
   build/parse.o
   build/client.o
-  build/serve.o 
+  build/serve.o
   build/acl.o
-  build/ioutil.o 
+  build/ioutil.o
   build/util.o} do |t|
   gcc_link t.name, t.prerequisites + [LIBCHECK]
 end
@@ -112,7 +112,7 @@ file check("acl") =>
 %w{build/tests/check_acl.o
   build/parse.o
   build/acl.o
-  build/util.o} do |t| 
+  build/util.o} do |t|
   gcc_link t.name, t.prerequisites + [LIBCHECK]
 end
 
@@ -124,9 +124,9 @@ file check("serve") =>
   build/readwrite.o
   build/parse.o
   build/client.o
-  build/serve.o 
+  build/serve.o
   build/acl.o
-  build/ioutil.o 
+  build/ioutil.o
   build/util.o} do |t|
   gcc_link t.name, t.prerequisites + [LIBCHECK]
 end
@@ -149,8 +149,8 @@ end
 
 (TEST_MODULES- %w{acl client serve readwrite}).each do |m|
   tgt = "build/tests/check_#{m}.o"
-  deps = ["build/ioutil.o", "build/util.o"]
   maybe_obj_name = "build/#{m}.o"
+  deps = ["build/ioutil.o", "build/util.o"] - [maybe_obj_name]
 
   deps << maybe_obj_name if OBJECTS.include?( maybe_obj_name )
 

src/ioutil.c

@@ -197,19 +197,27 @@ int splice_via_pipe_loop(int fd_in, int fd_out, size_t len)
 	return spliced < len ? -1 : 0;
 }
 
+/* Reads single bytes from fd until either an EOF or a newline appears.
+ * If an EOF occurs before a newline, returns -1.  The line is lost.
+ * Inserts the read bytes (without the newline) into buf, followed by a
+ * trailing NULL.
+ * Returns the number of read bytes: the length of the line without the
+ * newline, plus the trailing null.
+ */
 int read_until_newline(int fd, char* buf, int bufsize)
 {
 	int cur;
-	bufsize -=1;
 	
 	for (cur=0; cur < bufsize; cur++) {
 		int result = read(fd, buf+cur, 1);
-		if (result < 0) { return -1; }
+		if (result <= 0) { return -1; }
-		if (buf[cur] == 10) { break; }
+		if (buf[cur] == 10) { 
+			buf[cur] = '\0';
+			break; 
+		}
 	}
-	buf[cur++] = 0;
 	
-	return cur;
+	return cur+1;
 }
 
 int read_lines_until_blankline(int fd, int max_line_length, char ***lines)
@@ -221,9 +229,14 @@ int read_lines_until_blankline(int fd, int max_line_length, char ***lines)
 	memset(line, 0, max_line_length+1);
 	
 	while (1) {
-		if (read_until_newline(fd, line, max_line_length) < 0) {
+		int readden = read_until_newline(fd, line, max_line_length);
-			return lines_count;
+		/* readden will be:
-		}
+		 * 1 for an empty line
+		 * -1 for an eof
+		 * -1 for a read error
+		 */
+		if (readden <= 1) { return lines_count; }
+		fprintf(stderr, "Mallocing for %s\n", line );
 		*lines = xrealloc(*lines, (lines_count+1) * sizeof(char*));
 		(*lines)[lines_count] = strdup(line);
 		if ((*lines)[lines_count][0] == 0) {

tests/check_ioutil.c

@@ -0,0 +1,134 @@
+#include "ioutil.h"
+
+#include <check.h>
+
+START_TEST( test_read_until_newline_returns_line_length_plus_null )
+{
+	int fds[2];
+	int nread;
+	char buf[5] = {0};
+	pipe(fds);
+
+	write( fds[1], "1234\n", 5 );
+	
+	nread = read_until_newline( fds[0], buf, 5 );
+
+	ck_assert_int_eq( 5, nread );
+}
+END_TEST
+
+
+START_TEST( test_read_until_newline_inserts_null )
+{
+	int fds[2];
+	int nread;
+	char buf[5] = {0};
+	pipe(fds);
+
+	write( fds[1], "1234\n", 5 );
+	
+	nread = read_until_newline( fds[0], buf, 5 );
+
+	ck_assert_int_eq( '\0', buf[4] );
+
+}
+END_TEST
+
+
+START_TEST( test_read_empty_line_inserts_null )
+{
+	int fds[2];
+	int nread;
+	char buf[5] = {0};
+	pipe(fds);
+
+	write( fds[1], "\n", 1 );
+	nread = read_until_newline( fds[0], buf, 1 );
+
+	ck_assert_int_eq( '\0', buf[0] );
+	ck_assert_int_eq( 1, nread ); 
+}
+END_TEST
+
+
+START_TEST( test_read_eof_returns_err )
+{
+	int fds[2];
+	int nread;
+	char buf[5] = {0};
+	pipe( fds );
+
+	close( fds[1] );
+	nread = read_until_newline( fds[0], buf, 5 );
+
+	ck_assert_int_eq( -1, nread );
+}
+END_TEST
+
+
+START_TEST( test_read_eof_fills_line )
+{
+	int fds[2];
+	int nread;
+	char buf[5] = {0};
+	pipe(fds);
+
+	write( fds[1], "1234", 4 );
+	close( fds[1] );
+	nread = read_until_newline( fds[0], buf, 5 );
+
+	ck_assert_int_eq( -1, nread );
+	ck_assert_int_eq( '4', buf[3] );
+}
+END_TEST
+
+
+START_TEST( test_read_lines_until_blankline )
+{
+	char **lines = NULL;
+	int fds[2];
+	int nlines;
+	pipe( fds );
+
+	write( fds[1], "a\nb\nc\n\n", 7 );
+
+	nlines = read_lines_until_blankline( fds[0], 256, &lines );
+
+	ck_assert_int_eq( 3, nlines );
+}
+END_TEST
+
+
+Suite *ioutil_suite(void)
+{
+	Suite *s = suite_create("ioutil");
+
+	TCase *tc_read_until_newline = tcase_create("read_until_newline");
+	TCase *tc_read_lines_until_blankline = tcase_create("read_lines_until_blankline");
+
+	tcase_add_test(tc_read_until_newline, test_read_until_newline_returns_line_length_plus_null);
+	tcase_add_test(tc_read_until_newline, test_read_until_newline_inserts_null);
+	tcase_add_test(tc_read_until_newline, test_read_empty_line_inserts_null);
+	tcase_add_test(tc_read_until_newline, test_read_eof_returns_err);
+	tcase_add_test(tc_read_until_newline, test_read_eof_fills_line );
+
+	tcase_add_test(tc_read_lines_until_blankline, test_read_lines_until_blankline );
+
+	suite_add_tcase(s, tc_read_until_newline);
+	suite_add_tcase(s, tc_read_lines_until_blankline);
+
+	return s;
+}
+
+int main(void)
+{
+	int number_failed;
+	
+	Suite *s = ioutil_suite();
+	SRunner *sr = srunner_create(s);
+	srunner_run_all(sr, CK_NORMAL);
+	number_failed = srunner_ntests_failed(sr);
+	srunner_free(sr);
+	return (number_failed == 0) ? 0 : 1;
+}
+