Re: [PATCH 5/5] xfs_io: set exitcode on failure appropriately

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: Eric Sandeen <sandeen@sandeen.net>, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 5/5] xfs_io: set exitcode on failure appropriately
Date: Tue, 24 Mar 2020 16:24:01 -0700	[thread overview]
Message-ID: <20200324232401.GT29339@magnolia> (raw)
In-Reply-To: <20200324231255.GZ10776@dread.disaster.area>

On Wed, Mar 25, 2020 at 10:12:55AM +1100, Dave Chinner wrote:
> On Tue, Mar 24, 2020 at 03:57:26PM -0500, Eric Sandeen wrote:
> > On 3/23/20 7:19 PM, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Many operations don't set the exitcode when they fail, resulting
> > > in xfs_io exiting with a zero (no failure) exit code despite the
> > > command failing and returning an error. The command return code is
> > > really a boolean to tell the libxcmd command loop whether to
> > > continue processing or not, while exitcode is the actual xfs_io exit
> > > code returned to the parent on exit.
> > > 
> > > This patchset just makes the code do the right thing. It's not the
> > > nicest code, but it's a start at producing correct behaviour.
> > > 
> > > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > 
> > I wonder if there somewhere we could formally document these conventions...
> > 
> > Like maybe at least near the "exitcode" global declaration?
> 
> I really think we need to rework the way we do the error handling
> in the command line parsing for these utilities. One of the things I
> found in doing this is that most of the code does return error codes
> to the main function, only then to drop it on the floor and turn it
> into "exitcode = 1; return 0;" pair.
> 
> So I'm pondering how to make this much simpler - returning error
> codes from the command functions would be a much better idea,
> then have a command flag to indicate whether we continue on error or
> terminate.
> 
> That moves all the exit code handling out of the commands and
> provides consistent error handling for all commands and
> infrastructure - 0 = success, failure returns negative errno - and
> so should enable much more reliable and consistent error handling
> across all the utilities....

It seems reasonable to me, though I wonder how fstests will react to
that.  Then again, a lot of xfs_io error handling seems to be done via
grep so maybe it wouldn't be that bad. :)

--D

> Thoughts?
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com