Message-ID: <20180627161447.mtbpqttiibx3ha7j@localhost.localdomain>
Date:   Wed, 27 Jun 2018 18:14:47 +0200
From:   Lukas Czerner <lczerner@...hat.com>
To:     "Theodore Y. Ts'o" <tytso@....edu>
Cc:     linux-ext4@...r.kernel.org, adilger@...ger.ca
Subject: Re: test f_extent_htree fails on 1.44.3-rc1

On Wed, Jun 27, 2018 at 10:44:39AM -0400, Theodore Y. Ts'o wrote:
> On Wed, Jun 27, 2018 at 02:32:05PM +0200, Lukas Czerner wrote:
> > 
> > just letting you know that indeed, when disabling set_inode_xattr() in
> > __populate_fs() the test runs ok. If you don't already have one I'll
> > work on a patch to add this option.
> 
> Already done, I just didn't have a chance to send it out last night.

Meanwhile I've created something as well. I am not yet ready to send it
since I was going to review and create test for it. It has some more
functionality

It changes the -d option to work like this:

Options for populating file system with existing directory are separated by
comas, and may take an argument which is set off by an equals ('=') sign.
Directory option must be specified.

Valid options are:
	directory=<path to the directory to copy>
	owner=<uid>:<gid>
	noxattr

so you can do

mke2fs -d directory=/path/to/whatever,owner=200:200,noxattr /dev/device

I think I can also make it backwards compatible if needed. Just let me
know if I should finish, or drop it in favour of your version please.

Thanks!
-Lukas


diff --git a/misc/create_inode.c b/misc/create_inode.c
index caa36095..986c71c4 100644
--- a/misc/create_inode.c
+++ b/misc/create_inode.c
@@ -108,7 +108,7 @@ static errcode_t add_link(ext2_filsys fs, ext2_ino_t parent_ino,
 
 /* Set the uid, gid, mode and time for the inode */
 static errcode_t set_inode_extra(ext2_filsys fs, ext2_ino_t ino,
-				 struct stat *st)
+				 struct stat *st, uid_t uid, gid_t gid)
 {
 	errcode_t		retval;
 	struct ext2_inode	inode;
@@ -119,8 +119,14 @@ static errcode_t set_inode_extra(ext2_filsys fs, ext2_ino_t ino,
 		return retval;
 	}
 
-	inode.i_uid = st->st_uid;
-	inode.i_gid = st->st_gid;
+	if (uid)
+		inode.i_uid = uid;
+	else
+		inode.i_uid = st->st_uid;
+	if (gid)
+		inode.i_gid = gid;
+	else
+		inode.i_gid = st->st_gid;
 	inode.i_mode |= st->st_mode;
 	inode.i_atime = st->st_atime;
 	inode.i_mtime = st->st_mtime;
@@ -712,13 +718,11 @@ static errcode_t path_append(struct file_info *target, const char *file)
 }
 
 /* Copy files from source_dir to fs */
-static errcode_t __populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
-			       const char *source_dir, ext2_ino_t root,
-			       struct hdlinks_s *hdlinks,
+static errcode_t __populate_fs(ext2_filsys fs, struct hdlinks_s *hdlinks,
 			       struct file_info *target,
-			       struct fs_ops_callbacks *fs_callbacks)
+			       struct populate_fs_opts *pfs)
 {
-	const char	*name;
+	char		*name;
 	DIR		*dh;
 	struct dirent	*dent;
 	struct stat	st;
@@ -729,19 +733,22 @@ static errcode_t __populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
 	int		read_cnt;
 	int		hdlink;
 	size_t		cur_dir_path_len;
+	struct populate_fs_opts	pfs_r;
 
-	if (chdir(source_dir) < 0) {
+	memset(&pfs_r, 0, sizeof(struct populate_fs_opts));
+
+	if (chdir(pfs->source_dir) < 0) {
 		retval = errno;
 		com_err(__func__, retval,
 			_("while changing working directory to \"%s\""),
-			source_dir);
+			pfs->source_dir);
 		return retval;
 	}
 
 	if (!(dh = opendir("."))) {
 		retval = errno;
 		com_err(__func__, retval,
-			_("while opening directory \"%s\""), source_dir);
+			_("while opening directory \"%s\""), pfs->source_dir);
 		return retval;
 	}
 
@@ -763,7 +770,7 @@ static errcode_t __populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
 		    st.st_nlink > 1) {
 			hdlink = is_hardlink(hdlinks, st.st_dev, st.st_ino);
 			if (hdlink >= 0) {
-				retval = add_link(fs, parent_ino,
+				retval = add_link(fs, pfs->parent_ino,
 						  hdlinks->hdl[hdlink].dst_ino,
 						  name);
 				if (retval) {
@@ -784,9 +791,9 @@ static errcode_t __populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
 			goto out;
 		}
 
-		if (fs_callbacks && fs_callbacks->create_new_inode) {
-			retval = fs_callbacks->create_new_inode(fs,
-				target->path, name, parent_ino, root,
+		if (pfs->fs_callbacks && pfs->fs_callbacks->create_new_inode) {
+			retval = pfs->fs_callbacks->create_new_inode(fs,
+				target->path, name, pfs->parent_ino, pfs->root,
 				st.st_mode & S_IFMT);
 			if (retval)
 				goto out;
@@ -798,7 +805,8 @@ static errcode_t __populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
 		case S_IFIFO:
 #ifndef _WIN32
 		case S_IFSOCK:
-			retval = do_mknod_internal(fs, parent_ino, name, &st);
+			retval = do_mknod_internal(fs, pfs->parent_ino, name,
+						   &st);
 			if (retval) {
 				com_err(__func__, retval,
 					_("while creating special file "
@@ -831,8 +839,8 @@ static errcode_t __populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
 				goto out;
 			}
 			ln_target[read_cnt] = '\0';
-			retval = do_symlink_internal(fs, parent_ino, name,
-						     ln_target, root);
+			retval = do_symlink_internal(fs, pfs->parent_ino, name,
+						     ln_target, pfs->root);
 			free(ln_target);
 			if (retval) {
 				com_err(__func__, retval,
@@ -843,8 +851,8 @@ static errcode_t __populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
 			break;
 #endif
 		case S_IFREG:
-			retval = do_write_internal(fs, parent_ino, name, name,
-						   root);
+			retval = do_write_internal(fs, pfs->parent_ino,
+						   name, name, pfs->root);
 			if (retval) {
 				com_err(__func__, retval,
 					_("while writing file \"%s\""), name);
@@ -853,26 +861,29 @@ static errcode_t __populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
 			break;
 		case S_IFDIR:
 			/* Don't choke on /lost+found */
-			if (parent_ino == EXT2_ROOT_INO &&
+			if (pfs->parent_ino == EXT2_ROOT_INO &&
 			    strcmp(name, "lost+found") == 0)
 				goto find_lnf;
-			retval = do_mkdir_internal(fs, parent_ino, name,
-						   root);
+			retval = do_mkdir_internal(fs, pfs->parent_ino, name,
+						   pfs->root);
 			if (retval) {
 				com_err(__func__, retval,
 					_("while making dir \"%s\""), name);
 				goto out;
 			}
 find_lnf:
-			retval = ext2fs_namei(fs, root, parent_ino,
+			retval = ext2fs_namei(fs, pfs->root, pfs->parent_ino,
 					      name, &ino);
 			if (retval) {
 				com_err(name, retval, 0);
 					goto out;
 			}
 			/* Populate the dir recursively*/
-			retval = __populate_fs(fs, ino, name, root, hdlinks,
-					       target, fs_callbacks);
+			memcpy(&pfs_r, pfs, sizeof(struct populate_fs_opts));
+			pfs_r.parent_ino = ino;
+			pfs_r.source_dir = name;
+
+			retval = __populate_fs(fs, hdlinks, target, &pfs_r);
 			if (retval)
 				goto out;
 			if (chdir("..")) {
@@ -887,30 +898,35 @@ find_lnf:
 				_("ignoring entry \"%s\""), name);
 		}
 
-		retval =  ext2fs_namei(fs, root, parent_ino, name, &ino);
+		retval =  ext2fs_namei(fs, pfs->root, pfs->parent_ino, name,
+				       &ino);
 		if (retval) {
 			com_err(name, retval, _("while looking up \"%s\""),
 				name);
 			goto out;
 		}
 
-		retval = set_inode_extra(fs, ino, &st);
+		retval = set_inode_extra(fs, ino, &st, pfs->uid, pfs->gid);
 		if (retval) {
 			com_err(__func__, retval,
 				_("while setting inode for \"%s\""), name);
 			goto out;
 		}
 
-		retval = set_inode_xattr(fs, ino, name);
-		if (retval) {
-			com_err(__func__, retval,
-				_("while setting xattrs for \"%s\""), name);
-			goto out;
+		if (!(pfs->flags & POPULATE_FS_NOXATTR)) {
+			retval = set_inode_xattr(fs, ino, name);
+			if (retval) {
+				com_err(__func__, retval,
+					_("while setting xattrs for \"%s\""),
+					name);
+				goto out;
+			}
 		}
 
-		if (fs_callbacks && fs_callbacks->end_create_new_inode) {
-			retval = fs_callbacks->end_create_new_inode(fs,
-				target->path, name, parent_ino, root,
+		if (pfs->fs_callbacks &&
+		    pfs->fs_callbacks->end_create_new_inode) {
+			retval = pfs->fs_callbacks->end_create_new_inode(fs,
+				target->path, name, pfs->parent_ino, pfs->root,
 				st.st_mode & S_IFMT);
 			if (retval)
 				goto out;
@@ -950,9 +966,7 @@ out:
 	return retval;
 }
 
-errcode_t populate_fs2(ext2_filsys fs, ext2_ino_t parent_ino,
-		       const char *source_dir, ext2_ino_t root,
-		       struct fs_ops_callbacks *fs_callbacks)
+errcode_t populate_fs3(ext2_filsys fs, struct populate_fs_opts *pfs)
 {
 	struct file_info file_info;
 	struct hdlinks_s hdlinks;
@@ -976,16 +990,30 @@ errcode_t populate_fs2(ext2_filsys fs, ext2_ino_t parent_ino,
 	file_info.path_max_len = 255;
 	file_info.path = calloc(file_info.path_max_len, 1);
 
-	retval = __populate_fs(fs, parent_ino, source_dir, root, &hdlinks,
-			       &file_info, fs_callbacks);
+	retval = __populate_fs(fs, &hdlinks, &file_info, pfs);
 
 	free(file_info.path);
 	free(hdlinks.hdl);
 	return retval;
 }
 
+errcode_t populate_fs2(ext2_filsys fs, ext2_ino_t parent_ino,
+		       char *source_dir, ext2_ino_t root,
+		       struct fs_ops_callbacks *fs_callbacks)
+{
+	struct populate_fs_opts pfs;
+
+	memset(&pfs, 0, sizeof(struct populate_fs_opts));
+	pfs.parent_ino = parent_ino;
+	pfs.root = root;
+	pfs.source_dir = source_dir;
+	pfs.fs_callbacks = fs_callbacks;
+
+	return populate_fs3(fs, &pfs);
+}
+
 errcode_t populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
-		      const char *source_dir, ext2_ino_t root)
+		      char *source_dir, ext2_ino_t root)
 {
 	return populate_fs2(fs, parent_ino, source_dir, root, NULL);
 }
diff --git a/misc/create_inode.h b/misc/create_inode.h
index 17309c68..83e109b1 100644
--- a/misc/create_inode.h
+++ b/misc/create_inode.h
@@ -33,12 +33,25 @@ struct fs_ops_callbacks {
 		ext2_ino_t parent_ino, ext2_ino_t root, mode_t mode);
 };
 
+#define POPULATE_FS_NOXATTR	1
+
+struct populate_fs_opts {
+	ext2_ino_t			parent_ino;
+	ext2_ino_t			root;
+	char 				*source_dir;
+	struct fs_ops_callbacks		*fs_callbacks;
+	uid_t				uid;
+	gid_t				gid;
+	int				flags;
+};
+
 /* For populating the filesystem */
 extern errcode_t populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
-			     const char *source_dir, ext2_ino_t root);
+			     char *source_dir, ext2_ino_t root);
 extern errcode_t populate_fs2(ext2_filsys fs, ext2_ino_t parent_ino,
-			      const char *source_dir, ext2_ino_t root,
+			      char *source_dir, ext2_ino_t root,
 			      struct fs_ops_callbacks *fs_callbacks);
+extern errcode_t populate_fs3(ext2_filsys fs, struct populate_fs_opts *);
 extern errcode_t do_mknod_internal(ext2_filsys fs, ext2_ino_t cwd,
 				   const char *name, struct stat *st);
 extern errcode_t do_symlink_internal(ext2_filsys fs, ext2_ino_t cwd,
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index ce78b7c8..14796a54 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -113,7 +113,7 @@ static char *mount_dir;
 char *journal_device;
 static int sync_kludge;	/* Set using the MKE2FS_SYNC env. option */
 char **fs_types;
-const char *src_root_dir;  /* Copy files from the specified directory */
+struct populate_fs_opts *pfs = NULL; /* Copy the contents of the fiven directory */
 static char *undo_file;
 
 static int android_sparse_file; /* -E android_sparse */
@@ -782,6 +782,96 @@ static int set_os(struct ext2_super_block *sb, char *os)
 
 #define PATH_SET "PATH=/sbin"
 
+static void parse_populate_opts(struct populate_fs_opts *pfs,
+			       const char *opts)
+{
+	char	*buf, *token, *next, *p, *arg, *badopt = 0;
+	int	len;
+	int	usage = 0;
+	int	ret, i;
+
+	len = strlen(opts);
+	buf = malloc(len + 1);
+	if (!buf) {
+		fprintf(stderr, "%s",
+			_("Couldn't allocate memory to parse options!\n"));
+		exit(1);
+	}
+	strncpy(buf, opts, len + 1);
+	for (token = buf; token && *token; token = next) {
+		p = strchr(token, ',');
+		next = 0;
+		if (p) {
+			*p = 0;
+			next = p+1;
+		}
+		arg = strchr(token, '=');
+		if (arg) {
+			*arg = 0;
+			arg++;
+		}
+
+		if (strcmp(token, "directory") == 0) {
+			if (!arg) {
+				usage++;
+				badopt = token;
+				continue;
+			}
+			pfs->source_dir = strndup(arg, PATH_MAX);
+		} else if (strcmp(token, "owner") == 0) {
+			if (arg) {
+				pfs->uid = strtoul(arg, &p, 0);
+				if (*p != ':') {
+					fprintf(stderr,
+						_("Invalid owner: '%s'\n"),
+						arg);
+					usage++;
+					continue;
+				}
+				p++;
+				pfs->gid = strtoul(p, &p, 0);
+				if (*p) {
+					fprintf(stderr,
+						_("Invalid owner: '%s'\n"),
+						arg);
+					usage++;
+					continue;
+				}
+			} else {
+				pfs->uid = getuid();
+				pfs->gid = getgid();
+			}
+		} else if (strcmp(token, "noxattr") == 0) {
+			pfs->flags |= POPULATE_FS_NOXATTR;
+		} else {
+			usage++;
+			badopt = token;
+		}
+	}
+	/* source dir is mandatory */
+	if (!pfs->source_dir)
+		usage++;
+
+	if (usage) {
+		fprintf(stderr, _("\nBad option(s) specified: %s\n\n"
+			"Options for populating file system with existing "
+			"directory are separated by\n"
+			"comas, and may take an argument which is set off "
+			"by an equals ('=') sign.\n"
+			"Directory option must be specified."
+			"\n\nValid options are:\n"
+			"\tdirectory=<path to the directory to copy>\n"
+			"\towner=<uid>:<gid>\n"
+			"\tnoxattr\n"),
+			badopt ? badopt : "");
+		free(buf);
+		exit(1);
+	}
+
+	pfs->parent_ino = EXT2_ROOT_INO;
+	pfs->root = EXT2_ROOT_INO;
+}
+
 static void parse_extended_opts(struct ext2_super_block *param,
 				const char *opts)
 {
@@ -1488,6 +1578,7 @@ static void PRS(int argc, char *argv[])
 	errcode_t	retval;
 	char *		oldpath = getenv("PATH");
 	char *		extended_opts = 0;
+	char *		populate_opts = 0;
 	char *		fs_type = 0;
 	char *		usage_types = 0;
 	/*
@@ -1615,7 +1706,7 @@ profile_error:
 			}
 			break;
 		case 'd':
-			src_root_dir = optarg;
+			populate_opts = optarg;
 			break;
 		case 'D':
 			direct_io = 1;
@@ -2348,6 +2439,17 @@ profile_error:
 	if (extended_opts)
 		parse_extended_opts(&fs_param, extended_opts);
 
+	if (populate_opts) {
+		pfs = malloc(sizeof(struct populate_fs_opts));
+		if (!pfs) {
+			com_err(program_name, ENOMEM, "%s",
+				_("in malloc for populate_fs_opts"));
+			exit(1);
+		}
+		memset(pfs, 0, sizeof(struct populate_fs_opts));
+		parse_populate_opts(pfs, populate_opts);
+	}
+
 	if (explicit_fssize == 0 && offset > 0) {
 		fs_blocks_count -= offset / EXT2_BLOCK_SIZE(&fs_param);
 		ext2fs_blocks_count_set(&fs_param, fs_blocks_count);
@@ -3280,18 +3382,24 @@ no_journal:
 	if (retval)
 		com_err(program_name, retval, "while creating huge files");
 	/* Copy files from the specified directory */
-	if (src_root_dir) {
+	if (pfs) {
 		if (!quiet)
 			printf("%s", _("Copying files into the device: "));
 
+/*
 		retval = populate_fs(fs, EXT2_ROOT_INO, src_root_dir,
 				     EXT2_ROOT_INO);
+		*/
+		retval = populate_fs3(fs, pfs);
 		if (retval) {
 			com_err(program_name, retval, "%s",
 				_("while populating file system"));
 			exit(1);
 		} else if (!quiet)
 			printf("%s", _("done\n"));
+		if (pfs->source_dir)
+			free(pfs->source_dir);
+		free(pfs);
 	}
 
 	if (!quiet)

> 
> commit 8cec4acdc03a449e8b193948ebce22fe4ad21324
> Author: Theodore Ts'o <tytso@....edu>
> Date:   Tue Jun 26 15:21:28 2018 -0400
> 
>     tests, mke2fs: add option to suppress xattr copying to fix f_extent_htree
>     
>     If the developer is running with SELinux enabled on /tmp, the
>     f_extent_htree regression test will fail because mke2fs by default
>     copies the extended attributes into the newly created file system (if
>     a directory hierarchy is specified via the -d option).
>     
>     Fix this by adding a new extended option to mke2fs, -E no_copy_xattrs
>     and using it in f_extent_htree's test script.
>     
>     Reported-by: Lukas Czerner <lczerner@...hat.com>
>     Signed-off-by: Theodore Ts'o <tytso@....edu>
> 
> diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
> index fe859d458..0b04508ef 100644
> --- a/debugfs/debugfs.c
> +++ b/debugfs/debugfs.c
> @@ -55,6 +55,7 @@ int sci_idx;
>  ext2_filsys	current_fs;
>  quota_ctx_t	current_qctx;
>  ext2_ino_t	root, cwd;
> +int		no_copy_xattrs;
>  
>  static int debugfs_setup_tdb(const char *device_name, char *undo_file,
>  			     io_manager *io_ptr)
> diff --git a/misc/create_inode.c b/misc/create_inode.c
> index 6621b0ab7..05aa63638 100644
> --- a/misc/create_inode.c
> +++ b/misc/create_inode.c
> @@ -142,6 +142,9 @@ static errcode_t set_inode_xattr(ext2_filsys fs, ext2_ino_t ino,
>  	char				*list = NULL;
>  	int				i;
>  
> +	if (no_copy_xattrs)
> +		return 0;
> +
>  	size = llistxattr(filename, NULL, 0);
>  	if (size == -1) {
>  		retval = errno;
> diff --git a/misc/create_inode.h b/misc/create_inode.h
> index 3a376322a..b5eeb420f 100644
> --- a/misc/create_inode.h
> +++ b/misc/create_inode.h
> @@ -33,6 +33,9 @@ struct fs_ops_callbacks {
>  		ext2_ino_t parent_ino, ext2_ino_t root, mode_t mode);
>  };
>  
> +extern int no_copy_xattrs; 	/* this should eventually be a flag
> +				   passed to populate_fs3() */
> +
>  /* For populating the filesystem */
>  extern errcode_t populate_fs(ext2_filsys fs, ext2_ino_t parent_ino,
>  			     const char *source_dir, ext2_ino_t root);
> diff --git a/misc/mke2fs.8.in b/misc/mke2fs.8.in
> index 7b989e0bd..603e37e54 100644
> --- a/misc/mke2fs.8.in
> +++ b/misc/mke2fs.8.in
> @@ -338,6 +338,15 @@ small risk if the system crashes before the journal has been overwritten
>  entirely one time.  If the option value is omitted, it defaults to 1 to
>  enable lazy journal inode zeroing.
>  .TP
> +.BI no_copy_xattrs
> +Normally
> +.B mke2fs
> +will copy the extended attributes of the files in the directory
> +hierarchy specified via the (optional)
> +.B \-d
> +option.  This will disable the copy and leaves the files in the newly
> +created file system without any extended attributes.
> +.TP
>  .BI num_backup_sb= <0|1|2>
>  If the
>  .B sparse_super2
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index ce78b7c82..b23ea766c 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -95,6 +95,7 @@ int	journal_size;
>  int	journal_flags;
>  static int	lazy_itable_init;
>  static int	packed_meta_blocks;
> +int		no_copy_xattrs;
>  static char	*bad_blocks_filename = NULL;
>  static __u32	fs_stride;
>  /* Initialize usr/grp quotas by default */
> @@ -880,6 +881,9 @@ static void parse_extended_opts(struct ext2_super_block *param,
>  				r_usage++;
>  				continue;
>  			}
> +		} else if (strcmp(token, "no_copy_xattrs") == 0) {
> +			no_copy_xattrs = 1;
> +			continue;
>  		} else if (strcmp(token, "num_backup_sb") == 0) {
>  			if (!arg) {
>  				r_usage++;
> diff --git a/tests/f_extent_htree/script b/tests/f_extent_htree/script
> index ccd97e14f..4939accc3 100644
> --- a/tests/f_extent_htree/script
> +++ b/tests/f_extent_htree/script
> @@ -30,8 +30,8 @@ fi
>  # make filesystem with enough inodes and blocks to hold all the test files
>  > $TMPFILE
>  NUM=$((NUM * 5 / 3))
> -echo "mke2fs -b $BSIZE -O dir_index,extent -d$SRC -N$NUM $TMPFILE $NUM" >> $OUT
> -$MKE2FS -b $BSIZE -O dir_index,extent -d$SRC -N$NUM $TMPFILE $NUM >> $OUT 2>&1
> +echo "mke2fs -b $BSIZE -O dir_index,extent -E no_copy_xattrs -d$SRC -N$NUM $TMPFILE $NUM" >> $OUT
> +$MKE2FS -b $BSIZE -O dir_index,extent -E no_copy_xattrs -d$SRC -N$NUM $TMPFILE $NUM >> $OUT 2>&1
>  rm -r $SRC
>  
>  # Run e2fsck to convert dir to htree before deleting the files, as mke2fs
> 
> 

Powered by blists - more mailing lists