On Tue, Sep 22, 2015 at 09:54:32AM +0200, Carlos Maiolino wrote:
> On Tue, Sep 22, 2015 at 08:08:32AM +1000, Dave Chinner wrote:
> > On Mon, Sep 21, 2015 at 10:56:08AM +0200, Carlos Maiolino wrote:
...
> > > +
> > > + bulkreq.lastip = &last;
> > > + bulkreq.icount = 1;
> > > + bulkreq.ubuffer = &igroup;
> > > + bulkreq.ocount = &count;
> > > +
> > > + if (!xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS, &bulkreq)) {
> > > + if (count > 0) {
> > > + printf("Filesystem does have 64bit inodes\n");
> > > + return 0;
> > > + } else {
> > > + printf("Filesystem does not have 64bit inodes\n");
> > > + return 0;
> > > + }
> > > + }
> > > + perror("xfsctl(XFS_IOC_FSINUMBERS)");
> >
> > I'd do this the other way around:
> >
> > if (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS, &bulkreq)) {
> > perror("XFS_IOC_FSINUMBERS");
> > exitcode = 1;
> > return 0;
> > }
> >
> > if (count > 0)
> > printf("Filesystem does have 64bit inodes\n");
> > else
> > printf("Filesystem does not have 64bit inodes\n");
> > return 0;
> >
> > is sufficient, xfsctl is a one line wrapper around ioctl().
> >
>
> Ok, I'll change this, but, just a matter of curiosity, what are the technical
> reasons to write the conditional this way, instead of the way I wrote first?
> Just to avoid entering the conditional?
>
Not to speak for Dave, but I had the same thought just skimming through
the patch. I don't think it's a technical reason so much as a style one.
The further the function is grown, the easier it is to see that this
kind of flow is not the most friendly thing. E.g.:
ret = do_stuff();
if (!ret) {
...
ret = do_more_stuff();
if (!ret) {
...
}
}
In other words, the common execution path of the function starts to flow
"inward" rather than the natural top-down flow of the function:
ret = do_stuff();
if (ret)
handle_err;
ret = do_more_stuff();
if (ret)
handle_err;
...
The latter tends to be easier to follow, easier to review the error
handling cases, probably avoids indentation problems down the road, etc.
The function as it is right now is simple and so this probably isn't a
big deal, but I suspect seeing the latter form so frequently in all of
our other code is probably what causes something like the former to
stand out (even in a simple/harmless example). Just my .02. :)
Brian
>
> > > @@ -815,6 +847,12 @@ open_init(void)
> > > _("get/set preferred extent size (in bytes) for the open file");
> > > extsize_cmd.help = extsize_help;
> > >
> > > + inodes64_cmd.name = "inodes64";
> > > + inodes64_cmd.cfunc = inodes64_f;
> > > + inodes64_cmd.flags = CMD_NOMAP_OK;
> > > + inodes64_cmd.oneline =
> > > + _("Checks if filesyste contais 64bit inodes");
> > ^^^ ^^ 64 bit
> >
> > "inodes64" could be improved as a command name. e.g:
> >
> > oneline = _("Query inode number usage in the filesystem")
> >
> > And could do a little more to help users work out what the
> > problematic inode is. e.g:
> >
> > Long help:
> >
> > Inode_numbers [-s|-l] [num]
> >
> > [-s] returns the physical size of the largest inode
> > number in the filesystem (32 bit of 64 bit)
> > [-l] returns the largest inode number in the filesystem
> > [-n] returns the next inode number after [num]
> > [num] returns whether the inode number exists
> >
> > i.e. if you want 32 bit inodes in the fs, you can use [-n num] to
> > iterate all the inode numbers above 32 bits in size...
> >
>
> Ok, np
>
> > Cheers,
> >
> > Dave.
> > --
> > Dave Chinner
> > david@xxxxxxxxxxxxx
> >
> > _______________________________________________
> > 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
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
