On Thu, Jun 12, 2014 at 04:01:41PM -0400, Brian Foster wrote: > On Wed, Jun 11, 2014 at 07:10:20PM +1000, Dave Chinner wrote: > > On Wed, Jun 11, 2014 at 07:48:50AM +1000, Dave Chinner wrote: > > > On Tue, Jun 10, 2014 at 07:58:57AM -0400, Brian Foster wrote: > > > > On Tue, Jun 10, 2014 at 08:33:20AM +1000, Dave Chinner wrote: > > > > > The issue is the negative error number patchset, and how to handle > > > > > review and testing. The patchset is already 62 patches long and it > > > > > converts roughly half the code base. It'll take me another couple of > > > > > days to convert the rest of the code, and that will probably take > > > > > another 60 patches. > > > > > > > > > > I understand that reviewing 100+ patches is going to be a pain, but > > > > > each patch currently averages about +/- 10 lines. The current > > > > > diffstat is: > > > > > > > > > > 37 files changed, 723 insertions(+), 722 deletions(-) > > > > > > > > > > And that will probably double, so it's still going to be a fair > > > > > amount of change. > > > > > > > > Is there any sort of more coarse logical breakdown of this series, or do > > > > we want/need to convert the entire codebase all at once? The individual > > > > patches sound relatively small... is there a particular method at play > > > > there? E.g., a patch per function? file? call chain? > > > > > > I'm doing it layer by layer, starting from the linux interface > > > layers and working my way down. e.g. fs/xfs/xfs_file.c first, > > > the fs/xfs/xfs_iops.c, and so on, and there are multiple patches per > > > file for each (roughly) logical change. e.g. converting xfs_iops.c: > > [...] > > > > I've decided that there really is too much unnecessary code churn > > from this approach. I end up converting all the call sites to negate > > the error sign, and then end up converting them back to the original > > code some time later, leaving only the source of the errors with a > > changed sign. > > > > So, I stopped doing that to see just what the brute force, change > > source and conversions only, and I found with a few simple searches > > I could identify all the locations that need changing. So, in a > > couple of hours I churned out the patch that converted everything. > > Still pretty large, even though it only changes error values and > > conversion points. > > > > 67 files changed, 879 insertions(+), 884 deletions(-) > > > > Not sure how I could break this up - it really is an all-or-nothing > > patch this Big Hammer approach.... > > > > Yeah, now that I look at it, it's kind of hard to review as any other > way as well. I've done some grepping and made a pass through all of the > changes. I noticed some very minor things like not all of the comments > being converted and some multi-line parameter lists going out of > alignment, but I didn't bother to even make notes of those. > > I've gone through an xfstests run without any explosions as well. > > A couple general observations: > > - I assume the xfs_buf->b_error type change is intentional..? yes - it was an unsigned short, which is incompaitble with negative integer values, and there is a 2 byte hole in the xfs_buf structure after it, anyway.... > - Same for the change in value for the ASSERT(error <=0 && error >= > -1000) assert in xfs_buf_ioerror (previously it used 64k). Right - it was checking to see if it fit in an unsigned short, while now it checks for the valid "negative errno" range the kernel uses. > ... and I saw a few spots that looked like could still need conversion. No surprises there... > A diff is inlined below. Yup, I missed a couple. I'll fold them in to the patch. Thanks! Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
