Re: [PATCH 2/2] xfs_db: add an ls command

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Oct 28, 2020 at 12:27:03PM +1100, Dave Chinner wrote:
> On Mon, Oct 26, 2020 at 04:32:41PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Add to xfs_db the ability to list a directory.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  db/namei.c               |  380 ++++++++++++++++++++++++++++++++++++++++++++++
> >  libxfs/libxfs_api_defs.h |    1 
> >  man/man8/xfs_db.8        |   14 ++
> >  3 files changed, 395 insertions(+)
> > 
> > 
> > diff --git a/db/namei.c b/db/namei.c
> > index 3c9889d62338..b2c036e6777a 100644
> > --- a/db/namei.c
> > +++ b/db/namei.c
> > @@ -221,8 +221,388 @@ static const cmdinfo_t path_cmd = {
> >  	.help		= path_help,
> >  };
> >  
> > +/* List a directory's entries. */
> > +
> > +static const char *filetype_strings[XFS_DIR3_FT_MAX] = {
> > +	[XFS_DIR3_FT_UNKNOWN]	= N_("unknown"),
> > +	[XFS_DIR3_FT_REG_FILE]	= N_("regular"),
> > +	[XFS_DIR3_FT_DIR]	= N_("directory"),
> > +	[XFS_DIR3_FT_CHRDEV]	= N_("chardev"),
> > +	[XFS_DIR3_FT_BLKDEV]	= N_("blkdev"),
> > +	[XFS_DIR3_FT_FIFO]	= N_("fifo"),
> > +	[XFS_DIR3_FT_SOCK]	= N_("socket"),
> > +	[XFS_DIR3_FT_SYMLINK]	= N_("symlink"),
> > +	[XFS_DIR3_FT_WHT]	= N_("whiteout"),
> > +};
> 
> What does N_() do that is different to _()?

LOL, it doesn't do anything at all!

Sigh... WTF was the point of commit 97294b227aefd?

> > +static const char *
> > +get_dstr(
> > +	struct xfs_mount	*mp,
> > +	uint8_t			filetype)
> > +{
> > +	if (!xfs_sb_version_hasftype(&mp->m_sb))
> > +		return filetype_strings[XFS_DIR3_FT_UNKNOWN];
> > +
> > +	if (filetype >= XFS_DIR3_FT_MAX)
> > +		return filetype_strings[XFS_DIR3_FT_UNKNOWN];
> > +
> > +	return filetype_strings[filetype];
> > +}
> > +
> > +static void
> > +dir_emit(
> > +	struct xfs_mount	*mp,
> > +	char			*name,
> > +	ssize_t			namelen,
> > +	xfs_ino_t		ino,
> > +	uint8_t			dtype)
> > +{
> > +	char			*display_name;
> > +	struct xfs_name		xname = { .name = name };
> > +	const char		*dstr = get_dstr(mp, dtype);
> > +	xfs_dahash_t		hash;
> > +	bool			good;
> > +
> > +	if (namelen < 0) {
> > +		/* Negative length means that name is null-terminated. */
> > +		display_name = name;
> > +		xname.len = strlen(name);
> > +		good = true;
> > +	} else {
> > +		/*
> > +		 * Otherwise, name came from a directory entry, so we have to
> > +		 * copy the string to a buffer so that we can add the null
> > +		 * terminator.
> > +		 */
> > +		display_name = malloc(namelen + 1);
> > +		memcpy(display_name, name, namelen);
> > +		display_name[namelen] = 0;
> > +		xname.len = namelen;
> > +		good = libxfs_dir2_namecheck(name, namelen);
> > +	}
> > +	hash = libxfs_dir2_hashname(mp, &xname);
> > +
> > +	dbprintf("%-18llu %-14s 0x%08llx %3d %s", ino, dstr, hash, xname.len,
> > +			display_name);
> > +	if (!good)
> > +		dbprintf(_(" (corrupt)"));
> > +	dbprintf("\n");
> 
> Can we get this to emit the directory offset of the entry as well?

Er... I think so.  Do you want to report the u32 value that gets loaded
in ctx->pos?  Or the actual byte offset within the directory?

> Also, can this be done as a single dbprintf call like this?
> 
> 	dbprintf(%-18llu %-14s 0x%08llx %3d %s %s\n",
> 		ino, dstr, hash, xname.len, display_name,
> 		good ? _("(good)") : _("(corrupt)"));
> 
> (there will be lots of output on big directories....)

Ok.

> > +static int
> > +list_sfdir(
> > +	struct xfs_da_args		*args)
> > +{
> > +	struct xfs_inode		*dp = args->dp;
> > +	struct xfs_mount		*mp = dp->i_mount;
> > +	struct xfs_dir2_sf_entry	*sfep;
> > +	struct xfs_dir2_sf_hdr		*sfp;
> > +	xfs_ino_t			ino;
> > +	unsigned int			i;
> > +	uint8_t				filetype;
> > +
> > +	sfp = (struct xfs_dir2_sf_hdr *)dp->i_df.if_u1.if_data;
> > +
> > +	/* . and .. entries */
> > +	dir_emit(args->dp->i_mount, ".", -1, dp->i_ino, XFS_DIR3_FT_DIR);
> > +
> > +	ino = libxfs_dir2_sf_get_parent_ino(sfp);
> > +	dir_emit(args->dp->i_mount, "..", -1, ino, XFS_DIR3_FT_DIR);
> > +
> > +	/* Walk everything else. */
> > +	sfep = xfs_dir2_sf_firstentry(sfp);
> > +	for (i = 0; i < sfp->count; i++) {
> > +		ino = libxfs_dir2_sf_get_ino(mp, sfp, sfep);
> > +		filetype = libxfs_dir2_sf_get_ftype(mp, sfep);
> > +
> > +		dir_emit(args->dp->i_mount, (char *)sfep->name, sfep->namelen,
> > +				ino, filetype);
> > +		sfep = libxfs_dir2_sf_nextentry(mp, sfp, sfep);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Hmmm - how much of the xfs_readdir() implementation from the kernel
> does this duplicate? It doesn't contain the seek cookie stuff, but
> otherwise it's almost identical, right?

Yep.  I think it also omits a fair amount of error handling since we'd
rather just keep going for as long as we can.

> [....]
> 
> > +/* If the io cursor points to a directory, list its contents. */
> > +static int
> > +ls_cur(
> > +	char			*tag,
> > +	bool			direct)
> 
> I find the name "direct" rather confusing here. according to
> the help below, it will be true when we want to "list the directory
> itself, not it's contents"....
> 
> 
> > +{
> > +	struct xfs_inode	*dp;
> > +	int			ret = 0;
> > +
> > +	if (iocur_top->typ != &typtab[TYP_INODE]) {
> > +		dbprintf(_("current object is not an inode.\n"));
> > +		return -1;
> > +	}
> > +
> > +	ret = -libxfs_iget(mp, NULL, iocur_top->ino, 0, &dp);
> > +	if (ret) {
> > +		dbprintf(_("failed to iget directory %llu, error %d\n"),
> > +				(unsigned long long)iocur_top->ino, ret);
> > +		return -1;
> > +	}
> > +
> > +	if (S_ISDIR(VFS_I(dp)->i_mode) && !direct) {
> > +		/* List the contents of a directory. */
> > +		if (tag)
> > +			dbprintf(_("%s:\n"), tag);
> > +
> > +		ret = listdir(dp);
> > +		if (ret) {
> > +			dbprintf(_("failed to list directory %llu: %s\n"),
> > +					(unsigned long long)iocur_top->ino,
> > +					strerror(ret));
> > +			ret = -1;
> > +			goto rele;
> > +		}
> > +	} else if (direct || !S_ISDIR(VFS_I(dp)->i_mode)) {
> > +		/* List the directory entry associated with a single file. */
> > +		char		inum[32];
> > +
> > +		if (!tag) {
> > +			snprintf(inum, sizeof(inum), "<%llu>",
> > +					(unsigned long long)iocur_top->ino);
> > +			tag = inum;
> > +		} else {
> > +			char	*p = strrchr(tag, '/');
> > +
> > +			if (p)
> > +				tag = p + 1;
> > +		}
> > +
> > +		dir_emit(mp, tag, -1, iocur_top->ino,
> > +				libxfs_mode_to_ftype(VFS_I(dp)->i_mode));
> 
> I'm not sure what this is supposed to do - we turn the current inode
> if it's not a directory into a -directory entry- without actually
> know it's name? And we can pass in an inode that isn't a directory
> and do the same? This doesn't make a huge amount of sense to me - it
> tries to display the inode number as a dirent?

I added this (somewhat confusing) ability so that fstests could resolve
a path to an inode number without having to dig any farther into the
disk format.

IOWs, you can do:

ino=$(_scratch_xfs_db -c 'ls -d /usr/bin/bash')

to get the inode number directly.  Without this, you'd have to do
something horrible like this...

ino=$(_scratch_xfs_db -c 'path /usr/bin/bash' -c 'print' -c 'stack' /dev/sda | \
	tr ',' ' ' | \
	awk '{if ($1 ~ /inumber/) {print $3; exit(0); } else if ($1 == "inode") {print $2; exit(0);}}')

To map a path to an inode number.  I thought it made a lot more sense to
do that in C (even if it makes the xfs_db CLI a little weird) than
implement a bunch of string parsing after the fact.

Maybe I should just simplify it to "display the inode number of whatever
the path resolves to" instead of constructing an artificial directory
entry.

> > +	} else {
> > +		dbprintf(_("current inode %llu is not a directory.\n"),
> > +				(unsigned long long)iocur_top->ino);
> > +		ret = -1;
> > +		goto rele;
> > +	}
> 
> I don't think we can get to this else branch. If we don't take the
> first branch (dir && !direct), the either we are not a dir or direct
> is set. The second branch will then be taken if we are not a dir or
> direct is set....

Yes, I /will/ do that.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com