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

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

On Wed, Oct 28, 2020 at 11:35:51AM +1100, Dave Chinner wrote:
> 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....

Ok, will fix.  Thanks!

--D

> Otherwise, looks fine.
> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com