Merge lp:~xnox/ubuntu/raring/libnih/fix-xfs into lp:ubuntu/raring/libnih

[Dimitri John Ledkov]

[Dimitri John Ledkov (xnox)]

I have tested this on ubuntu & debian and it fixes the referenced bugs.
But I still would like a second review before pushing to lp:ubuntu/libnih.

[Steve Langasek (vorlon)]

The main concern I have here is that we're not checking the return value of lstat, which means statbuf may be uninitialized and yield random behavior. If lstat() fails, we should assume it's not a directory.

The other consideration is that having to do an lstat for each entry just to check if it's a directory makes this walk a bit more expensive. According to readdir(3), if d_type isn't supported, its value should be set to DT_UNKNOWN. What about calling lstat *iff* d_type == DT_UNKNOWN? This would keep this speedy in the common case, but address the problem of portability to other filesystems.

[lp:~xnox/ubuntu/raring/libnih/fix-xfs]

1069. By Dimitri John Ledkov on 2012-12-11

use lstat, if d_type is not present

1070. By Dimitri John Ledkov on 2012-12-11

Check lstat return code

[Dimitri John Ledkov (xnox)]

I like your suggestion. d_type indeed is set to DT_UNKNOWN on xfs. Implemented your suggestions and tested on ext4 & xfs. All tests pass.

[lp:~xnox/ubuntu/raring/libnih/fix-xfs]

1071. By Dimitri John Ledkov on 2012-12-11

Update changelog entry

=== modified file 'debian/changelog'
--- debian/changelog	2012-10-25 10:57:30 +0000
+++ debian/changelog	2012-12-11 11:12:26 +0000
@@ -1,3 +1,11 @@
+libnih (1.0.3-4ubuntu13) UNRELEASED; urgency=low
+
+  [ Petr Lautrbach <[e-mail]>, Dmitrijs Ledkovs ]
+  * Fallback to lstat, if dirent.d_type is not available (not portable)
+    (LP: #672643) (Closes: #695604)
+
+ -- Dmitrijs Ledkovs <[e-mail]>  Mon, 10 Dec 2012 18:13:28 +0000
+
 libnih (1.0.3-4ubuntu12) raring; urgency=low
 
   * nih/logging.c: Use our own __nih_abort_msg rather than the (e)glibc
 
=== modified file 'nih/file.c'
--- nih/file.c	2010-02-04 22:30:23 +0000
+++ nih/file.c	2012-12-11 11:12:26 +0000
@@ -619,6 +619,8 @@
 	struct dirent  *ent;
 	char          **paths;
 	size_t          npaths;
+	int             isdir;
+	struct stat     statbuf;
 
 	nih_assert (path != NULL);
 
@@ -640,7 +642,15 @@
 		subpath = NIH_MUST (nih_sprintf (NULL, "%s/%s",
 						 path, ent->d_name));
 
-		if (filter && filter (data, subpath, ent->d_type == DT_DIR))
+		if (ent->d_type == DT_UNKNOWN) {
+			if ( lstat (subpath, &statbuf))
+				isdir = 0;
+			else
+				isdir = S_ISDIR(statbuf.st_mode);
+		} else
+			isdir = ent->d_type == DT_DIR;
+
+		if (filter && filter (data, subpath, isdir))
 			continue;
 
 		NIH_MUST (nih_str_array_addp (&paths, NULL, &npaths, subpath));
 
=== modified file 'nih/tests/test_file.c'
--- nih/tests/test_file.c	2010-02-04 22:30:23 +0000
+++ nih/tests/test_file.c	2012-12-11 11:12:26 +0000
@@ -724,6 +724,25 @@
 	return FALSE;
 }
 
+/* find only frodo files */
+static int
+my_filter_frodo_file (void       *data,
+	   const char *path,
+	   int         is_dir)
+{
+	char *slash;
+
+	if (is_dir)
+		return FALSE;
+
+	slash = strrchr (path, '/');
+	if (strcmp (slash, "/frodo"))
+		return TRUE;
+
+	return FALSE;
+}
+
+
 static int logger_called = 0;
 
 static int
@@ -905,6 +924,48 @@
 		TEST_EQ_STR (v->path, filename);
 
 		nih_free (visited);
+
+				/* Try also inverse filter */
+		TEST_ALLOC_SAFE {
+			visitor_called = 0;
+			visited = nih_list_new (NULL);
+		}
+
+		ret = nih_dir_walk (dirname, my_filter_frodo_file,
+				    my_visitor, NULL, &ret);
+
+		TEST_EQ (ret, 0);
+		TEST_EQ (visitor_called, 4);
+
+		v = (Visited *)visited->next;
+		TEST_EQ (v->data, &ret);
+		TEST_EQ_STR (v->dirname, dirname);
+		strcpy (filename, dirname);
+		strcat (filename, "/bar");
+		TEST_EQ_STR (v->path, filename);
+
+		v = (Visited *)v->entry.next;
+		TEST_EQ (v->data, &ret);
+		TEST_EQ_STR (v->dirname, dirname);
+		strcpy (filename, dirname);
+		strcat (filename, "/bar/frodo");
+		TEST_EQ_STR (v->path, filename);
+
+		v = (Visited *)v->entry.next;
+		TEST_EQ (v->data, &ret);
+		TEST_EQ_STR (v->dirname, dirname);
+		strcpy (filename, dirname);
+		strcat (filename, "/baz");
+		TEST_EQ_STR (v->path, filename);
+
+		v = (Visited *)v->entry.next;
+		TEST_EQ (v->data, &ret);
+		TEST_EQ_STR (v->dirname, dirname);
+		strcpy (filename, dirname);
+		strcat (filename, "/frodo");
+		TEST_EQ_STR (v->path, filename);
+
+		nih_free (visited);
 	}