[PATCH 1/3] lib/ext2fs: Correct interates typo in texinfo docs

[PATCH 1/3] lib/ext2fs: Correct interates typo in texinfo docs

[Darren Hart]

Just a typo fix.

Signed-off-by: Darren Hart <dvhart@infradead.org>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger@dilger.ca>
---
 doc/libext2fs.texinfo | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/libext2fs.texinfo b/doc/libext2fs.texinfo
index 9d8e7f1..8272d1d 100644
--- a/doc/libext2fs.texinfo
+++ b/doc/libext2fs.texinfo
@@ -767,7 +767,7 @@ to initialize directory entries for @file{.} and @file{..}, respectively.
 
 @deftypefun errcode_t ext2fs_dir_iterate (ext2_filsys @var{fs}, ext2_ino_t @var{dir}, int @var{flags}, char *@var{block_buf}, int (*@var{func})(struct ext2_dir_entry *@var{dirent}, int @var{offset}, int @var{blocksize}, char *@var{buf}, void *@var{private}), void *@var{private})
 
-This function interates over all of the directory entries in the
+This function iterates over all of the directory entries in the
 directory @var{dir}, calling the callback function @var{func} for each
 directory entry in the directory.  The @var{block_buf} parameter should
 either be NULL, or if the @code{ext2fs_dir_iterate} function is 
-- 
1.7.11.7

[PATCH RFC 0/3][RESEND] ext2fsprogs: Symlink support and doc fix

[Darren Hart]

As we appear to have agreed that adding symlink support to debugfs via a new
ext2fs_symlink() function in libext2fs was something that needed doing, I
thought I'd use this as a trial run for my first contribution to the ext2fsprogs
package before I continue working on the larger project of completing initial
directory support for mke2fs.

While I modeled this patch after existing code, their were some inconcsistencies
in the code examples I used that I'd welcome input on. In particular:

o Should the ext2fs_link() happen right after ext2fs_new_inode()? Or should it
  happen closer to the end of the operation? do_write() and ext2fs_mkdir()
  handle this differently.

o Is it necessary to allocate the first block and assign it to the inode or the
  extents? ext2fs_mkdir() does this, do_write() does not. I opted for the
  simpler of the two and it passes my initial simple tests.

o Should I pass "mode" to ext2fs_new_inode() even though it is ignored?

o Would it make sense to try once to expand_dir rather than bailing out of
  ext2fs_mkdir() and ext2fs_symlink() if the directory is full?

o What would we like the initial uid,gid,mode,*time values to be for
  files/directories/links/etc. created with libext2fs?

Finally, I made an attempt to follow the coding style I observed in the code,
but if I missed something, please let me know.

Thanks,

Darren Hart
Intel Open Source Technology Center

[PATCH 1/3] lib/ext2fs: Correct interates typo in texinfo docs

[Darren Hart]

Just a typo fix.

Signed-off-by: Darren Hart <dvhart@infradead.org>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger@dilger.ca>
---
 doc/libext2fs.texinfo | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/libext2fs.texinfo b/doc/libext2fs.texinfo
index 9d8e7f1..8272d1d 100644
--- a/doc/libext2fs.texinfo
+++ b/doc/libext2fs.texinfo
@@ -767,7 +767,7 @@ to initialize directory entries for @file{.} and @file{..}, respectively.
 
 @deftypefun errcode_t ext2fs_dir_iterate (ext2_filsys @var{fs}, ext2_ino_t @var{dir}, int @var{flags}, char *@var{block_buf}, int (*@var{func})(struct ext2_dir_entry *@var{dirent}, int @var{offset}, int @var{blocksize}, char *@var{buf}, void *@var{private}), void *@var{private})
 
-This function interates over all of the directory entries in the
+This function iterates over all of the directory entries in the
 directory @var{dir}, calling the callback function @var{func} for each
 directory entry in the directory.  The @var{block_buf} parameter should
 either be NULL, or if the @code{ext2fs_dir_iterate} function is 
-- 
1.7.11.7

[PATCH 2/3] lib/ext2fs: Add ext2fs_symlink

[Darren Hart]

Creating symlinks is a complex affair when accounting for slowlinks.

Create a new function, ext2fs_symlink(), modeled after ext2fs_mkdir()
with input from debugfs's do_write() and copy_file(). Like
ext2fs_mkdir(), ext2fs_symlink() takes on the task of allocating a new
inode, setting up sane default values in the inode, accounting for the
inode stats, and copying the target path to either the inode (for
fastlinks) or to the blocks/extents (for slowlinks) using
ext2fs_file_write().

It does not attempt to expand the parent directory, instead returning
EXT2_ET_DIR_NO_SPACE and leaving it to the caller to expand just as
ext2fs_mkdir() does. Ideally, I think both of these functions should
make a single attempt to expand the directory.

Adds 1556 bytes to libext2fs.a (stripped).

Signed-off-by: Darren Hart <dvhart@infradead.org>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger@dilger.ca>
---
 lib/ext2fs/Makefile.in    |   8 +++
 lib/ext2fs/ext2_err.et.in |   3 +
 lib/ext2fs/symlink.c      | 137 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 148 insertions(+)
 create mode 100644 lib/ext2fs/symlink.c

diff --git a/lib/ext2fs/Makefile.in b/lib/ext2fs/Makefile.in
index e05b438..5411826 100644
--- a/lib/ext2fs/Makefile.in
+++ b/lib/ext2fs/Makefile.in
@@ -80,6 +80,7 @@ OBJS= $(DEBUGFS_LIB_OBJS) $(RESIZE_LIB_OBJS) $(E2IMAGE_LIB_OBJS) \
 	res_gdt.o \
 	rw_bitmaps.o \
 	swapfs.o \
+	symlink.o \
 	tdb.o \
 	undo_io.o \
 	unix_io.o \
@@ -155,6 +156,7 @@ SRCS= ext2_err.c \
 	$(srcdir)/res_gdt.c \
 	$(srcdir)/rw_bitmaps.c \
 	$(srcdir)/swapfs.c \
+	$(srcdir)/symlink.c \
 	$(srcdir)/tdb.c \
 	$(srcdir)/test_io.c \
 	$(srcdir)/tst_badblocks.c \
@@ -881,6 +883,12 @@ swapfs.o: $(srcdir)/swapfs.c $(top_builddir)/lib/config.h \
  $(srcdir)/ext2_fs.h $(srcdir)/ext3_extents.h $(top_srcdir)/lib/et/com_err.h \
  $(srcdir)/ext2_io.h $(top_builddir)/lib/ext2fs/ext2_err.h \
  $(srcdir)/ext2_ext_attr.h $(srcdir)/bitops.h
+symlink.o: $(srcdir)/symlink.c $(top_builddir)/lib/config.h \
+ $(top_builddir)/lib/dirpaths.h $(srcdir)/ext2_fs.h \
+ $(top_builddir)/lib/ext2fs/ext2_types.h $(srcdir)/ext2fs.h \
+ $(srcdir)/ext2_fs.h $(srcdir)/ext3_extents.h $(top_srcdir)/lib/et/com_err.h \
+ $(srcdir)/ext2_io.h $(top_builddir)/lib/ext2fs/ext2_err.h \
+ $(srcdir)/ext2_ext_attr.h $(srcdir)/bitops.h
 tdb.o: $(srcdir)/tdb.c $(top_builddir)/lib/config.h \
  $(top_builddir)/lib/dirpaths.h $(srcdir)/tdb.h
 test_io.o: $(srcdir)/test_io.c $(top_builddir)/lib/config.h \
diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in
index c99a167..e1313a8 100644
--- a/lib/ext2fs/ext2_err.et.in
+++ b/lib/ext2fs/ext2_err.et.in
@@ -248,6 +248,9 @@ ec	EXT2_ET_DB_NOT_FOUND,
 ec	EXT2_ET_DIR_EXISTS,
 	"Ext2 directory already exists"
 
+ec	EXT2_ET_FILE_EXISTS,
+	"Ext2 file already exists"
+
 ec	EXT2_ET_UNIMPLEMENTED,
 	"Unimplemented ext2 library function"
 
diff --git a/lib/ext2fs/symlink.c b/lib/ext2fs/symlink.c
new file mode 100644
index 0000000..5bfb9fc
--- /dev/null
+++ b/lib/ext2fs/symlink.c
@@ -0,0 +1,137 @@
+/*
+ * symlink.c --- make a symlink in the filesystem, based on mkdir.c
+ *
+ * Copyright (c) 2012, Intel Corporation.
+ * All Rights Reserved.
+ *
+ * %Begin-Header%
+ * This file may be redistributed under the terms of the GNU Library
+ * General Public License, version 2.
+ * %End-Header%
+ */
+
+#include "config.h"
+#include <stdio.h>
+#include <string.h>
+#if HAVE_UNISTD_H
+#include <unistd.h>
+#endif
+#include <fcntl.h>
+#include <time.h>
+#if HAVE_SYS_STAT_H
+#include <sys/stat.h>
+#endif
+#if HAVE_SYS_TYPES_H
+#include <sys/types.h>
+#endif
+
+#include "ext2_fs.h"
+#include "ext2fs.h"
+
+#ifndef EXT2_FT_SYMLINK
+#define EXT2_FT_SYMLINK		7
+#endif
+
+errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
+                         const char *name, char *target)
+{
+	ext2_extent_handle_t	handle;
+	errcode_t		retval;
+	struct ext2_inode	inode;
+	ext2_ino_t		scratch_ino;
+	int			fastlink;
+	int			target_len;
+
+	EXT2_CHECK_MAGIC(fs, EXT2_ET_MAGIC_EXT2FS_FILSYS);
+
+	/*
+	 * Allocate an inode, if necessary
+	 */
+	if (!ino) {
+		retval = ext2fs_new_inode(fs, parent, LINUX_S_IFLNK | 0755,
+		                          0, &ino);
+		if (retval)
+			goto cleanup;
+	}
+
+	/*
+	 * Link the symlink into the filesystem hierarchy
+	 */
+	retval = ext2fs_lookup(fs, parent, name, strlen(name), 0,
+	                       &scratch_ino);
+	if (!retval) {
+		retval = EXT2_ET_FILE_EXISTS;
+		goto cleanup;
+	}
+	if (retval != EXT2_ET_FILE_NOT_FOUND)
+		goto cleanup;
+	retval = ext2fs_link(fs, parent, name, ino, EXT2_FT_SYMLINK);
+	if (retval)
+		goto cleanup;
+
+	ext2fs_inode_alloc_stats2(fs, ino, +1, 1);
+
+	/*
+	 * Create the inode structure....
+	 */
+	memset(&inode, 0, sizeof(struct ext2_inode));
+	inode.i_mode = LINUX_S_IFLNK | 0777;
+	inode.i_uid = inode.i_gid = 0;
+	/* FIXME: set the time fields */
+	inode.i_links_count = 1;
+
+	target_len = strlen(target);
+	fastlink = (target_len < sizeof(inode.i_block));
+	if (fastlink) {
+		/* Fast symlinks, target stored in inode */
+		inode.i_size = target_len;
+		strcpy((char *)&inode.i_block, target);
+	} else {
+		if (retval)
+			goto cleanup;
+
+		if (fs->super->s_feature_incompat & EXT3_FEATURE_INCOMPAT_EXTENTS)
+			inode.i_flags |= EXT4_EXTENTS_FL;
+
+		inode.i_size = (target_len % fs->blocksize) ?
+		               target_len + (fs->blocksize - target_len) : target_len;
+	}
+
+	/*
+	 * Write out the inode and inode data block.  The inode generation
+	 * number is assigned by write_new_inode, which means that the file
+	 * operations below should come after it.
+	 */
+	retval = ext2fs_write_new_inode(fs, ino, &inode);
+	if (retval)
+		goto cleanup;
+
+	/*
+	 * For slow links, the target path is written to the blocks/extents
+	 */
+	if (!fastlink) {
+		ext2_extent_handle_t handle;
+		ext2_file_t file;
+		int written = 0;
+		char *rem;
+
+		/* Write the target to file */
+		retval = ext2fs_file_open2(fs, ino, &inode,
+		                           EXT2_FILE_CREATE | EXT2_FILE_WRITE,
+		                           &file);
+		if (retval)
+			goto cleanup;
+
+		rem = target;
+		while (strlen(rem)) {
+			retval = ext2fs_file_write(file, (void *)rem, strlen(rem), &written);
+			rem += written;
+			if (retval)
+				break;
+		}
+		ext2fs_file_close(file);
+	}
+
+cleanup:
+	return retval;
+}
-- 
1.7.11.7

[PATCH 3/3] debugfs: Add symlink command

[Darren Hart]

Add support for symbolic links using a new symlink command.
Modeled after the do_mkdir() command.

Very long target paths fail as the command parsing appears to truncate the
input to somewhere around 256 bytes. Independent testing of the
ext2fs_symlink() command demonstrates it works with multi-block-length
pathes.

Signed-off-by: Darren Hart <dvhart@infradead.org>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger@dilger.ca>
---
 debugfs/debug_cmds.ct |  3 +++
 debugfs/debugfs.8.in  |  3 +++
 debugfs/debugfs.c     | 43 +++++++++++++++++++++++++++++++++++++++++++
 debugfs/debugfs.h     |  1 +
 debugfs/dump.c        |  2 +-
 5 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/debugfs/debug_cmds.ct b/debugfs/debug_cmds.ct
index a799dd7..338192f 100644
--- a/debugfs/debug_cmds.ct
+++ b/debugfs/debug_cmds.ct
@@ -160,6 +160,9 @@ request do_bmap, "Calculate the logical->physical block mapping for an inode",
 request do_punch, "Punch (or truncate) blocks from an inode by deallocating them",
 	punch, truncate;
 
+request do_symlink, "Create a symbolic link",
+	symlink;
+
 request do_imap, "Calculate the location of an inode",
 	imap;
 
diff --git a/debugfs/debugfs.8.in b/debugfs/debugfs.8.in
index bfcb1a8..c3b49f9 100644
--- a/debugfs/debugfs.8.in
+++ b/debugfs/debugfs.8.in
@@ -469,6 +469,9 @@ is, all of the blocks starting at
 .I start_blk
 through to the end of the file will be deallocated.
 .TP
+.I symlink filespec target
+Make a symbolic link.
+.TP
 .I pwd
 Print the current working directory.
 .TP
diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index aec61ab..6c4c1b1 100644
--- a/debugfs/debugfs.c
+++ b/debugfs/debugfs.c
@@ -2204,6 +2204,49 @@ void do_punch(int argc, char *argv[])
 }
 #endif /* READ_ONLY */
 
+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;
+	}
+
+}
+
 void do_dump_mmp(int argc EXT2FS_ATTR((unused)), char *argv[])
 {
 #if CONFIG_MMP
diff --git a/debugfs/debugfs.h b/debugfs/debugfs.h
index 994577c..434dbf0 100644
--- a/debugfs/debugfs.h
+++ b/debugfs/debugfs.h
@@ -133,6 +133,7 @@ extern void do_imap(int argc, char **argv);
 extern void do_set_current_time(int argc, char **argv);
 extern void do_supported_features(int argc, char **argv);
 extern void do_punch(int argc, char **argv);
+extern void do_symlink(int argc, char **argv);
 
 extern void do_dump_mmp(int argc, char **argv);
 extern void do_set_mmp_value(int argc, char **argv);
diff --git a/debugfs/dump.c b/debugfs/dump.c
index a15a0b7..2d830c9 100644
--- a/debugfs/dump.c
+++ b/debugfs/dump.c
@@ -285,7 +285,7 @@ static void rdump_inode(ext2_ino_t ino, struct ext2_inode *inode,
 
 		fix_perms("rdump", inode, -1, fullname);
 	}
-	/* else do nothing (don't dump device files, sockets, fifos, etc.) */
+	/* else do nothing (don't dump device files, sockets, fifos, symlinks, etc.) */
 
 errout:
 	free(fullname);
-- 
1.7.11.7

Re: [PATCH 2/3] lib/ext2fs: Add ext2fs_symlink

[Darrick J. Wong]

On Wed, Dec 19, 2012 at 06:49:22PM -0800, Darren Hart wrote:
> Creating symlinks is a complex affair when accounting for slowlinks.
> 
> Create a new function, ext2fs_symlink(), modeled after ext2fs_mkdir()
> with input from debugfs's do_write() and copy_file(). Like
> ext2fs_mkdir(), ext2fs_symlink() takes on the task of allocating a new
> inode, setting up sane default values in the inode, accounting for the
> inode stats, and copying the target path to either the inode (for
> fastlinks) or to the blocks/extents (for slowlinks) using
> ext2fs_file_write().
> 
> It does not attempt to expand the parent directory, instead returning
> EXT2_ET_DIR_NO_SPACE and leaving it to the caller to expand just as
> ext2fs_mkdir() does. Ideally, I think both of these functions should
> make a single attempt to expand the directory.
> 
> Adds 1556 bytes to libext2fs.a (stripped).
> 
> Signed-off-by: Darren Hart <dvhart@infradead.org>
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Andreas Dilger <adilger@dilger.ca>
> ---
>  lib/ext2fs/Makefile.in    |   8 +++
>  lib/ext2fs/ext2_err.et.in |   3 +
>  lib/ext2fs/symlink.c      | 137 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 148 insertions(+)
>  create mode 100644 lib/ext2fs/symlink.c
> 
> diff --git a/lib/ext2fs/Makefile.in b/lib/ext2fs/Makefile.in
> index e05b438..5411826 100644
> --- a/lib/ext2fs/Makefile.in
> +++ b/lib/ext2fs/Makefile.in
> @@ -80,6 +80,7 @@ OBJS= $(DEBUGFS_LIB_OBJS) $(RESIZE_LIB_OBJS) $(E2IMAGE_LIB_OBJS) \
>  	res_gdt.o \
>  	rw_bitmaps.o \
>  	swapfs.o \
> +	symlink.o \
>  	tdb.o \
>  	undo_io.o \
>  	unix_io.o \
> @@ -155,6 +156,7 @@ SRCS= ext2_err.c \
>  	$(srcdir)/res_gdt.c \
>  	$(srcdir)/rw_bitmaps.c \
>  	$(srcdir)/swapfs.c \
> +	$(srcdir)/symlink.c \
>  	$(srcdir)/tdb.c \
>  	$(srcdir)/test_io.c \
>  	$(srcdir)/tst_badblocks.c \
> @@ -881,6 +883,12 @@ swapfs.o: $(srcdir)/swapfs.c $(top_builddir)/lib/config.h \
>   $(srcdir)/ext2_fs.h $(srcdir)/ext3_extents.h $(top_srcdir)/lib/et/com_err.h \
>   $(srcdir)/ext2_io.h $(top_builddir)/lib/ext2fs/ext2_err.h \
>   $(srcdir)/ext2_ext_attr.h $(srcdir)/bitops.h
> +symlink.o: $(srcdir)/symlink.c $(top_builddir)/lib/config.h \
> + $(top_builddir)/lib/dirpaths.h $(srcdir)/ext2_fs.h \
> + $(top_builddir)/lib/ext2fs/ext2_types.h $(srcdir)/ext2fs.h \
> + $(srcdir)/ext2_fs.h $(srcdir)/ext3_extents.h $(top_srcdir)/lib/et/com_err.h \
> + $(srcdir)/ext2_io.h $(top_builddir)/lib/ext2fs/ext2_err.h \
> + $(srcdir)/ext2_ext_attr.h $(srcdir)/bitops.h
>  tdb.o: $(srcdir)/tdb.c $(top_builddir)/lib/config.h \
>   $(top_builddir)/lib/dirpaths.h $(srcdir)/tdb.h
>  test_io.o: $(srcdir)/test_io.c $(top_builddir)/lib/config.h \
> diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in
> index c99a167..e1313a8 100644
> --- a/lib/ext2fs/ext2_err.et.in
> +++ b/lib/ext2fs/ext2_err.et.in
> @@ -248,6 +248,9 @@ ec	EXT2_ET_DB_NOT_FOUND,
>  ec	EXT2_ET_DIR_EXISTS,
>  	"Ext2 directory already exists"
>  
> +ec	EXT2_ET_FILE_EXISTS,
> +	"Ext2 file already exists"
> +
>  ec	EXT2_ET_UNIMPLEMENTED,
>  	"Unimplemented ext2 library function"
>  
> diff --git a/lib/ext2fs/symlink.c b/lib/ext2fs/symlink.c
> new file mode 100644
> index 0000000..5bfb9fc
> --- /dev/null
> +++ b/lib/ext2fs/symlink.c
> @@ -0,0 +1,137 @@
> +/*
> + * symlink.c --- make a symlink in the filesystem, based on mkdir.c
> + *
> + * Copyright (c) 2012, Intel Corporation.
> + * All Rights Reserved.
> + *
> + * %Begin-Header%
> + * This file may be redistributed under the terms of the GNU Library
> + * General Public License, version 2.
> + * %End-Header%
> + */
> +
> +#include "config.h"
> +#include <stdio.h>
> +#include <string.h>
> +#if HAVE_UNISTD_H
> +#include <unistd.h>
> +#endif
> +#include <fcntl.h>
> +#include <time.h>
> +#if HAVE_SYS_STAT_H
> +#include <sys/stat.h>
> +#endif
> +#if HAVE_SYS_TYPES_H
> +#include <sys/types.h>
> +#endif
> +
> +#include "ext2_fs.h"
> +#include "ext2fs.h"
> +
> +#ifndef EXT2_FT_SYMLINK
> +#define EXT2_FT_SYMLINK		7
> +#endif

Is this ever /not/ defined?  ext2_fs.h should always define this, right?

> +
> +errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
> +                         const char *name, char *target)
> +{
> +	ext2_extent_handle_t	handle;
> +	errcode_t		retval;
> +	struct ext2_inode	inode;
> +	ext2_ino_t		scratch_ino;
> +	int			fastlink;
> +	int			target_len;
> +
> +	EXT2_CHECK_MAGIC(fs, EXT2_ET_MAGIC_EXT2FS_FILSYS);
> +
> +	/*
> +	 * Allocate an inode, if necessary
> +	 */
> +	if (!ino) {
> +		retval = ext2fs_new_inode(fs, parent, LINUX_S_IFLNK | 0755,
> +		                          0, &ino);
> +		if (retval)
> +			goto cleanup;
> +	}
> +
> +	/*
> +	 * Link the symlink into the filesystem hierarchy
> +	 */
> +	retval = ext2fs_lookup(fs, parent, name, strlen(name), 0,
> +	                       &scratch_ino);
> +	if (!retval) {
> +		retval = EXT2_ET_FILE_EXISTS;
> +		goto cleanup;
> +	}
> +	if (retval != EXT2_ET_FILE_NOT_FOUND)
> +		goto cleanup;
> +	retval = ext2fs_link(fs, parent, name, ino, EXT2_FT_SYMLINK);
> +	if (retval)
> +		goto cleanup;
> +
> +	ext2fs_inode_alloc_stats2(fs, ino, +1, 1);

You know, I've always wondered about why there are unary plus scattered
throughout calls to this function in e2fsprogs.

> +	/*
> +	 * Create the inode structure....
> +	 */
> +	memset(&inode, 0, sizeof(struct ext2_inode));
> +	inode.i_mode = LINUX_S_IFLNK | 0777;
> +	inode.i_uid = inode.i_gid = 0;
> +	/* FIXME: set the time fields */

Are you going to fix this? :)

> +	inode.i_links_count = 1;
> +
> +	target_len = strlen(target);
> +	fastlink = (target_len < sizeof(inode.i_block));
> +	if (fastlink) {
> +		/* Fast symlinks, target stored in inode */
> +		inode.i_size = target_len;
> +		strcpy((char *)&inode.i_block, target);
> +	} else {
> +		if (retval)
> +			goto cleanup;
> +
> +		if (fs->super->s_feature_incompat & EXT3_FEATURE_INCOMPAT_EXTENTS)
> +			inode.i_flags |= EXT4_EXTENTS_FL;
> +
> +		inode.i_size = (target_len % fs->blocksize) ?
> +		               target_len + (fs->blocksize - target_len) : target_len;

Shouldn't this just be i_size = target_len?

$ ln -s "$(perl -e 'print "x" x 4000;')" cow

...nets me a symlink with a reported length of 4000.

Come to think of it, ext4 (kernel) doesn't seem to allow slow symlinks to have
a target longer than $blocksize.  I suspect that might be a side effect of
blocksize <= pagesize, and therefore we allocate at most a page's worth to read
the target into.

Either way, we probably ought to check.

Also, I think the VFS errors out if you try to pass '' as the target, so
perhaps we ought to look for that.

I think the rest looks more or less ok, barring the 80 char overflows that I
think is happening on the ext2fs_file_write line.

--D

> +	}
> +
> +	/*
> +	 * Write out the inode and inode data block.  The inode generation
> +	 * number is assigned by write_new_inode, which means that the file
> +	 * operations below should come after it.
> +	 */
> +	retval = ext2fs_write_new_inode(fs, ino, &inode);
> +	if (retval)
> +		goto cleanup;
> +
> +	/*
> +	 * For slow links, the target path is written to the blocks/extents
> +	 */
> +	if (!fastlink) {
> +		ext2_extent_handle_t handle;
> +		ext2_file_t file;
> +		int written = 0;
> +		char *rem;
> +
> +		/* Write the target to file */
> +		retval = ext2fs_file_open2(fs, ino, &inode,
> +		                           EXT2_FILE_CREATE | EXT2_FILE_WRITE,
> +		                           &file);
> +		if (retval)
> +			goto cleanup;
> +
> +		rem = target;
> +		while (strlen(rem)) {
> +			retval = ext2fs_file_write(file, (void *)rem, strlen(rem), &written);
> +			rem += written;
> +			if (retval)
> +				break;
> +		}
> +		ext2fs_file_close(file);
> +	}
> +
> +cleanup:
> +	return retval;
> +}
> -- 
> 1.7.11.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] lib/ext2fs: Add ext2fs_symlink

[Darren Hart]

Hi Darrick!

On 12/20/2012 01:51 PM, Darrick J. Wong wrote:
> On Wed, Dec 19, 2012 at 06:49:22PM -0800, Darren Hart wrote:
>> Creating symlinks is a complex affair when accounting for slowlinks.
>>
>> Create a new function, ext2fs_symlink(), modeled after ext2fs_mkdir()
>> with input from debugfs's do_write() and copy_file(). Like
>> ext2fs_mkdir(), ext2fs_symlink() takes on the task of allocating a new
>> inode, setting up sane default values in the inode, accounting for the
>> inode stats, and copying the target path to either the inode (for
>> fastlinks) or to the blocks/extents (for slowlinks) using
>> ext2fs_file_write().
>>
>> It does not attempt to expand the parent directory, instead returning
>> EXT2_ET_DIR_NO_SPACE and leaving it to the caller to expand just as
>> ext2fs_mkdir() does. Ideally, I think both of these functions should
>> make a single attempt to expand the directory.
>>
>> Adds 1556 bytes to libext2fs.a (stripped).
>>
>> Signed-off-by: Darren Hart <dvhart@infradead.org>
>> Cc: "Theodore Ts'o" <tytso@mit.edu>
>> Cc: Andreas Dilger <adilger@dilger.ca>
>> ---
>>  lib/ext2fs/Makefile.in    |   8 +++
>>  lib/ext2fs/ext2_err.et.in |   3 +
>>  lib/ext2fs/symlink.c      | 137 ++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 148 insertions(+)
>>  create mode 100644 lib/ext2fs/symlink.c
>>
>> diff --git a/lib/ext2fs/Makefile.in b/lib/ext2fs/Makefile.in
>> index e05b438..5411826 100644
>> --- a/lib/ext2fs/Makefile.in
>> +++ b/lib/ext2fs/Makefile.in
>> @@ -80,6 +80,7 @@ OBJS= $(DEBUGFS_LIB_OBJS) $(RESIZE_LIB_OBJS) $(E2IMAGE_LIB_OBJS) \
>>  	res_gdt.o \
>>  	rw_bitmaps.o \
>>  	swapfs.o \
>> +	symlink.o \
>>  	tdb.o \
>>  	undo_io.o \
>>  	unix_io.o \
>> @@ -155,6 +156,7 @@ SRCS= ext2_err.c \
>>  	$(srcdir)/res_gdt.c \
>>  	$(srcdir)/rw_bitmaps.c \
>>  	$(srcdir)/swapfs.c \
>> +	$(srcdir)/symlink.c \
>>  	$(srcdir)/tdb.c \
>>  	$(srcdir)/test_io.c \
>>  	$(srcdir)/tst_badblocks.c \
>> @@ -881,6 +883,12 @@ swapfs.o: $(srcdir)/swapfs.c $(top_builddir)/lib/config.h \
>>   $(srcdir)/ext2_fs.h $(srcdir)/ext3_extents.h $(top_srcdir)/lib/et/com_err.h \
>>   $(srcdir)/ext2_io.h $(top_builddir)/lib/ext2fs/ext2_err.h \
>>   $(srcdir)/ext2_ext_attr.h $(srcdir)/bitops.h
>> +symlink.o: $(srcdir)/symlink.c $(top_builddir)/lib/config.h \
>> + $(top_builddir)/lib/dirpaths.h $(srcdir)/ext2_fs.h \
>> + $(top_builddir)/lib/ext2fs/ext2_types.h $(srcdir)/ext2fs.h \
>> + $(srcdir)/ext2_fs.h $(srcdir)/ext3_extents.h $(top_srcdir)/lib/et/com_err.h \
>> + $(srcdir)/ext2_io.h $(top_builddir)/lib/ext2fs/ext2_err.h \
>> + $(srcdir)/ext2_ext_attr.h $(srcdir)/bitops.h
>>  tdb.o: $(srcdir)/tdb.c $(top_builddir)/lib/config.h \
>>   $(top_builddir)/lib/dirpaths.h $(srcdir)/tdb.h
>>  test_io.o: $(srcdir)/test_io.c $(top_builddir)/lib/config.h \
>> diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in
>> index c99a167..e1313a8 100644
>> --- a/lib/ext2fs/ext2_err.et.in
>> +++ b/lib/ext2fs/ext2_err.et.in
>> @@ -248,6 +248,9 @@ ec	EXT2_ET_DB_NOT_FOUND,
>>  ec	EXT2_ET_DIR_EXISTS,
>>  	"Ext2 directory already exists"
>>  
>> +ec	EXT2_ET_FILE_EXISTS,
>> +	"Ext2 file already exists"
>> +
>>  ec	EXT2_ET_UNIMPLEMENTED,
>>  	"Unimplemented ext2 library function"
>>  
>> diff --git a/lib/ext2fs/symlink.c b/lib/ext2fs/symlink.c
>> new file mode 100644
>> index 0000000..5bfb9fc
>> --- /dev/null
>> +++ b/lib/ext2fs/symlink.c
>> @@ -0,0 +1,137 @@
>> +/*
>> + * symlink.c --- make a symlink in the filesystem, based on mkdir.c
>> + *
>> + * Copyright (c) 2012, Intel Corporation.
>> + * All Rights Reserved.
>> + *
>> + * %Begin-Header%
>> + * This file may be redistributed under the terms of the GNU Library
>> + * General Public License, version 2.
>> + * %End-Header%
>> + */
>> +
>> +#include "config.h"
>> +#include <stdio.h>
>> +#include <string.h>
>> +#if HAVE_UNISTD_H
>> +#include <unistd.h>
>> +#endif
>> +#include <fcntl.h>
>> +#include <time.h>
>> +#if HAVE_SYS_STAT_H
>> +#include <sys/stat.h>
>> +#endif
>> +#if HAVE_SYS_TYPES_H
>> +#include <sys/types.h>
>> +#endif
>> +
>> +#include "ext2_fs.h"
>> +#include "ext2fs.h"
>> +
>> +#ifndef EXT2_FT_SYMLINK
>> +#define EXT2_FT_SYMLINK		7
>> +#endif
> 
> Is this ever /not/ defined?  ext2_fs.h should always define this, right?


I thought the same thing, but I'm following convention here from mkdir.c.


>> +
>> +errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
>> +                         const char *name, char *target)
>> +{
>> +	ext2_extent_handle_t	handle;
>> +	errcode_t		retval;
>> +	struct ext2_inode	inode;
>> +	ext2_ino_t		scratch_ino;
>> +	int			fastlink;
>> +	int			target_len;
>> +
>> +	EXT2_CHECK_MAGIC(fs, EXT2_ET_MAGIC_EXT2FS_FILSYS);
>> +
>> +	/*
>> +	 * Allocate an inode, if necessary
>> +	 */
>> +	if (!ino) {
>> +		retval = ext2fs_new_inode(fs, parent, LINUX_S_IFLNK | 0755,
>> +		                          0, &ino);
>> +		if (retval)
>> +			goto cleanup;
>> +	}
>> +
>> +	/*
>> +	 * Link the symlink into the filesystem hierarchy
>> +	 */
>> +	retval = ext2fs_lookup(fs, parent, name, strlen(name), 0,
>> +	                       &scratch_ino);
>> +	if (!retval) {
>> +		retval = EXT2_ET_FILE_EXISTS;
>> +		goto cleanup;
>> +	}
>> +	if (retval != EXT2_ET_FILE_NOT_FOUND)
>> +		goto cleanup;
>> +	retval = ext2fs_link(fs, parent, name, ino, EXT2_FT_SYMLINK);
>> +	if (retval)
>> +		goto cleanup;
>> +
>> +	ext2fs_inode_alloc_stats2(fs, ino, +1, 1);
> 
> You know, I've always wondered about why there are unary plus scattered
> throughout calls to this function in e2fsprogs.


Me too :-) Again, just following convention.


>> +	/*
>> +	 * Create the inode structure....
>> +	 */
>> +	memset(&inode, 0, sizeof(struct ext2_inode));
>> +	inode.i_mode = LINUX_S_IFLNK | 0777;
>> +	inode.i_uid = inode.i_gid = 0;
>> +	/* FIXME: set the time fields */
> 
> Are you going to fix this? :)


It is an open question as to what they should be initialized to. Should
the current time be used? Should a fixed time be used for all of a
debugfs (or similar) session? This strikes me as a policy decision which
I was hoping to leave to the maintainers to dictate (at which point I
would implement it).


>> +	inode.i_links_count = 1;
>> +
>> +	target_len = strlen(target);
>> +	fastlink = (target_len < sizeof(inode.i_block));
>> +	if (fastlink) {
>> +		/* Fast symlinks, target stored in inode */
>> +		inode.i_size = target_len;
>> +		strcpy((char *)&inode.i_block, target);
>> +	} else {
>> +		if (retval)
>> +			goto cleanup;
>> +
>> +		if (fs->super->s_feature_incompat & EXT3_FEATURE_INCOMPAT_EXTENTS)
>> +			inode.i_flags |= EXT4_EXTENTS_FL;
>> +
>> +		inode.i_size = (target_len % fs->blocksize) ?
>> +		               target_len + (fs->blocksize - target_len) : target_len;
> 
> Shouldn't this just be i_size = target_len?


Hrm... I thought it had to be block, but I can't remember where I got
that notion from. I confirmed with your example and a few others, that
target_len is indeed correct. Nice, I hated that parameterized integer
ceiling function anyway :-)


> 
> $ ln -s "$(perl -e 'print "x" x 4000;')" cow
> 
> ...nets me a symlink with a reported length of 4000.
> 
> Come to think of it, ext4 (kernel) doesn't seem to allow slow symlinks to have
> a target longer than $blocksize.  I suspect that might be a side effect of
> blocksize <= pagesize, and therefore we allocate at most a page's worth to read
> the target into.
> 
> Either way, we probably ought to check.


Agreed. I'll give it a closer look.


> Also, I think the VFS errors out if you try to pass '' as the target, so
> perhaps we ought to look for that.


Will do.


> I think the rest looks more or less ok, barring the 80 char overflows that I
> think is happening on the ext2fs_file_write line.


I'm happy to enforce 80 chars if that is the preferred coding style.
There were many instances where the existing code went beyond 80, so I
wasn't sure what was preferred and was trying my best to follow convention.

I'm away with family until the new year, so my V2 will come in early
January, but I'll be able to respond to discussions here.

Thanks for taking the time to review Darrick!

--
Darren

> 
> --D
> 
>> +	}
>> +
>> +	/*
>> +	 * Write out the inode and inode data block.  The inode generation
>> +	 * number is assigned by write_new_inode, which means that the file
>> +	 * operations below should come after it.
>> +	 */
>> +	retval = ext2fs_write_new_inode(fs, ino, &inode);
>> +	if (retval)
>> +		goto cleanup;
>> +
>> +	/*
>> +	 * For slow links, the target path is written to the blocks/extents
>> +	 */
>> +	if (!fastlink) {
>> +		ext2_extent_handle_t handle;
>> +		ext2_file_t file;
>> +		int written = 0;
>> +		char *rem;
>> +
>> +		/* Write the target to file */
>> +		retval = ext2fs_file_open2(fs, ino, &inode,
>> +		                           EXT2_FILE_CREATE | EXT2_FILE_WRITE,
>> +		                           &file);
>> +		if (retval)
>> +			goto cleanup;
>> +
>> +		rem = target;
>> +		while (strlen(rem)) {
>> +			retval = ext2fs_file_write(file, (void *)rem, strlen(rem), &written);
>> +			rem += written;
>> +			if (retval)
>> +				break;
>> +		}
>> +		ext2fs_file_close(file);
>> +	}
>> +
>> +cleanup:
>> +	return retval;
>> +}
>> -- 
>> 1.7.11.7
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/3] lib/ext2fs: Add ext2fs_symlink

[Darrick J. Wong]

On Fri, Dec 21, 2012 at 09:11:43AM -0800, Darren Hart wrote:
> Hi Darrick!

:D

> On 12/20/2012 01:51 PM, Darrick J. Wong wrote:
> > On Wed, Dec 19, 2012 at 06:49:22PM -0800, Darren Hart wrote:
> >> Creating symlinks is a complex affair when accounting for slowlinks.
> >>
> >> Create a new function, ext2fs_symlink(), modeled after ext2fs_mkdir()
> >> with input from debugfs's do_write() and copy_file(). Like
> >> ext2fs_mkdir(), ext2fs_symlink() takes on the task of allocating a new
> >> inode, setting up sane default values in the inode, accounting for the
> >> inode stats, and copying the target path to either the inode (for
> >> fastlinks) or to the blocks/extents (for slowlinks) using
> >> ext2fs_file_write().
> >>
> >> It does not attempt to expand the parent directory, instead returning
> >> EXT2_ET_DIR_NO_SPACE and leaving it to the caller to expand just as
> >> ext2fs_mkdir() does. Ideally, I think both of these functions should
> >> make a single attempt to expand the directory.
> >>
> >> Adds 1556 bytes to libext2fs.a (stripped).
> >>
> >> Signed-off-by: Darren Hart <dvhart@infradead.org>
> >> Cc: "Theodore Ts'o" <tytso@mit.edu>
> >> Cc: Andreas Dilger <adilger@dilger.ca>
> >> ---
> >>  lib/ext2fs/Makefile.in    |   8 +++
> >>  lib/ext2fs/ext2_err.et.in |   3 +
> >>  lib/ext2fs/symlink.c      | 137 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 148 insertions(+)
> >>  create mode 100644 lib/ext2fs/symlink.c
> >>
> >> diff --git a/lib/ext2fs/Makefile.in b/lib/ext2fs/Makefile.in
> >> index e05b438..5411826 100644
> >> --- a/lib/ext2fs/Makefile.in
> >> +++ b/lib/ext2fs/Makefile.in
> >> @@ -80,6 +80,7 @@ OBJS= $(DEBUGFS_LIB_OBJS) $(RESIZE_LIB_OBJS) $(E2IMAGE_LIB_OBJS) \
> >>  	res_gdt.o \
> >>  	rw_bitmaps.o \
> >>  	swapfs.o \
> >> +	symlink.o \
> >>  	tdb.o \
> >>  	undo_io.o \
> >>  	unix_io.o \
> >> @@ -155,6 +156,7 @@ SRCS= ext2_err.c \
> >>  	$(srcdir)/res_gdt.c \
> >>  	$(srcdir)/rw_bitmaps.c \
> >>  	$(srcdir)/swapfs.c \
> >> +	$(srcdir)/symlink.c \
> >>  	$(srcdir)/tdb.c \
> >>  	$(srcdir)/test_io.c \
> >>  	$(srcdir)/tst_badblocks.c \
> >> @@ -881,6 +883,12 @@ swapfs.o: $(srcdir)/swapfs.c $(top_builddir)/lib/config.h \
> >>   $(srcdir)/ext2_fs.h $(srcdir)/ext3_extents.h $(top_srcdir)/lib/et/com_err.h \
> >>   $(srcdir)/ext2_io.h $(top_builddir)/lib/ext2fs/ext2_err.h \
> >>   $(srcdir)/ext2_ext_attr.h $(srcdir)/bitops.h
> >> +symlink.o: $(srcdir)/symlink.c $(top_builddir)/lib/config.h \
> >> + $(top_builddir)/lib/dirpaths.h $(srcdir)/ext2_fs.h \
> >> + $(top_builddir)/lib/ext2fs/ext2_types.h $(srcdir)/ext2fs.h \
> >> + $(srcdir)/ext2_fs.h $(srcdir)/ext3_extents.h $(top_srcdir)/lib/et/com_err.h \
> >> + $(srcdir)/ext2_io.h $(top_builddir)/lib/ext2fs/ext2_err.h \
> >> + $(srcdir)/ext2_ext_attr.h $(srcdir)/bitops.h
> >>  tdb.o: $(srcdir)/tdb.c $(top_builddir)/lib/config.h \
> >>   $(top_builddir)/lib/dirpaths.h $(srcdir)/tdb.h
> >>  test_io.o: $(srcdir)/test_io.c $(top_builddir)/lib/config.h \
> >> diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in
> >> index c99a167..e1313a8 100644
> >> --- a/lib/ext2fs/ext2_err.et.in
> >> +++ b/lib/ext2fs/ext2_err.et.in
> >> @@ -248,6 +248,9 @@ ec	EXT2_ET_DB_NOT_FOUND,
> >>  ec	EXT2_ET_DIR_EXISTS,
> >>  	"Ext2 directory already exists"
> >>  
> >> +ec	EXT2_ET_FILE_EXISTS,
> >> +	"Ext2 file already exists"
> >> +
> >>  ec	EXT2_ET_UNIMPLEMENTED,
> >>  	"Unimplemented ext2 library function"
> >>  
> >> diff --git a/lib/ext2fs/symlink.c b/lib/ext2fs/symlink.c
> >> new file mode 100644
> >> index 0000000..5bfb9fc
> >> --- /dev/null
> >> +++ b/lib/ext2fs/symlink.c
> >> @@ -0,0 +1,137 @@
> >> +/*
> >> + * symlink.c --- make a symlink in the filesystem, based on mkdir.c
> >> + *
> >> + * Copyright (c) 2012, Intel Corporation.
> >> + * All Rights Reserved.
> >> + *
> >> + * %Begin-Header%
> >> + * This file may be redistributed under the terms of the GNU Library
> >> + * General Public License, version 2.
> >> + * %End-Header%
> >> + */
> >> +
> >> +#include "config.h"
> >> +#include <stdio.h>
> >> +#include <string.h>
> >> +#if HAVE_UNISTD_H
> >> +#include <unistd.h>
> >> +#endif
> >> +#include <fcntl.h>
> >> +#include <time.h>
> >> +#if HAVE_SYS_STAT_H
> >> +#include <sys/stat.h>
> >> +#endif
> >> +#if HAVE_SYS_TYPES_H
> >> +#include <sys/types.h>
> >> +#endif
> >> +
> >> +#include "ext2_fs.h"
> >> +#include "ext2fs.h"
> >> +
> >> +#ifndef EXT2_FT_SYMLINK
> >> +#define EXT2_FT_SYMLINK		7
> >> +#endif
> > 
> > Is this ever /not/ defined?  ext2_fs.h should always define this, right?
> 
> 
> I thought the same thing, but I'm following convention here from mkdir.c.

<shrug> Was hoping Ted or anyone else could comment on this and the next bit...
> 
> 
> >> +
> >> +errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
> >> +                         const char *name, char *target)
> >> +{
> >> +	ext2_extent_handle_t	handle;
> >> +	errcode_t		retval;
> >> +	struct ext2_inode	inode;
> >> +	ext2_ino_t		scratch_ino;
> >> +	int			fastlink;
> >> +	int			target_len;
> >> +
> >> +	EXT2_CHECK_MAGIC(fs, EXT2_ET_MAGIC_EXT2FS_FILSYS);
> >> +
> >> +	/*
> >> +	 * Allocate an inode, if necessary
> >> +	 */
> >> +	if (!ino) {
> >> +		retval = ext2fs_new_inode(fs, parent, LINUX_S_IFLNK | 0755,
> >> +		                          0, &ino);
> >> +		if (retval)
> >> +			goto cleanup;
> >> +	}
> >> +
> >> +	/*
> >> +	 * Link the symlink into the filesystem hierarchy
> >> +	 */
> >> +	retval = ext2fs_lookup(fs, parent, name, strlen(name), 0,
> >> +	                       &scratch_ino);
> >> +	if (!retval) {
> >> +		retval = EXT2_ET_FILE_EXISTS;
> >> +		goto cleanup;
> >> +	}
> >> +	if (retval != EXT2_ET_FILE_NOT_FOUND)
> >> +		goto cleanup;
> >> +	retval = ext2fs_link(fs, parent, name, ino, EXT2_FT_SYMLINK);
> >> +	if (retval)
> >> +		goto cleanup;
> >> +
> >> +	ext2fs_inode_alloc_stats2(fs, ino, +1, 1);
> > 
> > You know, I've always wondered about why there are unary plus scattered
> > throughout calls to this function in e2fsprogs.
> 
> 
> Me too :-) Again, just following convention.
> 
> 
> >> +	/*
> >> +	 * Create the inode structure....
> >> +	 */
> >> +	memset(&inode, 0, sizeof(struct ext2_inode));
> >> +	inode.i_mode = LINUX_S_IFLNK | 0777;
> >> +	inode.i_uid = inode.i_gid = 0;
> >> +	/* FIXME: set the time fields */
> > 
> > Are you going to fix this? :)
> 
> 
> It is an open question as to what they should be initialized to. Should
> the current time be used? Should a fixed time be used for all of a
> debugfs (or similar) session? This strikes me as a policy decision which
> I was hoping to leave to the maintainers to dictate (at which point I
> would implement it).

ext2fs_write_new_inode sets atime = ctime = mtime = now() if you don't
otherwise set them.  If that's ok with you (I'm having trouble grokking why
you'd want to set all the symlink times to a fixed value...) then you could
just remove the comment.

> >> +	inode.i_links_count = 1;
> >> +
> >> +	target_len = strlen(target);
> >> +	fastlink = (target_len < sizeof(inode.i_block));
> >> +	if (fastlink) {
> >> +		/* Fast symlinks, target stored in inode */
> >> +		inode.i_size = target_len;
> >> +		strcpy((char *)&inode.i_block, target);
> >> +	} else {
> >> +		if (retval)
> >> +			goto cleanup;
> >> +
> >> +		if (fs->super->s_feature_incompat & EXT3_FEATURE_INCOMPAT_EXTENTS)
> >> +			inode.i_flags |= EXT4_EXTENTS_FL;
> >> +
> >> +		inode.i_size = (target_len % fs->blocksize) ?
> >> +		               target_len + (fs->blocksize - target_len) : target_len;
> > 
> > Shouldn't this just be i_size = target_len?
> 
> 
> Hrm... I thought it had to be block, but I can't remember where I got
> that notion from. I confirmed with your example and a few others, that
> target_len is indeed correct. Nice, I hated that parameterized integer
> ceiling function anyway :-)

Directory files have lengths that must be strict multiples of $blocksize,
because the driver writes (empty) directory entries all the way to the end of
each block.  As far as I know nothing else has that requirement.

> > 
> > $ ln -s "$(perl -e 'print "x" x 4000;')" cow
> > 
> > ...nets me a symlink with a reported length of 4000.
> > 
> > Come to think of it, ext4 (kernel) doesn't seem to allow slow symlinks to have
> > a target longer than $blocksize.  I suspect that might be a side effect of
> > blocksize <= pagesize, and therefore we allocate at most a page's worth to read
> > the target into.
> > 
> > Either way, we probably ought to check.
> 
> 
> Agreed. I'll give it a closer look.

Yep ... the first code stanza of ext4_symlink in fs/ext4/namei.c does indeed
enforce that.

> 
> > Also, I think the VFS errors out if you try to pass '' as the target, so
> > perhaps we ought to look for that.
> 
> 
> Will do.
> 
> 
> > I think the rest looks more or less ok, barring the 80 char overflows that I
> > think is happening on the ext2fs_file_write line.
> 
> 
> I'm happy to enforce 80 chars if that is the preferred coding style.
> There were many instances where the existing code went beyond 80, so I
> wasn't sure what was preferred and was trying my best to follow convention.

There's still quite a bit of old code in e2fsprogs that don't seem to follow
any conventions.  I'm not sure which conventions Ted wants for e2fsprogs, but
so far I haven't gotten any resistance from using kernel coding style.

> 
> I'm away with family until the new year, so my V2 will come in early
> January, but I'll be able to respond to discussions here.
> 
> Thanks for taking the time to review Darrick!

np!

--D
> 
> --
> Darren
> 
> > 
> > --D
> > 
> >> +	}
> >> +
> >> +	/*
> >> +	 * Write out the inode and inode data block.  The inode generation
> >> +	 * number is assigned by write_new_inode, which means that the file
> >> +	 * operations below should come after it.
> >> +	 */
> >> +	retval = ext2fs_write_new_inode(fs, ino, &inode);
> >> +	if (retval)
> >> +		goto cleanup;
> >> +
> >> +	/*
> >> +	 * For slow links, the target path is written to the blocks/extents
> >> +	 */
> >> +	if (!fastlink) {
> >> +		ext2_extent_handle_t handle;
> >> +		ext2_file_t file;
> >> +		int written = 0;
> >> +		char *rem;
> >> +
> >> +		/* Write the target to file */
> >> +		retval = ext2fs_file_open2(fs, ino, &inode,
> >> +		                           EXT2_FILE_CREATE | EXT2_FILE_WRITE,
> >> +		                           &file);
> >> +		if (retval)
> >> +			goto cleanup;
> >> +
> >> +		rem = target;
> >> +		while (strlen(rem)) {
> >> +			retval = ext2fs_file_write(file, (void *)rem, strlen(rem), &written);
> >> +			rem += written;
> >> +			if (retval)
> >> +				break;
> >> +		}
> >> +		ext2fs_file_close(file);
> >> +	}
> >> +
> >> +cleanup:
> >> +	return retval;
> >> +}
> >> -- 
> >> 1.7.11.7
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

Re: [PATCH 1/3] lib/ext2fs: Correct interates typo in texinfo docs

[Theodore Ts'o]

On Wed, Dec 19, 2012 at 06:49:21PM -0800, Darren Hart wrote:
> Just a typo fix.
> 
> Signed-off-by: Darren Hart <dvhart@infradead.org>
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Andreas Dilger <adilger@dilger.ca>

Thanks, applied.

						- Ted

Re: [PATCH 2/3] lib/ext2fs: Add ext2fs_symlink

[Theodore Ts'o]

On Fri, Dec 21, 2012 at 01:11:18PM -0800, Darrick J. Wong wrote:
> > >> --- a/lib/ext2fs/ext2_err.et.in
> > >> +++ b/lib/ext2fs/ext2_err.et.in
> > >> @@ -248,6 +248,9 @@ ec	EXT2_ET_DB_NOT_FOUND,
> > >>  ec	EXT2_ET_DIR_EXISTS,
> > >>  	"Ext2 directory already exists"
> > >>  
> > >> +ec	EXT2_ET_FILE_EXISTS,
> > >> +	"Ext2 file already exists"
> > >> +
> > >>  ec	EXT2_ET_UNIMPLEMENTED,
> > >>  	"Unimplemented ext2 library function"

Error codes have to be added at the end of the error table.  Otherwise
you end up changing the numbers assigned to the error codes, and it
would be like renumbering EINVAL, EAGAIN, etc.  It's part of the ABI
that we don't want to break.

> > >> +#ifndef EXT2_FT_SYMLINK
> > >> +#define EXT2_FT_SYMLINK		7
> > >> +#endif
> > > 
> > > Is this ever /not/ defined?  ext2_fs.h should always define this, right?
> > 
> > I thought the same thing, but I'm following convention here from mkdir.c.
> 
> <shrug> Was hoping Ted or anyone else could comment on this and the next bit...

This was needed a decade or more ago, back when used to include
ext2_fs.h from the kernel headers in /usr/include/linux.

We can remove it from mkdir.c, too.

> > >> +
> > >> +	ext2fs_inode_alloc_stats2(fs, ino, +1, 1);
> > > 
> > > You know, I've always wondered about why there are unary plus scattered
> > > throughout calls to this function in e2fsprogs.
> > 
> > Me too :-) Again, just following convention.

It's to make it easier to understand what is going on.  The inuse
parameter can be either -1 or +1.  If I was redoing this interface
from scratch today, I'd have done something like this:

void ext2fs_alloc_stats(ext2_filsys fs, ext2_ino_t ino, int flags);

#define EXT2FS_ALLOC_STATS_ALLOC_FL	0x0000
#define EXT2FS_ALLOC_STATS_DEALLOC_FL	0x0001
#define EXT2FS_ALLOC_STATS_DIR_FL	0x0002

Oh, well.... legacy....

> > >> +	/*
> > >> +	 * Create the inode structure....
> > >> +	 */
> > >> +	memset(&inode, 0, sizeof(struct ext2_inode));
> > >> +	inode.i_mode = LINUX_S_IFLNK | 0777;
> > >> +	inode.i_uid = inode.i_gid = 0;
> > >> +	/* FIXME: set the time fields */
> > > 
> > > Are you going to fix this? :)
> > 
> > It is an open question as to what they should be initialized to. Should
> > the current time be used? Should a fixed time be used for all of a
> > debugfs (or similar) session? This strikes me as a policy decision which
> > I was hoping to leave to the maintainers to dictate (at which point I
> > would implement it).

Nothing is needed here.  The time fields are set by ext2fs_write_new_inode(), 
as you've already noted.

> (I'm having trouble grokking why
> you'd want to set all the symlink times to a fixed value...) 

The reason for fs->now is so that if we are creating file systems for
the regression test suite; see the use of debugfs's command
set_current_time in tests/f_dup4/script, for example.

> > >> +		inode.i_size = (target_len % fs->blocksize) ?
> > >> +		               target_len + (fs->blocksize - target_len) : target_len;
> > > 
> > > Shouldn't this just be i_size = target_len?

Yup.

> > > Come to think of it, ext4 (kernel) doesn't seem to allow slow symlinks to have
> > > a target longer than $blocksize.  

Yes.  We need to add a check for this.

> > > I think the rest looks more or less ok, barring the 80 char overflows that I
> > > think is happening on the ext2fs_file_write line.
> > 
> > 
> > I'm happy to enforce 80 chars if that is the preferred coding style.
> > There were many instances where the existing code went beyond 80, so I
> > wasn't sure what was preferred and was trying my best to follow convention.

For new code, yes, we should try to keep things to 80 characters,
unless it really makes things harder to read.  Usually there was a
good reason why the rules were bent....


One other thing which I noted when I looked at the patch.  You need
make the cleanup code do the right thing if we get an error half-way
through the operation.  That's one of the reasons why we do the link
at the very end of the ext2fs_mkdir(), and why we allocate the block
by hand and either set i_block[0] by hand, or use
ext2fs_extent_set_bmap(), instead of using the ext2fs_file_*
operations; it makes the code easier to unwind in case of errors.

Basically, if we fail while we are writing the new directory block,
it's no big deal, since we are writing into an unallocated block.  We
save the call to ext2fs_link() for the very last, since that's the
call which is more work to unwind.

     	    	  	       	       	    - Ted