Re: [PATCH] xfsprogs: fix geometry calls on older kernels for 5.2.1

[Dave Chinner <david@fromorbit.com>]

On Tue, Aug 20, 2019 at 02:18:28PM -0700, Darrick J. Wong wrote:
> On Tue, Aug 20, 2019 at 03:47:29PM -0500, Eric Sandeen wrote:
> > I didn't think 5.2.0 through; the udpate of the geometry ioctl means
> > that the tools won't work on older kernels that don't support the
> > v5 ioctls, since I failed to merge Darrick's wrappers.
> > 
> > As a very quick one-off I'd like to merge this to just revert every
> > geometry call back to the original ioctl, so it keeps working on
> > older kernels and I'll release 5.2.1.  This hack can go away when
> > Darrick's wrappers get merged.
> > 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> For the four line code fix,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> > ---
> > 
> > I'm a little concerned that 3rd party existing code which worked fine
> > before will now get the new XFS_IOC_FSGEOMETRY definition if they get
> > rebuilt, and suddenly stop working on older kernels. Am I overreacting
> > or misunderstanding our compatibility goals?
> 
> As for this question ^^^ ... <URRRK>.
> 
> I thought the overall strategy was to get everything in xfsprogs using
> libfrog wrappers that would degrade gracefully on old kernels.

The wrappers were a necessary part of the conversion. They should
have been merged with the rest of XFS_IOC_FSGEOMETRY changes. How
did this get broken up?

> For xfsdump/restore, I think we should just merge it into xfsprogs and
> then it can use our wrappers.

Don't need to care about dump/restore:

$ git grep FSGEOM
common/fs.c:    if (ioctl(fd, XFS_IOC_FSGEOMETRY_V1, &geo)) {
doc/CHANGES:      XFS_IOC_FSGEOMETRY instead of XFS_IOC_GETFSUUID ioctl, so
$

It only uses teh V1 ioctl.

As it is, the correct thing to do here is to put the fallback into
the xfsctl() function. This is actually an exported and documented
interface to use xfs ioctls by external problems - it's part of
libhandle(), and that should be obvious by the fact the man page
that describes all this is xfsctl(3).

i.e. any app using XFS ioctls should be using the xfsctl()
interface, not calling ioctl directly. The whole reason for that it
because it allows us to handle things like this in application
independent code....
 
So I'd suggest that the fallback code should be in the xfsctl
handler and then userspace will pick this up and won't care about
which kernel it is running on...

I suspect the bigger picture is to convert all the open ioctl()
calls in xfsprogs for XFS specific ioctls to xfsctl(). We've kinda
screwed this pooch since we stopped having to support multiple
platforms.

> For everything else... I thought the story was that you shouldn't really
> be using xfs ioctls unless you're keeping up with upstream.

Or you should be linked against libhandle and using xfsctl() to
be isolated from these sorts of things.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com