Re: [PATCH 12/14] xfsprogs: make fsr use mntinfo when there is no mntent

[Jan Tulak <jtulak@xxxxxxxxxx>]

On Fri, Sep 25, 2015 at 12:53 AM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:

On Thu, Sep 24, 2015 at 04:38:49PM +0200, Jan Tulak wrote:
> On Wed, Sep 23, 2015 at 5:36 AM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>
> > On Tue, Sep 15, 2015 at 11:59:22AM +0200, Jan Tulak wrote:
> > > @@ -202,6 +205,27 @@ find_mountpoint(char *mtab, char *argname, struct
> > stat64 *sb)
> > >Â Â Â Â}
> > >
> > >Â Â Â Âwhile ((t = getmntent(mtabp))) {
> > > +#elif defined(HAVE_GETMNTINFO)
> > > +  Âstruct statfs Â*stats;
> > > +Â Â Âint error, i, count;
> > > +Â Â Â// because "t" is a pointer, but we don't need to use
> > > +Â Â Â// malloc for this usage
> > > +Â Â Âstruct mntent t_tmp;
> > > +Â Â Ât = &t_tmp;
> > > +
> > > +
> > > +Â Â Âerror = 0;
> > > +Â Â Âif ((count = getmntinfo(&stats, 0)) < 0) {
> > > +Â Â Â Â Â Â Âfprintf(stderr, _("%s: getmntinfo() failed: %s\n"),
> > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âprogname, strerror(errno));
> > > +Â Â Â Â Â Â Âreturn 0;
> > > +Â Â Â}
> > > +
> > > +Â Â Âfor (i = 0; i < count; i++) {
> > > +Â Â Â Â Â Â Âmntinfo2mntent(&stats[i], t);
> > > +#else
> > > +# error "How do I extract info about mounted filesystems on this
> > platform?"
> > > +#endif
> >
> > No, please don't do that. Having a loop iterator split across two
> > separate defines is unmaintainable. Write two separate functions
> > with the different loop iterators, then factor the common bit out
> > of them into a single function.
> >
> >
> I did a little refactoring to solve it. What I would like to ask abâout is
> this:
> When I can put ifdef just inside of a function like fnc(void) { #ifdef...
> #else ... #endif }, with little to no code outside of the ifdef, is it
> better to put the ifdef outside, or keep it inside?

The idea is that the "little differences" are put in functions that
end up in include/<platform>.h or libxfs/<platform>.c, so there are
no ifdefs in any of the application or library code. The build will
automatically include the correct function on the given platform,
and so the application code does not need such ifdefs at all.

e.g. you could implement these functions to abstract the differences
away from xfs_fsr and any other code that iterates the mount table:

struct mntent_cursor {
    /* variables needed to track iteration of the mtab */
};

platform_first_mntent()
platform_next_mntent()
platform_end_mntent()

and so the code would look like:

    struct mntent_cursor  cursor;

    mntent = platform_first_mntent(&cursor)

    do {
        /* process mntent */
    } while (mntent = platform_next_mntent(&cursor, mntent));

    platform_end_mntent(&cursor);

This completely abstracts the differences related to the the mount
table traversal, and allows the aplication level code to be written
in a clean, easily maintainable fashion...

âSo something like what I just posted? I added the same code that works for Linux into other platforms other than OS X. â

Â

âBecause as it was there before, Âit either works on them, âor no one care. And I really don't have an idea how to test it on Irix... :-)

Cheers,

Jan

--

Jan Tulak

jtulak@xxxxxxxxxxÂ/ jan@xxxxxxxx