Hi Brian,
>
> Well, the code looks Ok to me but the design still seems overdone IMO. I
> have no major objection if this goes in as is (with one exception noted
> below), but IMO we should take the approach somewhat discussed in the v3
> review.
First of all, thanks for reviewing the patch again, I took some time to reply
because I was waiting for any other comments and had a few other things to do.
>
> In particular, define an actual default behavior for the command to
> return the largest inode number (or return 1/0 for "ability to mount w/
> inode32" as Dave suggested). For example, kill both of the -l and -s
> flags and just return 1/0 by default. Define a single verbose (-v) flag
> to print the combined inode number and size. This mode can be
> implemented as the body of the inode_f() function. If -n is specified,
> basically do what the current version does. Just my .02.
>
I agree with you here and tbh, I don't really think a -v flag is really needed,
although it can certainly facilitate the usage of the xfs_io -c "inode".
Checking for the size of the inode returned against UINT32_MAX is not that hard
anyway, but I think keeping a very simple return value for the command might be
the best approach.
I'm going to re-write it and send a V5 today including a review of your comment
below.
thanks again for the review
> > io/open.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 151 insertions(+)
> >
> > diff --git a/io/open.c b/io/open.c
> > index ac5a5e0..2fc8aab 100644
> > --- a/io/open.c
> > +++ b/io/open.c
> ...
> > + if (ret_lsize || ret_largest) {
> > +
> > + bulkreq.lastip = &last;
> > + bulkreq.icount = 1024; /* User-defined maybe!? */
> > + bulkreq.ubuffer = &igroup;
> > + bulkreq.ocount = &count;
> > +
> > + for (;;) {
> > +
> > + if (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS,
> > + &bulkreq)) {
> > + perror("XFS_IOC_FSINUMBERS");
> > + exitcode = 1;
> > + return 0;
> > + }
> > +
> > + if (count == 0)
> > + break;
> > +
> > + lastgrp = count;
> > + }
> > +
> > + lastgrp--;
> > + igrp_rec = igroup[lastgrp];
>
> IIRC the point of igrp_rec was to save off the last record so it could
> be used directly after the loop without need for the count (because it
> could be 0). Here we use a separate lastgrp count to protect against the
> 0 case, yet still do the record copy after the loop... what's the point
> of that?
>
> Brian
>
> > + lastino = igrp_rec.xi_startino +
> > + xfs_highbit64(igrp_rec.xi_allocmask);
> > +
> > + if (ret_lsize)
> > + printf (_("Largest inode size: %d\n"),
> > + lastino > XFS_MAXINUMBER_32 ? 64 : 32);
> > + else
> > + printf(_("Largest inode: %llu\n"), lastino);
> > +
> > + return 0;
> > + }
> > +
> > + return command_usage(&inode_cmd);
> > +}
> > +
> > void
> > open_init(void)
> > {
> > @@ -815,6 +955,16 @@ open_init(void)
> > _("get/set preferred extent size (in bytes) for the open file");
> > extsize_cmd.help = extsize_help;
> >
> > + inode_cmd.name = "inode";
> > + inode_cmd.cfunc = inode_f;
> > + inode_cmd.args = _("[-s | -l | -n] [num]");
> > + inode_cmd.argmin = 1;
> > + inode_cmd.argmax = 2;
> > + inode_cmd.flags = CMD_NOMAP_OK;
> > + inode_cmd.oneline =
> > + _("Query inode number usage in the filesystem");
> > + inode_cmd.help = inode_help;
> > +
> > add_command(&open_cmd);
> > add_command(&stat_cmd);
> > add_command(&close_cmd);
> > @@ -822,4 +972,5 @@ open_init(void)
> > add_command(&chproj_cmd);
> > add_command(&lsproj_cmd);
> > add_command(&extsize_cmd);
> > + add_command(&inode_cmd);
> > }
> > --
> > 2.4.3
> >
> > _______________________________________________
> > xfs mailing list
> > xfs@xxxxxxxxxxx
> > http://oss.sgi.com/mailman/listinfo/xfs
--
Carlos
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
