2013-08-28 05:30:32

by Robert Yang

[permalink] [raw]
Subject: [RFC 00/10] e2fsprogs/mke2fs: add an option: -d root-directory

This option is used for adding the files from the root-directory to the
filesystem, it is similiar to genext2fs, but genext2fs doesn't fully
support ext4.

* Questions
- Is such an option acceptable ?

- Most of the code have been in debugfs/debugfs.c already, I moved them to
misc/util.c and modified them to let both mke2fs and debugfs can use them,
maybe we should put these code in another separate file ?

- Where can I get the up-to-date development git repo, please? I think that
there would be conflicts with the dev git repo, I'd like to rebase it if I
can get the repo, currently, I'm using this one:

http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git

* The size impact on misc/mke2fs:
1,677,297 -> 1,728,110 (non stripped, about 50K increases)
316,968 -> 325,160 (stripped, 8K increases)

Please feel free to give your comments.

// Robert

Robert Yang (10):
mke2fs.c: add an option: -d root-directory
misc/util.c: implement populate_fs()
misc/util.c: create special file
misc/util.c: create symlink
misc/util.c: copy regular file
misc/util.c: create directory
misc/util.c: set more information for inode
misc/util.c: handle hardlinks
mke2fs.8.in: update the manual for the -d option
debugfs: use the functions in misc/util.c

debugfs/Makefile.in | 7 +-
debugfs/debugfs.c | 251 ++-----------------------
debugfs/debugfs.h | 1 +
misc/mke2fs.8.in | 7 +
misc/mke2fs.c | 39 +++-
misc/util.c | 519 ++++++++++++++++++++++++++++++++++++++++++++++++++++
misc/util.h | 32 ++++
7 files changed, 614 insertions(+), 242 deletions(-)

--
1.8.1.2



2013-08-28 05:30:45

by Robert Yang

[permalink] [raw]
Subject: [RFC 07/10] misc/util.c: set more information for inode

Set the uid, gid, mode and time for inode.

Signed-off-by: Robert Yang <[email protected]>
---
misc/util.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)

diff --git a/misc/util.c b/misc/util.c
index 7a45120..1a0a58c 100644
--- a/misc/util.c
+++ b/misc/util.c
@@ -314,6 +314,41 @@ void dump_mmp_msg(struct mmp_struct *mmp, const char *msg)
}
}

+/* Fill the uid, gid, mode and time for the inode */
+static void fill_inode(struct ext2_inode *inode, struct stat *st)
+{
+ if (st != NULL) {
+ inode->i_uid = st->st_uid;
+ inode->i_gid = st->st_gid;
+ inode->i_mode |= st->st_mode;
+ inode->i_atime = st->st_atime;
+ inode->i_mtime = st->st_mtime;
+ inode->i_ctime = st->st_ctime;
+ }
+}
+
+/* Set the uid, gid, mode and time for the inode */
+errcode_t set_inode_extra(ext2_ino_t cwd, ext2_ino_t ino, struct stat *st)
+{
+ errcode_t retval;
+ struct ext2_inode inode;
+ char *func_name = "set_inode_extra";
+
+ retval = ext2fs_read_inode(current_fs, ino, &inode);
+ if (retval) {
+ com_err(func_name, retval, "while reading inode %u", ino);
+ return retval;
+ }
+
+ fill_inode(&inode, st);
+
+ retval = ext2fs_write_inode(current_fs, ino, &inode);
+ if (retval) {
+ com_err(func_name, retval, "while writing inode %u", ino);
+ return retval;
+ }
+}
+
/* Make a special file which is block, character and fifo */
errcode_t do_mknod_internal(ext2_ino_t cwd, const char *name, struct stat *st)
{
@@ -687,6 +722,17 @@ errcode_t populate_fs(ext2_ino_t parent_ino, const char *source_dir)
com_err(func_name, 0,
_("ignoring entry \"%s\""), name);
}
+
+ if ((retval = ext2fs_namei(current_fs, root, parent_ino, name, &ino))){
+ com_err(name, retval, 0);
+ return retval;
+ }
+
+ if ((retval = set_inode_extra(parent_ino, ino, &st))) {
+ com_err(func_name, retval,
+ _("while setting inode for \"%s\""), name);
+ return retval;
+ }
}
closedir(dh);
return retval;
--
1.8.1.2


2013-08-28 05:30:35

by Robert Yang

[permalink] [raw]
Subject: [RFC 02/10] misc/util.c: implement populate_fs()

Use opendir() and readdir() to read the native directory, then use
lstat() to identify the file type and call the corresponding function to
add the file to the filesystem, call the populate_fs() recursively if it
is a directory.

NOTE: the libext2fs doesn't support create the socket file at them
moment.

Signed-off-by: Robert Yang <[email protected]>
---
misc/util.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 90 insertions(+)

diff --git a/misc/util.c b/misc/util.c
index cbc7cc0..6d87393 100644
--- a/misc/util.c
+++ b/misc/util.c
@@ -337,4 +337,94 @@ errcode_t do_write_internal(ext2_ino_t cwd, const char *src, const char *dest)
/* Copy files from source_dir to fs */
errcode_t populate_fs(ext2_ino_t parent_ino, const char *source_dir)
{
+ const char *name;
+ DIR *dh;
+ struct dirent *dent;
+ struct stat st;
+ char ln_target[PATH_MAX];
+ char *func_name = "populate_fs";
+ ext2_ino_t ino;
+ errcode_t retval;
+ int read_cnt;
+
+ root = EXT2_ROOT_INO;
+
+ if (chdir(source_dir) < 0) {
+ com_err(func_name, errno,
+ _("while changing working directory to \"%s\""), source_dir);
+ return errno;
+ }
+
+ if (!(dh = opendir("."))) {
+ com_err(func_name, errno,
+ _("while openning directory \"%s\""), source_dir);
+ return errno;
+ }
+
+ while((dent = readdir(dh))) {
+ if((!strcmp(dent->d_name, ".")) || (!strcmp(dent->d_name, "..")))
+ continue;
+ lstat(dent->d_name, &st);
+ name = dent->d_name;
+
+ switch(st.st_mode & S_IFMT) {
+ case S_IFCHR:
+ case S_IFBLK:
+ case S_IFIFO:
+ if ((retval = do_mknod_internal(parent_ino, name, &st))) {
+ com_err(func_name, retval,
+ _("while creating special file \"%s\""), name);
+ return retval;
+ }
+ break;
+ case S_IFSOCK:
+ /* FIXME: there is no make sockect function atm. */
+ com_err(func_name, 0,
+ _("ignoring sockect file\"%s\""), name);
+ break;
+ case S_IFLNK:
+ if((read_cnt = readlink(name, ln_target, sizeof(ln_target))) == -1) {
+ com_err(func_name, errno,
+ _("while trying to readlink \"%s\""), name);
+ return errno;
+ }
+ ln_target[read_cnt] = '\0';
+ if ((retval = do_symlink_internal(parent_ino, name, ln_target))) {
+ com_err(func_name, retval,
+ _("while writing symlink\"%s\""), name);
+ return retval;
+ }
+ break;
+ case S_IFREG:
+ if ((retval = do_write_internal(parent_ino, name, name))) {
+ com_err(func_name, retval,
+ _("while writing file \"%s\""), name);
+ return retval;
+ }
+ break;
+ case S_IFDIR:
+ if ((retval = do_mkdir_internal(parent_ino, name, &st))) {
+ com_err(func_name, retval,
+ _("while making dir \"%s\""), name);
+ return retval;
+ }
+ if ((retval = ext2fs_namei(current_fs, root, parent_ino, name, &ino))) {
+ com_err(name, retval, 0);
+ return retval;
+ }
+ /* Populate the dir recursively*/
+ retval = populate_fs(ino, name);
+ if (retval) {
+ com_err(func_name, retval, _("while adding dir \"%s\""), name);
+ return retval;
+ }
+ chdir("..");
+ break;
+ default:
+ com_err(func_name, 0,
+ _("ignoring entry \"%s\""), name);
+ }
+ }
+ closedir(dh);
+ return retval;
}
--
1.8.1.2


2013-08-28 05:30:39

by Robert Yang

[permalink] [raw]
Subject: [RFC 04/10] misc/util.c: create symlink

The do_symlink_internal() is used for creating symlinks, most of the
code are from debugfs/debugfs.c, the debugfs/debugfs.c will be modified
to use this function.

Signed-off-by: Robert Yang <[email protected]>
---
misc/util.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)

diff --git a/misc/util.c b/misc/util.c
index c793b42..f6ae7ca 100644
--- a/misc/util.c
+++ b/misc/util.c
@@ -392,6 +392,40 @@ errcode_t do_mknod_internal(ext2_ino_t cwd, const char *name, struct stat *st)
/* Make a symlink name -> target */
errcode_t do_symlink_internal(ext2_ino_t cwd, const char *name, char *target)
{
+ char *cp;
+ ext2_ino_t parent_ino;
+ errcode_t retval;
+ struct ext2_inode inode;
+ struct stat st;
+
+ cp = strrchr(name, '/');
+ if (cp) {
+ *cp = 0;
+ if ((retval = ext2fs_namei(current_fs, root, cwd, name, &parent_ino))){
+ com_err(name, retval, 0);
+ return retval;
+ }
+ name = cp+1;
+ } else {
+ parent_ino = cwd;
+ name = name;
+ }
+
+try_again:
+ retval = ext2fs_symlink(current_fs, parent_ino, 0, name, target);
+ if (retval == EXT2_ET_DIR_NO_SPACE) {
+ retval = ext2fs_expand_dir(current_fs, parent_ino);
+ if (retval) {
+ com_err("do_symlink_internal", retval, "while expanding directory");
+ return retval;
+ }
+ goto try_again;
+ }
+ if (retval) {
+ com_err("ext2fs_symlink", retval, 0);
+ return retval;
+ }
+
}

/* Make a directory in the fs */
--
1.8.1.2


2013-08-28 05:30:42

by Robert Yang

[permalink] [raw]
Subject: [RFC 05/10] misc/util.c: copy regular file

The do_write_internal() is used for copying file from native fs to
target, most of the code are from debugfs/debugfs.c, the
debugfs/debugfs.c will be modified to use this function.

NOTE: It needs further more work which is merging the sparse copy patch.

Signed-off-by: Robert Yang <[email protected]>
---
misc/util.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 125 insertions(+)

diff --git a/misc/util.c b/misc/util.c
index f6ae7ca..e35fd88 100644
--- a/misc/util.c
+++ b/misc/util.c
@@ -428,6 +428,46 @@ try_again:

}

+static errcode_t copy_file(int fd, ext2_ino_t ino)
+{
+ ext2_file_t e2_file;
+ errcode_t retval;
+ int got;
+ unsigned int written;
+ char buf[8192];
+ char *ptr;
+
+ retval = ext2fs_file_open(current_fs, ino, EXT2_FILE_WRITE, &e2_file);
+ if (retval)
+ return retval;
+
+ while (1) {
+ got = read(fd, buf, sizeof(buf));
+ if (got == 0)
+ break;
+ if (got < 0) {
+ retval = errno;
+ goto fail;
+ }
+ ptr = buf;
+ while (got > 0) {
+ retval = ext2fs_file_write(e2_file, ptr,
+ got, &written);
+ if (retval)
+ goto fail;
+
+ got -= written;
+ ptr += written;
+ }
+ }
+ retval = ext2fs_file_close(e2_file);
+ return retval;
+
+fail:
+ (void) ext2fs_file_close(e2_file);
+ return retval;
+}
+
/* Make a directory in the fs */
errcode_t do_mkdir_internal(ext2_ino_t cwd, const char *name, struct stat *st)
{
@@ -436,6 +476,91 @@ errcode_t do_mkdir_internal(ext2_ino_t cwd, const char *name, struct stat *st)
/* Copy the native file to the fs */
errcode_t do_write_internal(ext2_ino_t cwd, const char *src, const char *dest)
{
+ int fd;
+ struct stat statbuf;
+ ext2_ino_t ino;
+ errcode_t retval;
+ struct ext2_inode inode;
+ char *func_name = "do_write_internal";
+ int hdlink;
+
+ fd = open(src, O_RDONLY);
+ if (fd < 0) {
+ com_err(src, errno, 0);
+ return errno;
+ }
+ if (fstat(fd, &statbuf) < 0) {
+ com_err(src, errno, 0);
+ close(fd);
+ return errno;
+ }
+
+ retval = ext2fs_namei(current_fs, root, cwd, dest, &ino);
+ if (retval == 0) {
+ com_err(func_name, 0, "The file '%s' already exists\n", dest);
+ close(fd);
+ return errno;
+ }
+
+ retval = ext2fs_new_inode(current_fs, cwd, 010755, 0, &ino);
+ if (retval) {
+ com_err(func_name, retval, 0);
+ close(fd);
+ return errno;
+ }
+ printf("Allocated inode: %u\n", ino);
+ retval = ext2fs_link(current_fs, cwd, dest, ino, EXT2_FT_REG_FILE);
+ if (retval == EXT2_ET_DIR_NO_SPACE) {
+ retval = ext2fs_expand_dir(current_fs, cwd);
+ if (retval) {
+ com_err(func_name, retval, "while expanding directory");
+ close(fd);
+ return errno;
+ }
+ retval = ext2fs_link(current_fs, cwd, dest, ino, EXT2_FT_REG_FILE);
+ }
+ if (retval) {
+ com_err(dest, retval, 0);
+ close(fd);
+ return errno;
+ }
+ if (ext2fs_test_inode_bitmap2(current_fs->inode_map, ino))
+ com_err(func_name, 0, "Warning: inode already set");
+ ext2fs_inode_alloc_stats2(current_fs, ino, +1, 0);
+ memset(&inode, 0, sizeof(inode));
+ inode.i_mode = (statbuf.st_mode & ~LINUX_S_IFMT) | LINUX_S_IFREG;
+ inode.i_atime = inode.i_ctime = inode.i_mtime =
+ current_fs->now ? current_fs->now : time(0);
+ inode.i_links_count = 1;
+ inode.i_size = statbuf.st_size;
+ if (current_fs->super->s_feature_incompat &
+ EXT3_FEATURE_INCOMPAT_EXTENTS) {
+ int i;
+ struct ext3_extent_header *eh;
+
+ eh = (struct ext3_extent_header *) &inode.i_block[0];
+ eh->eh_depth = 0;
+ eh->eh_entries = 0;
+ eh->eh_magic = EXT3_EXT_MAGIC;
+ i = (sizeof(inode.i_block) - sizeof(*eh)) /
+ sizeof(struct ext3_extent);
+ eh->eh_max = ext2fs_cpu_to_le16(i);
+ inode.i_flags |= EXT4_EXTENTS_FL;
+ }
+
+ if ((retval = ext2fs_write_new_inode(current_fs, ino, &inode))) {
+ com_err(func_name, retval, "while creating inode %u", ino);
+ close(fd);
+ return errno;
+ }
+ if (LINUX_S_ISREG(inode.i_mode)) {
+ retval = copy_file(fd, ino);
+ if (retval)
+ com_err("copy_file", retval, 0);
+ }
+ close(fd);
+
+ return 0;
}

/* Copy files from source_dir to fs */
--
1.8.1.2


2013-08-28 05:30:34

by Robert Yang

[permalink] [raw]
Subject: [RFC 01/10] mke2fs.c: add an option: -d root-directory

This option is used for adding the files from the root-directory to the
filesystem, it is similiar to genext2fs, but genext2fs doesn't fully
support ext4.

This commit describes the skeleton of the implementation:
* The "struct hdlink_s" will be used for saving hard links, I
referred this from the genext2fs.

* The do_xxx_internal will be used by both mke2fs and debugfs, most of
their operations are similar.

Signed-off-by: Robert Yang <[email protected]>
---
misc/mke2fs.c | 39 ++++++++++++++++++++++++++++++++++-----
misc/util.c | 35 +++++++++++++++++++++++++++++++++++
misc/util.h | 32 ++++++++++++++++++++++++++++++++
3 files changed, 101 insertions(+), 5 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index d96f156..6401ae0 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -44,8 +44,6 @@ extern int optind;
#include <errno.h>
#endif
#include <sys/ioctl.h>
-#include <sys/types.h>
-#include <sys/stat.h>
#include <libgen.h>
#include <limits.h>
#include <blkid/blkid.h>
@@ -105,6 +103,7 @@ char *mount_dir;
char *journal_device;
int sync_kludge; /* Set using the MKE2FS_SYNC env. option */
char **fs_types;
+const char *root_dir; /* Copy files from the specified directory */

profile_t profile;

@@ -116,7 +115,8 @@ static void usage(void)
fprintf(stderr, _("Usage: %s [-c|-l filename] [-b block-size] "
"[-C cluster-size]\n\t[-i bytes-per-inode] [-I inode-size] "
"[-J journal-options]\n"
- "\t[-G flex-group-size] [-N number-of-inodes]\n"
+ "\t[-G flex-group-size] [-N number-of-inodes] "
+ "[-d root-directory]\n"
"\t[-m reserved-blocks-percentage] [-o creator-os]\n"
"\t[-g blocks-per-group] [-L volume-label] "
"[-M last-mounted-directory]\n\t[-O feature[,...]] "
@@ -1386,7 +1386,7 @@ profile_error:
}

while ((c = getopt (argc, argv,
- "b:cg:i:jl:m:no:qr:s:t:vC:DE:FG:I:J:KL:M:N:O:R:ST:U:V")) != EOF) {
+ "b:cg:i:jl:m:no:qr:s:t:d:vC:DE:FG:I:J:KL:M:N:O:R:ST:U:V")) != EOF) {
switch (c) {
case 'b':
blocksize = parse_num_blocks2(optarg, -1);
@@ -1572,6 +1572,9 @@ profile_error:
case 'U':
fs_uuid = optarg;
break;
+ case 'd':
+ root_dir = optarg;
+ break;
case 'v':
verbose = 1;
break;
@@ -2773,7 +2776,6 @@ no_journal:
"filesystem accounting information: "));
checkinterval = fs->super->s_checkinterval;
max_mnt_count = fs->super->s_max_mnt_count;
- retval = ext2fs_close(fs);
if (retval) {
fprintf(stderr,
_("\nWarning, had trouble writing out superblocks."));
@@ -2789,5 +2791,32 @@ no_journal:
for (i=0; fs_types[i]; i++)
free(fs_types[i]);
free(fs_types);
+
+ /* Copy files from the specified directory */
+ if (root_dir) {
+ if (!quiet)
+ printf(_("Copying files into the device...\n"));
+
+ /*
+ * Allocate memory for the hardlinks, we don't need free()
+ * since the lifespan will be over after the fs populated.
+ */
+ if ((hdlinks.hdl = (struct hdlink_s *)
+ malloc(hdlink_cnt * sizeof(struct hdlink_s))) == NULL) {
+ fprintf(stderr, _("\nNot enough memory"));
+ retval = ext2fs_close(fs);
+ return retval;
+ }
+
+ current_fs = fs;
+ root = EXT2_ROOT_INO;
+ retval = populate_fs(root, root_dir);
+ if (retval)
+ fprintf(stderr,
+ _("\nError while populating %s"), root_dir);
+ }
+
+ retval = ext2fs_close(fs);
+
return retval;
}
diff --git a/misc/util.c b/misc/util.c
index 6c93e1c..cbc7cc0 100644
--- a/misc/util.c
+++ b/misc/util.c
@@ -32,8 +32,18 @@
#include "ext2fs/ext2fs.h"
#include "nls-enable.h"
#include "blkid/blkid.h"
+
+#include <fcntl.h>
+
#include "util.h"

+int journal_size;
+int journal_flags;
+char *journal_device;
+
+/* For saving the hard links */
+int hdlink_cnt = HDLINK_CNT;
+
#ifndef HAVE_STRCASECMP
int strcasecmp (char *s1, char *s2)
{
@@ -303,3 +313,28 @@ void dump_mmp_msg(struct mmp_struct *mmp, const char *msg)
ctime(&t), mmp->mmp_nodename, mmp->mmp_bdevname);
}
}
+
+/* Make a special file which is block, character and fifo */
+errcode_t do_mknod_internal(ext2_ino_t cwd, const char *name, struct stat *st)
+{
+}
+
+/* Make a symlink name -> target */
+errcode_t do_symlink_internal(ext2_ino_t cwd, const char *name, char *target)
+{
+}
+
+/* Make a directory in the fs */
+errcode_t do_mkdir_internal(ext2_ino_t cwd, const char *name, struct stat *st)
+{
+}
+
+/* Copy the native file to the fs */
+errcode_t do_write_internal(ext2_ino_t cwd, const char *src, const char *dest)
+{
+}
+
+/* Copy files from source_dir to fs */
+errcode_t populate_fs(ext2_ino_t parent_ino, const char *source_dir)
+{
+}
diff --git a/misc/util.h b/misc/util.h
index f872c38..e71caf0 100644
--- a/misc/util.h
+++ b/misc/util.h
@@ -14,6 +14,31 @@ extern int journal_size;
extern int journal_flags;
extern char *journal_device;

+/* For struct stat */
+#include <sys/types.h>
+#include <sys/stat.h>
+
+struct hdlink_s
+{
+ ext2_ino_t src_ino;
+ ext2_ino_t dst_ino;
+};
+
+struct hdlinks_s
+{
+ int count;
+ struct hdlink_s *hdl;
+};
+
+struct hdlinks_s hdlinks;
+
+ext2_filsys current_fs;
+ext2_ino_t root;
+
+/* For saving the hard links */
+#define HDLINK_CNT 4
+extern int hdlink_cnt;
+
#ifndef HAVE_STRCASECMP
extern int strcasecmp (char *s1, char *s2);
#endif
@@ -25,3 +50,10 @@ extern void check_mount(const char *device, int force, const char *type);
extern unsigned int figure_journal_size(int size, ext2_filsys fs);
extern void print_check_message(int, unsigned int);
extern void dump_mmp_msg(struct mmp_struct *mmp, const char *msg);
+
+/* For populating the filesystem */
+extern errcode_t populate_fs(ext2_ino_t parent_ino, const char *source_dir);
+extern errcode_t do_mknod_internal(ext2_ino_t cwd, const char *name, struct stat *st);
+extern errcode_t do_symlink_internal(ext2_ino_t cwd, const char *name, char *target);
+extern errcode_t do_mkdir_internal(ext2_ino_t cwd, const char *name, struct stat *st);
+extern errcode_t do_write_internal(ext2_ino_t cwd, const char *src, const char *dest);
--
1.8.1.2


2013-08-28 05:30:37

by Robert Yang

[permalink] [raw]
Subject: [RFC 03/10] misc/util.c: create special file

The do_mknod_internal() is used for creating special file which is
block, character and fifo, most of the code are from debugfs/debugfs.c,
the debugfs/debugfs.c will be modified to use this function.

Signed-off-by: Robert Yang <[email protected]>
---
misc/util.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)

diff --git a/misc/util.c b/misc/util.c
index 6d87393..c793b42 100644
--- a/misc/util.c
+++ b/misc/util.c
@@ -317,6 +317,76 @@ void dump_mmp_msg(struct mmp_struct *mmp, const char *msg)
/* Make a special file which is block, character and fifo */
errcode_t do_mknod_internal(ext2_ino_t cwd, const char *name, struct stat *st)
{
+ ext2_ino_t ino;
+ errcode_t retval;
+ struct ext2_inode inode;
+ char *func_name = "do_mknod_internal";
+ unsigned long major, minor, mode;
+ int filetype;
+
+ switch(st->st_mode & S_IFMT) {
+ case S_IFCHR:
+ mode = LINUX_S_IFCHR;
+ filetype = EXT2_FT_CHRDEV;
+ break;
+ case S_IFBLK:
+ mode = LINUX_S_IFBLK;
+ filetype = EXT2_FT_BLKDEV;
+ break;
+ case S_IFIFO:
+ mode = LINUX_S_IFIFO;
+ filetype = EXT2_FT_FIFO;
+ break;
+ }
+
+ if (!(current_fs->flags & EXT2_FLAG_RW)) {
+ com_err(func_name, 0, "Filesystem opened read/only");
+ return -1;
+ }
+ retval = ext2fs_new_inode(current_fs, cwd, 010755, 0, &ino);
+ if (retval) {
+ com_err(func_name, retval, 0);
+ return retval;
+ }
+ printf("Allocated inode: %u\n", ino);
+ retval = ext2fs_link(current_fs, cwd, name, ino, filetype);
+ if (retval == EXT2_ET_DIR_NO_SPACE) {
+ retval = ext2fs_expand_dir(current_fs, cwd);
+ if (retval) {
+ com_err(func_name, retval, "while expanding directory");
+ return retval;
+ }
+ retval = ext2fs_link(current_fs, cwd, name, ino, filetype);
+ }
+ if (retval) {
+ com_err(name, retval, 0);
+ return -1;
+ }
+ if (ext2fs_test_inode_bitmap2(current_fs->inode_map, ino))
+ com_err(func_name, 0, "Warning: inode already set");
+ ext2fs_inode_alloc_stats2(current_fs, ino, +1, 0);
+ memset(&inode, 0, sizeof(inode));
+ inode.i_mode = mode;
+ inode.i_atime = inode.i_ctime = inode.i_mtime =
+ current_fs->now ? current_fs->now : time(0);
+
+ major = major(st->st_rdev);
+ minor = minor(st->st_rdev);
+
+ if ((major < 256) && (minor < 256)) {
+ inode.i_block[0] = major * 256 + minor;
+ inode.i_block[1] = 0;
+ } else {
+ inode.i_block[0] = 0;
+ inode.i_block[1] = (minor & 0xff) | (major << 8) | ((minor & ~0xff) << 12);
+ }
+ inode.i_links_count = 1;
+
+ retval = ext2fs_write_new_inode(current_fs, ino, &inode);
+ if (retval)
+ com_err(func_name, retval, "while creating inode %u", ino);
+
+ return retval;
}

/* Make a symlink name -> target */
--
1.8.1.2


2013-08-28 05:30:43

by Robert Yang

[permalink] [raw]
Subject: [RFC 06/10] misc/util.c: create directory

The do_mkdir_internal() is used for making dir on the target fs, most of
the code are from debugfs/debugfs.c, the debugfs/debugfs.c will be
modified to use this function.

Signed-off-by: Robert Yang <[email protected]>
---
misc/util.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)

diff --git a/misc/util.c b/misc/util.c
index e35fd88..7a45120 100644
--- a/misc/util.c
+++ b/misc/util.c
@@ -471,6 +471,40 @@ fail:
/* Make a directory in the fs */
errcode_t do_mkdir_internal(ext2_ino_t cwd, const char *name, struct stat *st)
{
+ char *cp;
+ ext2_ino_t parent_ino, ino;
+ errcode_t retval;
+ struct ext2_inode inode;
+ char *func_name = "do_mkdir_internal";
+
+
+ cp = strrchr(name, '/');
+ if (cp) {
+ *cp = 0;
+ if ((retval = ext2fs_namei(current_fs, root, cwd, name, &parent_ino))){
+ com_err(name, retval, 0);
+ return retval;
+ }
+ name = cp+1;
+ } else {
+ parent_ino = cwd;
+ name = name;
+ }
+
+try_again:
+ retval = ext2fs_mkdir(current_fs, parent_ino, 0, name);
+ if (retval == EXT2_ET_DIR_NO_SPACE) {
+ retval = ext2fs_expand_dir(current_fs, parent_ino);
+ if (retval) {
+ com_err(func_name, retval, "while expanding directory");
+ return retval;
+ }
+ goto try_again;
+ }
+ if (retval) {
+ com_err("ext2fs_mkdir", retval, 0);
+ return retval;
+ }
}

/* Copy the native file to the fs */
--
1.8.1.2


2013-08-28 05:30:51

by Robert Yang

[permalink] [raw]
Subject: [RFC 10/10] debugfs: use the functions in misc/util.c

Use the functions in mist/util.c to reduce the duplicated code.

Signed-off-by: Robert Yang <[email protected]>
---
debugfs/Makefile.in | 7 +-
debugfs/debugfs.c | 251 ++++------------------------------------------------
debugfs/debugfs.h | 1 +
3 files changed, 22 insertions(+), 237 deletions(-)

diff --git a/debugfs/Makefile.in b/debugfs/Makefile.in
index 9a86dc6..4af2483 100644
--- a/debugfs/Makefile.in
+++ b/debugfs/Makefile.in
@@ -18,7 +18,7 @@ MK_CMDS= _SS_DIR_OVERRIDE=../lib/ss ../lib/ss/mk_cmds

DEBUG_OBJS= debug_cmds.o debugfs.o util.o ncheck.o icheck.o ls.o \
lsdel.o dump.o set_fields.o logdump.o htree.o unused.o e2freefrag.o \
- filefrag.o extent_cmds.o extent_inode.o zap.o
+ filefrag.o extent_cmds.o extent_inode.o zap.o $(srcdir)/../misc/util.o

RO_DEBUG_OBJS= ro_debug_cmds.o ro_debugfs.o util.o ncheck.o icheck.o ls.o \
lsdel.o logdump.o htree.o e2freefrag.o filefrag.o extent_cmds.o \
@@ -28,7 +28,8 @@ SRCS= debug_cmds.c $(srcdir)/debugfs.c $(srcdir)/util.c $(srcdir)/ls.c \
$(srcdir)/ncheck.c $(srcdir)/icheck.c $(srcdir)/lsdel.c \
$(srcdir)/dump.c $(srcdir)/set_fields.c ${srcdir}/logdump.c \
$(srcdir)/htree.c $(srcdir)/unused.c ${srcdir}/../misc/e2freefrag.c \
- $(srcdir)/filefrag.c $(srcdir)/extent_inode.c $(srcdir)/zap.c
+ $(srcdir)/filefrag.c $(srcdir)/extent_inode.c $(srcdir)/zap.c \
+ $(srcdir)/../misc/util.c

LIBS= $(LIBEXT2FS) $(LIBE2P) $(LIBSS) $(LIBCOM_ERR) $(LIBBLKID) \
$(LIBUUID)
@@ -141,7 +142,7 @@ debugfs.o: $(srcdir)/debugfs.c $(top_srcdir)/lib/et/com_err.h \
$(top_srcdir)/lib/ext2fs/ext2_ext_attr.h $(top_srcdir)/lib/ext2fs/bitops.h \
$(top_srcdir)/lib/e2p/e2p.h $(top_srcdir)/version.h $(srcdir)/jfs_user.h \
$(top_srcdir)/lib/ext2fs/kernel-jbd.h $(top_srcdir)/lib/ext2fs/jfs_compat.h \
- $(top_srcdir)/lib/ext2fs/kernel-list.h
+ $(top_srcdir)/lib/ext2fs/kernel-list.h $(top_srcdir)/misc/util.h
util.o: $(srcdir)/util.c $(srcdir)/debugfs.h \
$(top_srcdir)/lib/ext2fs/ext2_fs.h $(top_builddir)/lib/ext2fs/ext2_types.h \
$(top_srcdir)/lib/ext2fs/ext2fs.h $(top_srcdir)/lib/ext2fs/ext3_extents.h \
diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index 2660218..32daed7 100644
--- a/debugfs/debugfs.c
+++ b/debugfs/debugfs.c
@@ -25,8 +25,6 @@ extern char *optarg;
#include <errno.h>
#endif
#include <fcntl.h>
-#include <sys/types.h>
-#include <sys/stat.h>

#include "debugfs.h"
#include "uuid/uuid.h"
@@ -41,8 +39,7 @@ ss_request_table *extra_cmds;
const char *debug_prog_name;
int sci_idx;

-ext2_filsys current_fs = NULL;
-ext2_ino_t root, cwd;
+ext2_ino_t cwd;

static void open_filesystem(char *device, int open_flags, blk64_t superblock,
blk64_t blocksize, int catastrophic,
@@ -1571,144 +1568,25 @@ void do_find_free_inode(int argc, char *argv[])
}

#ifndef READ_ONLY
-static errcode_t copy_file(int fd, ext2_ino_t newfile)
-{
- ext2_file_t e2_file;
- errcode_t retval;
- int got;
- unsigned int written;
- char buf[8192];
- char *ptr;
-
- retval = ext2fs_file_open(current_fs, newfile,
- EXT2_FILE_WRITE, &e2_file);
- if (retval)
- return retval;
-
- while (1) {
- got = read(fd, buf, sizeof(buf));
- if (got == 0)
- break;
- if (got < 0) {
- retval = errno;
- goto fail;
- }
- ptr = buf;
- while (got > 0) {
- retval = ext2fs_file_write(e2_file, ptr,
- got, &written);
- if (retval)
- goto fail;
-
- got -= written;
- ptr += written;
- }
- }
- retval = ext2fs_file_close(e2_file);
- return retval;
-
-fail:
- (void) ext2fs_file_close(e2_file);
- return retval;
-}
-
-
void do_write(int argc, char *argv[])
{
- int fd;
- struct stat statbuf;
- ext2_ino_t newfile;
errcode_t retval;
- struct ext2_inode inode;

if (common_args_process(argc, argv, 3, 3, "write",
"<native file> <new file>", CHECK_FS_RW))
return;

- fd = open(argv[1], O_RDONLY);
- if (fd < 0) {
- com_err(argv[1], errno, 0);
- return;
- }
- if (fstat(fd, &statbuf) < 0) {
- com_err(argv[1], errno, 0);
- close(fd);
- return;
- }
-
- retval = ext2fs_namei(current_fs, root, cwd, argv[2], &newfile);
- if (retval == 0) {
- com_err(argv[0], 0, "The file '%s' already exists\n", argv[2]);
- close(fd);
- return;
- }
-
- retval = ext2fs_new_inode(current_fs, cwd, 010755, 0, &newfile);
- if (retval) {
+ if ((retval = do_write_internal(cwd, argv[1], argv[2])))
com_err(argv[0], retval, 0);
- close(fd);
- return;
- }
- printf("Allocated inode: %u\n", newfile);
- retval = ext2fs_link(current_fs, cwd, argv[2], newfile,
- EXT2_FT_REG_FILE);
- if (retval == EXT2_ET_DIR_NO_SPACE) {
- retval = ext2fs_expand_dir(current_fs, cwd);
- if (retval) {
- com_err(argv[0], retval, "while expanding directory");
- close(fd);
- return;
- }
- retval = ext2fs_link(current_fs, cwd, argv[2], newfile,
- EXT2_FT_REG_FILE);
- }
- if (retval) {
- com_err(argv[2], retval, 0);
- close(fd);
- return;
- }
- if (ext2fs_test_inode_bitmap2(current_fs->inode_map,newfile))
- com_err(argv[0], 0, "Warning: inode already set");
- ext2fs_inode_alloc_stats2(current_fs, newfile, +1, 0);
- memset(&inode, 0, sizeof(inode));
- inode.i_mode = (statbuf.st_mode & ~LINUX_S_IFMT) | LINUX_S_IFREG;
- inode.i_atime = inode.i_ctime = inode.i_mtime =
- current_fs->now ? current_fs->now : time(0);
- inode.i_links_count = 1;
- inode.i_size = statbuf.st_size;
- if (current_fs->super->s_feature_incompat &
- EXT3_FEATURE_INCOMPAT_EXTENTS) {
- int i;
- struct ext3_extent_header *eh;
-
- eh = (struct ext3_extent_header *) &inode.i_block[0];
- eh->eh_depth = 0;
- eh->eh_entries = 0;
- eh->eh_magic = EXT3_EXT_MAGIC;
- i = (sizeof(inode.i_block) - sizeof(*eh)) /
- sizeof(struct ext3_extent);
- eh->eh_max = ext2fs_cpu_to_le16(i);
- inode.i_flags |= EXT4_EXTENTS_FL;
- }
- if (debugfs_write_new_inode(newfile, &inode, argv[0])) {
- close(fd);
- return;
- }
- if (LINUX_S_ISREG(inode.i_mode)) {
- retval = copy_file(fd, newfile);
- if (retval)
- com_err("copy_file", retval, 0);
- }
- close(fd);
+
}

void do_mknod(int argc, char *argv[])
{
unsigned long mode, major, minor;
- ext2_ino_t newfile;
errcode_t retval;
- struct ext2_inode inode;
int filetype, nr;
+ struct stat st;

if (check_fs_open(argv[0]))
return;
@@ -1717,115 +1595,50 @@ void do_mknod(int argc, char *argv[])
com_err(argv[0], 0, "Usage: mknod <name> [p| [c|b] <major> <minor>]");
return;
}
+
mode = minor = major = 0;
switch (argv[2][0]) {
case 'p':
- mode = LINUX_S_IFIFO;
- filetype = EXT2_FT_FIFO;
+ st.st_mode = S_IFIFO;
nr = 3;
break;
case 'c':
- mode = LINUX_S_IFCHR;
- filetype = EXT2_FT_CHRDEV;
+ st.st_mode = S_IFCHR;
nr = 5;
break;
case 'b':
- mode = LINUX_S_IFBLK;
- filetype = EXT2_FT_BLKDEV;
+ st.st_mode = S_IFBLK;
nr = 5;
break;
default:
- filetype = 0;
nr = 0;
}
+
if (nr == 5) {
major = strtoul(argv[3], argv+3, 0);
minor = strtoul(argv[4], argv+4, 0);
if (major > 65535 || minor > 65535 || argv[3][0] || argv[4][0])
nr = 0;
}
+
if (argc != nr)
goto usage;
- if (check_fs_read_write(argv[0]))
- return;
- retval = ext2fs_new_inode(current_fs, cwd, 010755, 0, &newfile);
- if (retval) {
+
+ st.st_rdev = makedev(major, minor);
+ if ((retval = do_mknod_internal(cwd, argv[1], &st)))
com_err(argv[0], retval, 0);
- return;
- }
- printf("Allocated inode: %u\n", newfile);
- retval = ext2fs_link(current_fs, cwd, argv[1], newfile, filetype);
- if (retval == EXT2_ET_DIR_NO_SPACE) {
- retval = ext2fs_expand_dir(current_fs, cwd);
- if (retval) {
- com_err(argv[0], retval, "while expanding directory");
- return;
- }
- retval = ext2fs_link(current_fs, cwd, argv[1], newfile,
- filetype);
- }
- if (retval) {
- com_err(argv[1], retval, 0);
- return;
- }
- if (ext2fs_test_inode_bitmap2(current_fs->inode_map,newfile))
- com_err(argv[0], 0, "Warning: inode already set");
- ext2fs_inode_alloc_stats2(current_fs, newfile, +1, 0);
- memset(&inode, 0, sizeof(inode));
- inode.i_mode = mode;
- inode.i_atime = inode.i_ctime = inode.i_mtime =
- current_fs->now ? current_fs->now : time(0);
- if ((major < 256) && (minor < 256)) {
- inode.i_block[0] = major*256+minor;
- inode.i_block[1] = 0;
- } else {
- inode.i_block[0] = 0;
- inode.i_block[1] = (minor & 0xff) | (major << 8) | ((minor & ~0xff) << 12);
- }
- inode.i_links_count = 1;
- if (debugfs_write_new_inode(newfile, &inode, argv[0]))
- return;
}

void do_mkdir(int argc, char *argv[])
{
- char *cp;
- ext2_ino_t parent;
- char *name;
errcode_t retval;

if (common_args_process(argc, argv, 2, 2, "mkdir",
"<filename>", CHECK_FS_RW))
return;

- cp = strrchr(argv[1], '/');
- if (cp) {
- *cp = 0;
- parent = string_to_inode(argv[1]);
- if (!parent) {
- com_err(argv[1], ENOENT, 0);
- return;
- }
- name = cp+1;
- } else {
- parent = cwd;
- name = argv[1];
- }
-
-try_again:
- retval = ext2fs_mkdir(current_fs, parent, 0, name);
- if (retval == EXT2_ET_DIR_NO_SPACE) {
- retval = ext2fs_expand_dir(current_fs, parent);
- if (retval) {
- com_err(argv[0], retval, "while expanding directory");
- return;
- }
- goto try_again;
- }
- if (retval) {
- com_err("ext2fs_mkdir", retval, 0);
- return;
- }
+ if ((retval = do_mkdir_internal(cwd, argv[1], NULL)))
+ com_err(argv[0], retval, 0);

}

@@ -2214,44 +2027,14 @@ void do_punch(int argc, char *argv[])

void do_symlink(int argc, char *argv[])
{
- char *cp;
- ext2_ino_t parent;
- char *name, *target;
errcode_t retval;

if (common_args_process(argc, argv, 3, 3, "symlink",
"<filename> <target>", CHECK_FS_RW))
return;

- cp = strrchr(argv[1], '/');
- if (cp) {
- *cp = 0;
- parent = string_to_inode(argv[1]);
- if (!parent) {
- com_err(argv[1], ENOENT, 0);
- return;
- }
- name = cp+1;
- } else {
- parent = cwd;
- name = argv[1];
- }
- target = argv[2];
-
-try_again:
- retval = ext2fs_symlink(current_fs, parent, 0, name, target);
- if (retval == EXT2_ET_DIR_NO_SPACE) {
- retval = ext2fs_expand_dir(current_fs, parent);
- if (retval) {
- com_err(argv[0], retval, "while expanding directory");
- return;
- }
- goto try_again;
- }
- if (retval) {
- com_err("ext2fs_symlink", retval, 0);
- return;
- }
+ if ((retval = do_symlink_internal(cwd, argv[1], argv[2])))
+ com_err(argv[0], retval, 0);

}

diff --git a/debugfs/debugfs.h b/debugfs/debugfs.h
index 45175cf..64ed608 100644
--- a/debugfs/debugfs.h
+++ b/debugfs/debugfs.h
@@ -5,6 +5,7 @@
#include "ss/ss.h"
#include "ext2fs/ext2_fs.h"
#include "ext2fs/ext2fs.h"
+#include "../misc/util.h"

#ifdef __STDC__
#define NOARGS void
--
1.8.1.2


2013-08-28 05:30:49

by Robert Yang

[permalink] [raw]
Subject: [RFC 09/10] mke2fs.8.in: update the manual for the -d option

update the manual for the -d option

Signed-off-by: Robert Yang <[email protected]>
---
misc/mke2fs.8.in | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/misc/mke2fs.8.in b/misc/mke2fs.8.in
index fe2dcdb..b268154 100644
--- a/misc/mke2fs.8.in
+++ b/misc/mke2fs.8.in
@@ -52,6 +52,10 @@ mke2fs \- create an ext2/ext3/ext4 filesystem
.I number-of-inodes
]
[
+.B \-d
+.I root-directory
+]
+[
.B \-n
]
[
@@ -490,6 +494,9 @@ the
ratio). This allows the user to specify the number
of desired inodes directly.
.TP
+.BI \-d " root-directory"
+Add the files from the root-directory to the filesystem.
+.TP
.BI \-o " creator-os"
Overrides the default value of the "creator operating system" field of the
filesystem. The creator field is set by default to the name of the OS the
--
1.8.1.2


2013-08-28 05:30:52

by Robert Yang

[permalink] [raw]
Subject: [RFC 08/10] misc/util.c: handle hardlinks

Create the file and save the source inode number when we meet the hard
link at the first time, use ext2fs_link() to link the name to the target
inode number when we meet the same source inode number again.

Signed-off-by: Robert Yang <[email protected]>
---
misc/util.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 85 insertions(+)

diff --git a/misc/util.c b/misc/util.c
index 1a0a58c..11134db 100644
--- a/misc/util.c
+++ b/misc/util.c
@@ -314,6 +314,42 @@ void dump_mmp_msg(struct mmp_struct *mmp, const char *msg)
}
}

+/* Link an inode number to a directory */
+static errcode_t add_link(ext2_ino_t parent_ino, ext2_ino_t ino, const char *name)
+{
+ struct ext2_inode inode;
+ errcode_t retval;
+ char *func_name = "add_to_dir";
+
+ retval = ext2fs_read_inode(current_fs, ino, &inode);
+ if (retval) {
+ com_err(func_name, retval, "while reading inode %u", ino);
+ return retval;
+ }
+
+ retval = ext2fs_link(current_fs, parent_ino, name, ino, inode.i_flags);
+ if (retval == EXT2_ET_DIR_NO_SPACE) {
+ retval = ext2fs_expand_dir(current_fs, parent_ino);
+ if (retval) {
+ com_err(func_name, retval, "while expanding directory");
+ return retval;
+ }
+ retval = ext2fs_link(current_fs, parent_ino, name, ino, inode.i_flags);
+ }
+ if (retval) {
+ com_err(func_name, retval, "while linking %s", name);
+ return retval;
+ }
+
+ inode.i_links_count++;
+
+ retval = ext2fs_write_inode(current_fs, ino, &inode);
+ if (retval)
+ com_err(func_name, retval, "while writing inode %u", ino);
+
+ return retval;
+}
+
/* Fill the uid, gid, mode and time for the inode */
static void fill_inode(struct ext2_inode *inode, struct stat *st)
{
@@ -542,6 +578,17 @@ try_again:
}
}

+int is_hardlink(ext2_ino_t ino)
+{
+ int i;
+
+ for(i = 0; i < hdlinks.count; i++) {
+ if(hdlinks.hdl[i].src_ino == ino)
+ return i;
+ }
+ return -1;
+}
+
/* Copy the native file to the fs */
errcode_t do_write_internal(ext2_ino_t cwd, const char *src, const char *dest)
{
@@ -640,10 +687,12 @@ errcode_t populate_fs(ext2_ino_t parent_ino, const char *source_dir)
struct dirent *dent;
struct stat st;
char ln_target[PATH_MAX];
+ unsigned int save_inode = 0;
char *func_name = "populate_fs";
ext2_ino_t ino;
errcode_t retval;
int read_cnt;
+ int hdlink;

root = EXT2_ROOT_INO;

@@ -665,6 +714,21 @@ errcode_t populate_fs(ext2_ino_t parent_ino, const char *source_dir)
lstat(dent->d_name, &st);
name = dent->d_name;

+ /* Check for hardlinks */
+ if (!S_ISDIR(st.st_mode) && !S_ISLNK(st.st_mode) && st.st_nlink > 1) {
+ hdlink = is_hardlink(st.st_ino);
+ if (hdlink >= 0) {
+ retval = add_link(parent_ino,
+ hdlinks.hdl[hdlink].dst_ino, name);
+ if (retval) {
+ com_err(func_name, retval, "while linking %s", name);
+ return retval;
+ }
+ continue;
+ } else
+ save_inode = 1;
+ }
+
switch(st.st_mode & S_IFMT) {
case S_IFCHR:
case S_IFBLK:
@@ -733,6 +797,27 @@ errcode_t populate_fs(ext2_ino_t parent_ino, const char *source_dir)
_("while setting inode for \"%s\""), name);
return retval;
}
+
+ /* Save the hardlink ino */
+ if (save_inode) {
+ /*
+ * Check whether need more memory, and we don't need
+ * free() since the lifespan will be over after the fs
+ * populated.
+ */
+ if (hdlinks.count == hdlink_cnt) {
+ if ((hdlinks.hdl = realloc (hdlinks.hdl,
+ (hdlink_cnt + HDLINK_CNT) *
+ sizeof (struct hdlink_s))) == NULL) {
+ com_err(name, errno, "Not enough memory");
+ return errno;
+ }
+ hdlink_cnt += HDLINK_CNT;
+ }
+ hdlinks.hdl[hdlinks.count].src_ino = st.st_ino;
+ hdlinks.hdl[hdlinks.count].dst_ino = ino;
+ hdlinks.count++;
+ }
}
closedir(dh);
return retval;
--
1.8.1.2


2013-09-01 03:25:57

by Zheng Liu

[permalink] [raw]
Subject: Re: [RFC 00/10] e2fsprogs/mke2fs: add an option: -d root-directory

Hi Robert,

On Wed, Aug 28, 2013 at 01:25:50PM +0800, Robert Yang wrote:
> This option is used for adding the files from the root-directory to the
> filesystem, it is similiar to genext2fs, but genext2fs doesn't fully
> support ext4.
>
> * Questions
> - Is such an option acceptable ?
>
> - Most of the code have been in debugfs/debugfs.c already, I moved them to
> misc/util.c and modified them to let both mke2fs and debugfs can use them,
> maybe we should put these code in another separate file ?
>
> - Where can I get the up-to-date development git repo, please? I think that
> there would be conflicts with the dev git repo, I'd like to rebase it if I
> can get the repo, currently, I'm using this one:
>
> http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git
>
> * The size impact on misc/mke2fs:
> 1,677,297 -> 1,728,110 (non stripped, about 50K increases)
> 316,968 -> 325,160 (stripped, 8K increases)
>
> Please feel free to give your comments.

If I understand correctly, after applied this patch set, we can copy
some files from a directory that is indicated by '-d' option when we
create a new ext4 file system. My concern is why we need to add this
option? Without this option I can use 'mkfs.ext4 ${DEV}', 'mount ${DEV}
${MNT}' and 'cp ${SRC} ${MNT}' to do the same thing. I am not
convinced. Could you please describe more details for this patch set?

Regards,
- Zheng

>
> // Robert
>
> Robert Yang (10):
> mke2fs.c: add an option: -d root-directory
> misc/util.c: implement populate_fs()
> misc/util.c: create special file
> misc/util.c: create symlink
> misc/util.c: copy regular file
> misc/util.c: create directory
> misc/util.c: set more information for inode
> misc/util.c: handle hardlinks
> mke2fs.8.in: update the manual for the -d option
> debugfs: use the functions in misc/util.c
>
> debugfs/Makefile.in | 7 +-
> debugfs/debugfs.c | 251 ++-----------------------
> debugfs/debugfs.h | 1 +
> misc/mke2fs.8.in | 7 +
> misc/mke2fs.c | 39 +++-
> misc/util.c | 519 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> misc/util.h | 32 ++++
> 7 files changed, 614 insertions(+), 242 deletions(-)
>
> --
> 1.8.1.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-09-02 06:47:12

by Robert Yang

[permalink] [raw]
Subject: Re: [RFC 00/10] e2fsprogs/mke2fs: add an option: -d root-directory



On 09/01/2013 11:26 AM, Zheng Liu wrote:
> Hi Robert,
>
> On Wed, Aug 28, 2013 at 01:25:50PM +0800, Robert Yang wrote:
>> This option is used for adding the files from the root-directory to the
>> filesystem, it is similiar to genext2fs, but genext2fs doesn't fully
>> support ext4.
>>
>> * Questions
>> - Is such an option acceptable ?
>>
>> - Most of the code have been in debugfs/debugfs.c already, I moved them to
>> misc/util.c and modified them to let both mke2fs and debugfs can use them,
>> maybe we should put these code in another separate file ?
>>
>> - Where can I get the up-to-date development git repo, please? I think that
>> there would be conflicts with the dev git repo, I'd like to rebase it if I
>> can get the repo, currently, I'm using this one:
>>
>> http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git
>>
>> * The size impact on misc/mke2fs:
>> 1,677,297 -> 1,728,110 (non stripped, about 50K increases)
>> 316,968 -> 325,160 (stripped, 8K increases)
>>
>> Please feel free to give your comments.
>
> If I understand correctly, after applied this patch set, we can copy
> some files from a directory that is indicated by '-d' option when we
> create a new ext4 file system. My concern is why we need to add this
> option? Without this option I can use 'mkfs.ext4 ${DEV}', 'mount ${DEV}
> ${MNT}' and 'cp ${SRC} ${MNT}' to do the same thing. I am not
> convinced. Could you please describe more details for this patch set?
>

Hi Zheng,

Yes, you are right, we can run mkfs/mount/cp to do the same thing,
but the problem is that the "mount" command requires the root privilege,
but we may not have it when we are working on a sever (the mkfs doesn't
need the root privilege), for example

$ dd if=/dev/zero of=test.img count=0 bs=1k seek=2M
$ mke2fs -t ext4 -F test.img -d <root-dir>

Will create an ext4 image with the files copied without the root privilege.

// Robert

> Regards,
> - Zheng
>
>>
>> // Robert
>>
>> Robert Yang (10):
>> mke2fs.c: add an option: -d root-directory
>> misc/util.c: implement populate_fs()
>> misc/util.c: create special file
>> misc/util.c: create symlink
>> misc/util.c: copy regular file
>> misc/util.c: create directory
>> misc/util.c: set more information for inode
>> misc/util.c: handle hardlinks
>> mke2fs.8.in: update the manual for the -d option
>> debugfs: use the functions in misc/util.c
>>
>> debugfs/Makefile.in | 7 +-
>> debugfs/debugfs.c | 251 ++-----------------------
>> debugfs/debugfs.h | 1 +
>> misc/mke2fs.8.in | 7 +
>> misc/mke2fs.c | 39 +++-
>> misc/util.c | 519 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> misc/util.h | 32 ++++
>> 7 files changed, 614 insertions(+), 242 deletions(-)
>>
>> --
>> 1.8.1.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>

2013-09-02 11:54:12

by Zheng Liu

[permalink] [raw]
Subject: Re: [RFC 00/10] e2fsprogs/mke2fs: add an option: -d root-directory

On Mon, Sep 02, 2013 at 02:46:33PM +0800, Robert Yang wrote:
>
>
> On 09/01/2013 11:26 AM, Zheng Liu wrote:
> >Hi Robert,
> >
> >On Wed, Aug 28, 2013 at 01:25:50PM +0800, Robert Yang wrote:
> >>This option is used for adding the files from the root-directory to the
> >>filesystem, it is similiar to genext2fs, but genext2fs doesn't fully
> >>support ext4.
> >>
> >>* Questions
> >> - Is such an option acceptable ?
> >>
> >> - Most of the code have been in debugfs/debugfs.c already, I moved them to
> >> misc/util.c and modified them to let both mke2fs and debugfs can use them,
> >> maybe we should put these code in another separate file ?
> >>
> >> - Where can I get the up-to-date development git repo, please? I think that
> >> there would be conflicts with the dev git repo, I'd like to rebase it if I
> >> can get the repo, currently, I'm using this one:
> >>
> >> http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git
> >>
> >>* The size impact on misc/mke2fs:
> >> 1,677,297 -> 1,728,110 (non stripped, about 50K increases)
> >> 316,968 -> 325,160 (stripped, 8K increases)
> >>
> >>Please feel free to give your comments.
> >
> >If I understand correctly, after applied this patch set, we can copy
> >some files from a directory that is indicated by '-d' option when we
> >create a new ext4 file system. My concern is why we need to add this
> >option? Without this option I can use 'mkfs.ext4 ${DEV}', 'mount ${DEV}
> >${MNT}' and 'cp ${SRC} ${MNT}' to do the same thing. I am not
> >convinced. Could you please describe more details for this patch set?
> >
>
> Hi Zheng,
>
> Yes, you are right, we can run mkfs/mount/cp to do the same thing,
> but the problem is that the "mount" command requires the root privilege,
> but we may not have it when we are working on a sever (the mkfs doesn't
> need the root privilege), for example
>
> $ dd if=/dev/zero of=test.img count=0 bs=1k seek=2M
> $ mke2fs -t ext4 -F test.img -d <root-dir>
>
> Will create an ext4 image with the files copied without the root privilege.

Interesting. As you said above, if you haven't the root privilege, you
won't use this file system. But at least this file system will be used
by some one who have the root privilege. He/she can do the rest of
works. That means that you do 'dd/mke2fs' and he or she does
'mount/cp'. I don't see any reason that all dirs and files must be
created by mke2fs. Any real case that needs to do this?

- Zheng

2013-09-02 12:20:40

by Robert Yang

[permalink] [raw]
Subject: Re: [RFC 00/10] e2fsprogs/mke2fs: add an option: -d root-directory



On 09/02/2013 07:55 PM, Zheng Liu wrote:
> On Mon, Sep 02, 2013 at 02:46:33PM +0800, Robert Yang wrote:
>>
>>
>> On 09/01/2013 11:26 AM, Zheng Liu wrote:
>>> Hi Robert,
>>>
>>> On Wed, Aug 28, 2013 at 01:25:50PM +0800, Robert Yang wrote:
>>>> This option is used for adding the files from the root-directory to the
>>>> filesystem, it is similiar to genext2fs, but genext2fs doesn't fully
>>>> support ext4.
>>>>
>>>> * Questions
>>>> - Is such an option acceptable ?
>>>>
>>>> - Most of the code have been in debugfs/debugfs.c already, I moved them to
>>>> misc/util.c and modified them to let both mke2fs and debugfs can use them,
>>>> maybe we should put these code in another separate file ?
>>>>
>>>> - Where can I get the up-to-date development git repo, please? I think that
>>>> there would be conflicts with the dev git repo, I'd like to rebase it if I
>>>> can get the repo, currently, I'm using this one:
>>>>
>>>> http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git
>>>>
>>>> * The size impact on misc/mke2fs:
>>>> 1,677,297 -> 1,728,110 (non stripped, about 50K increases)
>>>> 316,968 -> 325,160 (stripped, 8K increases)
>>>>
>>>> Please feel free to give your comments.
>>>
>>> If I understand correctly, after applied this patch set, we can copy
>>> some files from a directory that is indicated by '-d' option when we
>>> create a new ext4 file system. My concern is why we need to add this
>>> option? Without this option I can use 'mkfs.ext4 ${DEV}', 'mount ${DEV}
>>> ${MNT}' and 'cp ${SRC} ${MNT}' to do the same thing. I am not
>>> convinced. Could you please describe more details for this patch set?
>>>
>>
>> Hi Zheng,
>>
>> Yes, you are right, we can run mkfs/mount/cp to do the same thing,
>> but the problem is that the "mount" command requires the root privilege,
>> but we may not have it when we are working on a sever (the mkfs doesn't
>> need the root privilege), for example
>>
>> $ dd if=/dev/zero of=test.img count=0 bs=1k seek=2M
>> $ mke2fs -t ext4 -F test.img -d <root-dir>
>>
>> Will create an ext4 image with the files copied without the root privilege.
>
> Interesting. As you said above, if you haven't the root privilege, you
> won't use this file system. But at least this file system will be used
> by some one who have the root privilege. He/she can do the rest of
> works. That means that you do 'dd/mke2fs' and he or she does
> 'mount/cp'. I don't see any reason that all dirs and files must be
> created by mke2fs. Any real case that needs to do this?
>

Hi Zheng,

For example, in Linux embedded development, we usually create a filesystem
image which contains the linux embedded OS, this image can be used by qemu,
Live CD and so on.

// Robert

> - Zheng
>
>

2013-09-02 12:26:18

by Zheng Liu

[permalink] [raw]
Subject: Re: [RFC 00/10] e2fsprogs/mke2fs: add an option: -d root-directory

On Mon, Sep 02, 2013 at 08:20:02PM +0800, Robert Yang wrote:
>
>
> On 09/02/2013 07:55 PM, Zheng Liu wrote:
> >On Mon, Sep 02, 2013 at 02:46:33PM +0800, Robert Yang wrote:
> >>
> >>
> >>On 09/01/2013 11:26 AM, Zheng Liu wrote:
> >>>Hi Robert,
> >>>
> >>>On Wed, Aug 28, 2013 at 01:25:50PM +0800, Robert Yang wrote:
> >>>>This option is used for adding the files from the root-directory to the
> >>>>filesystem, it is similiar to genext2fs, but genext2fs doesn't fully
> >>>>support ext4.
> >>>>
> >>>>* Questions
> >>>> - Is such an option acceptable ?
> >>>>
> >>>> - Most of the code have been in debugfs/debugfs.c already, I moved them to
> >>>> misc/util.c and modified them to let both mke2fs and debugfs can use them,
> >>>> maybe we should put these code in another separate file ?
> >>>>
> >>>> - Where can I get the up-to-date development git repo, please? I think that
> >>>> there would be conflicts with the dev git repo, I'd like to rebase it if I
> >>>> can get the repo, currently, I'm using this one:
> >>>>
> >>>> http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git
> >>>>
> >>>>* The size impact on misc/mke2fs:
> >>>> 1,677,297 -> 1,728,110 (non stripped, about 50K increases)
> >>>> 316,968 -> 325,160 (stripped, 8K increases)
> >>>>
> >>>>Please feel free to give your comments.
> >>>
> >>>If I understand correctly, after applied this patch set, we can copy
> >>>some files from a directory that is indicated by '-d' option when we
> >>>create a new ext4 file system. My concern is why we need to add this
> >>>option? Without this option I can use 'mkfs.ext4 ${DEV}', 'mount ${DEV}
> >>>${MNT}' and 'cp ${SRC} ${MNT}' to do the same thing. I am not
> >>>convinced. Could you please describe more details for this patch set?
> >>>
> >>
> >>Hi Zheng,
> >>
> >>Yes, you are right, we can run mkfs/mount/cp to do the same thing,
> >>but the problem is that the "mount" command requires the root privilege,
> >>but we may not have it when we are working on a sever (the mkfs doesn't
> >>need the root privilege), for example
> >>
> >>$ dd if=/dev/zero of=test.img count=0 bs=1k seek=2M
> >>$ mke2fs -t ext4 -F test.img -d <root-dir>
> >>
> >>Will create an ext4 image with the files copied without the root privilege.
> >
> >Interesting. As you said above, if you haven't the root privilege, you
> >won't use this file system. But at least this file system will be used
> >by some one who have the root privilege. He/she can do the rest of
> >works. That means that you do 'dd/mke2fs' and he or she does
> >'mount/cp'. I don't see any reason that all dirs and files must be
> >created by mke2fs. Any real case that needs to do this?
> >
>
> Hi Zheng,
>
> For example, in Linux embedded development, we usually create a filesystem
> image which contains the linux embedded OS, this image can be used by qemu,
> Live CD and so on.

Ah, got it. After adding this option, you can create a USB Live CD
directly and don't need to get the root privilege. Thanks for your
explanation.

- Zheng

2013-09-16 21:04:03

by Darren Hart

[permalink] [raw]
Subject: Re: [RFC 00/10] e2fsprogs/mke2fs: add an option: -d root-directory

On Mon, 2013-09-02 at 20:27 +0800, Zheng Liu wrote:
> On Mon, Sep 02, 2013 at 08:20:02PM +0800, Robert Yang wrote:
> >
> >
> > On 09/02/2013 07:55 PM, Zheng Liu wrote:
> > >On Mon, Sep 02, 2013 at 02:46:33PM +0800, Robert Yang wrote:
> > >>
> > >>
> > >>On 09/01/2013 11:26 AM, Zheng Liu wrote:
> > >>>Hi Robert,
> > >>>
> > >>>On Wed, Aug 28, 2013 at 01:25:50PM +0800, Robert Yang wrote:
> > >>>>This option is used for adding the files from the root-directory to the
> > >>>>filesystem, it is similiar to genext2fs, but genext2fs doesn't fully
> > >>>>support ext4.
> > >>>>
> > >>>>* Questions
> > >>>> - Is such an option acceptable ?
> > >>>>
> > >>>> - Most of the code have been in debugfs/debugfs.c already, I moved them to
> > >>>> misc/util.c and modified them to let both mke2fs and debugfs can use them,
> > >>>> maybe we should put these code in another separate file ?
> > >>>>
> > >>>> - Where can I get the up-to-date development git repo, please? I think that
> > >>>> there would be conflicts with the dev git repo, I'd like to rebase it if I
> > >>>> can get the repo, currently, I'm using this one:
> > >>>>
> > >>>> http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git
> > >>>>
> > >>>>* The size impact on misc/mke2fs:
> > >>>> 1,677,297 -> 1,728,110 (non stripped, about 50K increases)
> > >>>> 316,968 -> 325,160 (stripped, 8K increases)
> > >>>>
> > >>>>Please feel free to give your comments.
> > >>>
> > >>>If I understand correctly, after applied this patch set, we can copy
> > >>>some files from a directory that is indicated by '-d' option when we
> > >>>create a new ext4 file system. My concern is why we need to add this
> > >>>option? Without this option I can use 'mkfs.ext4 ${DEV}', 'mount ${DEV}
> > >>>${MNT}' and 'cp ${SRC} ${MNT}' to do the same thing. I am not
> > >>>convinced. Could you please describe more details for this patch set?
> > >>>
> > >>
> > >>Hi Zheng,
> > >>
> > >>Yes, you are right, we can run mkfs/mount/cp to do the same thing,
> > >>but the problem is that the "mount" command requires the root privilege,
> > >>but we may not have it when we are working on a sever (the mkfs doesn't
> > >>need the root privilege), for example
> > >>
> > >>$ dd if=/dev/zero of=test.img count=0 bs=1k seek=2M
> > >>$ mke2fs -t ext4 -F test.img -d <root-dir>
> > >>
> > >>Will create an ext4 image with the files copied without the root privilege.
> > >
> > >Interesting. As you said above, if you haven't the root privilege, you
> > >won't use this file system. But at least this file system will be used
> > >by some one who have the root privilege. He/she can do the rest of
> > >works. That means that you do 'dd/mke2fs' and he or she does
> > >'mount/cp'. I don't see any reason that all dirs and files must be
> > >created by mke2fs. Any real case that needs to do this?
> > >
> >
> > Hi Zheng,
> >
> > For example, in Linux embedded development, we usually create a filesystem
> > image which contains the linux embedded OS, this image can be used by qemu,
> > Live CD and so on.
>
> Ah, got it. After adding this option, you can create a USB Live CD
> directly and don't need to get the root privilege. Thanks for your
> explanation.
>

Robert covered this well, but if you want some more context, the
original email kicking this off is here:

http://www.spinics.net/lists/linux-ext4/msg35352.html

Thanks,

--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel



2013-10-14 02:41:31

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [RFC 01/10] mke2fs.c: add an option: -d root-directory

On Wed, Aug 28, 2013 at 01:25:51PM +0800, Robert Yang wrote:
> @@ -2773,7 +2776,6 @@ no_journal:
> "filesystem accounting information: "));
> checkinterval = fs->super->s_checkinterval;
> max_mnt_count = fs->super->s_max_mnt_count;
> - retval = ext2fs_close(fs);
> if (retval) {
> fprintf(stderr,
> _("\nWarning, had trouble writing out superblocks."));

You can't just move the call to ext2fs_close(). You also need to move

if (!quiet)
printf(_("Writing superblocks and "
"filesystem accounting information: "));

before the call to ext2fs_close() since this is used to print the
message for the progress information that will be emitted by
ext2fs_close(), and you also have to move the error checking:

if (retval) {
fprintf(stderr,
_("\nWarning, had trouble writing out superblocks."));
...

after the call to ext2fs_close().


This would have been ***painfully*** obvious if you had run the
regression test suite. ("make -j8 ; make -j8 check"), since the
inconsistent move of ext2fs_close() without the preceeding printf
would cause all of the mke2fs tests (the m_* tests) to fail.

This is why regression test suites are so important. :-)

- Ted

2013-10-14 12:22:18

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH] mke2fs: fix up the commit "mke2fs.c: add an option: -d root-directory"

Otherwise lots of test failures!

Signed-off-by: "Theodore Ts'o" <[email protected]>
---
misc/mke2fs.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index d82ccbb..28537da 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -2779,26 +2779,8 @@ no_journal:
EXT4_FEATURE_RO_COMPAT_QUOTA))
create_quota_inodes(fs);

- if (!quiet)
- printf(_("Writing superblocks and "
- "filesystem accounting information: "));
checkinterval = fs->super->s_checkinterval;
max_mnt_count = fs->super->s_max_mnt_count;
- if (retval) {
- fprintf(stderr,
- _("\nWarning, had trouble writing out superblocks."));
- } else if (!quiet) {
- printf(_("done\n\n"));
- if (!getenv("MKE2FS_SKIP_CHECK_MSG"))
- print_check_message(max_mnt_count, checkinterval);
- }
-
- remove_error_table(&et_ext2_error_table);
- remove_error_table(&et_prof_error_table);
- profile_release(profile);
- for (i=0; fs_types[i]; i++)
- free(fs_types[i]);
- free(fs_types);

/* Copy files from the specified directory */
if (root_dir) {
@@ -2824,7 +2806,25 @@ no_journal:
_("\nError while populating %s"), root_dir);
}

+ if (!quiet)
+ printf(_("Writing superblocks and "
+ "filesystem accounting information: "));
retval = ext2fs_close(fs);
+ if (retval) {
+ fprintf(stderr,
+ _("\nWarning, had trouble writing out superblocks."));
+ } else if (!quiet) {
+ printf(_("done\n\n"));
+ if (!getenv("MKE2FS_SKIP_CHECK_MSG"))
+ print_check_message(max_mnt_count, checkinterval);
+ }
+
+ remove_error_table(&et_ext2_error_table);
+ remove_error_table(&et_prof_error_table);
+ profile_release(profile);
+ for (i=0; fs_types[i]; i++)
+ free(fs_types[i]);
+ free(fs_types);

return retval;
}
--
1.7.12.rc0.22.gcdd159b


2013-10-14 14:40:16

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [RFC 10/10] debugfs: use the functions in misc/util.c

On Wed, Aug 28, 2013 at 01:26:00PM +0800, Robert Yang wrote:
> Use the functions in mist/util.c to reduce the duplicated code.

This patch conflicts with an earlier patch you had sent which allowed
copy_file() to handle sparse files.

Also, could you move all of the functions which you added to
misc/util.c to a new file, named something like misc/create_inode.c,
or something like that?

That will avoid bloating the tune2fs binary, which won't need any of
these new functions.

Eventually, I plan to rename libquota to libinternal, and use it for
internal functions used by e2fsprogs, and at that point we'll move
misc/util.c, e2fsck/profile.c, and the functions for creating inodes
into this libinternal library.

But for now, if you could organize these functions into a one or more
separate .c files, and leave them in the misc directory, that would be
great.

Thanks!!

- Ted

2013-10-14 16:26:26

by Darren Hart

[permalink] [raw]
Subject: Re: [RFC 01/10] mke2fs.c: add an option: -d root-directory

On Sun, 2013-10-13 at 22:41 -0400, Theodore Ts'o wrote:
> On Wed, Aug 28, 2013 at 01:25:51PM +0800, Robert Yang wrote:
> > @@ -2773,7 +2776,6 @@ no_journal:
> > "filesystem accounting information: "));
> > checkinterval = fs->super->s_checkinterval;
> > max_mnt_count = fs->super->s_max_mnt_count;
> > - retval = ext2fs_close(fs);
> > if (retval) {
> > fprintf(stderr,
> > _("\nWarning, had trouble writing out superblocks."));
>
> You can't just move the call to ext2fs_close(). You also need to move
>
> if (!quiet)
> printf(_("Writing superblocks and "
> "filesystem accounting information: "));
>
> before the call to ext2fs_close() since this is used to print the
> message for the progress information that will be emitted by
> ext2fs_close(), and you also have to move the error checking:
>
> if (retval) {
> fprintf(stderr,
> _("\nWarning, had trouble writing out superblocks."));
> ...
>
> after the call to ext2fs_close().
>
>
> This would have been ***painfully*** obvious if you had run the
> regression test suite. ("make -j8 ; make -j8 check"), since the
> inconsistent move of ext2fs_close() without the preceeding printf
> would cause all of the mke2fs tests (the m_* tests) to fail.
>
> This is why regression test suites are so important. :-)
>
> - Ted

Robert,

Can you please take Ted's feedback into account (this and his response
to patch 10/10) and prepare another version of the patches.

As Ted suggests here, please run the regression test suite prior to any
future patch submissions. Looks like we missed some critical bits by not
doing that.

Ted, thank you for the response and taking the time to point out the
test suite. Robert, could you check the README and see if anything needs
to get updated there to make sure other developers are aware of the
regressions suite and how to run it?

Thanks,


--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel



2013-10-15 01:38:21

by Robert Yang

[permalink] [raw]
Subject: Re: [RFC 01/10] mke2fs.c: add an option: -d root-directory



On 10/15/2013 12:26 AM, Darren Hart wrote:
> On Sun, 2013-10-13 at 22:41 -0400, Theodore Ts'o wrote:
>> On Wed, Aug 28, 2013 at 01:25:51PM +0800, Robert Yang wrote:
>>> @@ -2773,7 +2776,6 @@ no_journal:
>>> "filesystem accounting information: "));
>>> checkinterval = fs->super->s_checkinterval;
>>> max_mnt_count = fs->super->s_max_mnt_count;
>>> - retval = ext2fs_close(fs);
>>> if (retval) {
>>> fprintf(stderr,
>>> _("\nWarning, had trouble writing out superblocks."));
>>
>> You can't just move the call to ext2fs_close(). You also need to move
>>
>> if (!quiet)
>> printf(_("Writing superblocks and "
>> "filesystem accounting information: "));
>>
>> before the call to ext2fs_close() since this is used to print the
>> message for the progress information that will be emitted by
>> ext2fs_close(), and you also have to move the error checking:
>>
>> if (retval) {
>> fprintf(stderr,
>> _("\nWarning, had trouble writing out superblocks."));
>> ...
>>
>> after the call to ext2fs_close().
>>
>>
>> This would have been ***painfully*** obvious if you had run the
>> regression test suite. ("make -j8 ; make -j8 check"), since the
>> inconsistent move of ext2fs_close() without the preceeding printf
>> would cause all of the mke2fs tests (the m_* tests) to fail.
>>
>> This is why regression test suites are so important. :-)
>>
>> - Ted
>
> Robert,
>
> Can you please take Ted's feedback into account (this and his response
> to patch 10/10) and prepare another version of the patches.
>

Yes, of course:-)

> As Ted suggests here, please run the regression test suite prior to any
> future patch submissions. Looks like we missed some critical bits by not
> doing that.
>
> Ted, thank you for the response and taking the time to point out the
> test suite. Robert, could you check the README and see if anything needs
> to get updated there to make sure other developers are aware of the
> regressions suite and how to run it?
>

OK, I will, thanks Ted and Darren.

// Robert

> Thanks,
>
>