Re: [PATCH 1/2] xfs_db: add a directory path lookup command

[Dave Chinner <david@fromorbit.com>]

On Mon, Oct 26, 2020 at 04:32:34PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add a command to xfs_db so that we can navigate to inodes by path.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
....
> +/* Given a directory and a structured path, walk the path and set the cursor. */
> +static int
> +path_navigate(
> +	struct xfs_mount	*mp,
> +	xfs_ino_t		rootino,
> +	struct dirpath		*dirpath)
> +{
> +	struct xfs_inode	*dp;
> +	xfs_ino_t		ino = rootino;
> +	unsigned int		i;
> +	int			error;
> +
> +	error = -libxfs_iget(mp, NULL, ino, 0, &dp);
> +	if (error)
> +		return error;
> +
> +	for (i = 0; i < dirpath->depth; i++) {
> +		struct xfs_name	xname = {
> +			.name	= dirpath->path[i],
> +			.len	= strlen(dirpath->path[i]),
> +		};
> +
> +		if (!S_ISDIR(VFS_I(dp)->i_mode)) {
> +			error = ENOTDIR;
> +			goto rele;
> +		}
> +
> +		error = -libxfs_dir_lookup(NULL, dp, &xname, &ino, NULL);
> +		if (error)
> +			goto rele;
> +		if (!xfs_verify_ino(mp, ino)) {
> +			error = EFSCORRUPTED;
> +			goto rele;
> +		}
> +
> +		libxfs_irele(dp);
> +		dp = NULL;
> +
> +		error = -libxfs_iget(mp, NULL, ino, 0, &dp);
> +		switch (error) {
> +		case EFSCORRUPTED:
> +		case EFSBADCRC:
> +		case 0:
> +			break;
> +		default:
> +			return error;
> +		}
> +	}
> +
> +	set_cur_inode(ino);
> +rele:
> +	if (dp)
> +		libxfs_irele(dp);
> +	return error;
> +}

This could return negative errors....

> +/* Walk a directory path to an inode and set the io cursor to that inode. */
> +static int
> +path_walk(
> +	char		*path)
> +{
> +	struct dirpath	*dirpath;
> +	char		*p = path;
> +	xfs_ino_t	rootino = mp->m_sb.sb_rootino;
> +	int		ret = 0;
> +
> +	if (*p == '/') {
> +		/* Absolute path, start from the root inode. */
> +		p++;
> +	} else {
> +		/* Relative path, start from current dir. */
> +		if (iocur_top->typ != &typtab[TYP_INODE]) {
> +			dbprintf(_("current object is not an inode.\n"));
> +			return -1;
> +		}
> +
> +		if (!S_ISDIR(iocur_top->mode)) {
> +			dbprintf(_("current inode %llu is not a directory.\n"),
> +					(unsigned long long)iocur_top->ino);
> +			return -1;
> +		}
> +		rootino = iocur_top->ino;
> +	}
> +
> +	dirpath = path_parse(p);
> +	if (!dirpath) {
> +		dbprintf(_("%s: not enough memory to parse.\n"), path);
> +		return -1;
> +	}

and this could return -ENOMEM here with no error message....

> +
> +	ret = path_navigate(mp, rootino, dirpath);
> +	if (ret) {
> +		dbprintf(_("%s: %s\n"), path, strerror(ret));
> +		ret = -1;
> +	}

... don't overwrite ret here, move the dbprintf() to the caller and
the one error message captures all possible errors.

Also, no need for _() for a format string that contains no
translatable text....

Otherwise, looks fine.

-Dave.
-- 
Dave Chinner
david@fromorbit.com