Date:	Fri, 21 Dec 2012 09:11:43 -0800
From:	Darren Hart <dvhart@...radead.org>
To:	"Darrick J. Wong" <darrick.wong@...cle.com>
CC:	linux-ext4@...r.kernel.org, tytso@....edu, adilger@...ger.ca,
	sgw@...ux.intel.com
Subject: Re: [PATCH 2/3] lib/ext2fs: Add ext2fs_symlink

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@...radead.org>
>> Cc: "Theodore Ts'o" <tytso@....edu>
>> Cc: Andreas Dilger <adilger@...ger.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@...r.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@...r.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists