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@xxxxxxxxxx>
> >
> > Add a command to xfs_db so that we can navigate to inodes by path.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ....
> > +/* 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@xxxxxxxxxxxxx
