On Thu, Oct 29, 2015 at 08:13:34AM -0400, Brian Foster wrote: > On Thu, Oct 29, 2015 at 09:32:34AM +1100, Dave Chinner wrote: > > On Wed, Oct 28, 2015 at 07:51:22AM -0400, Brian Foster wrote: > > > On Fri, Oct 16, 2015 at 12:44:54PM +1100, Dave Chinner wrote: > > > > From: Dave Chinner <dchinner@xxxxxxxxxx> > > > > > > > > ASSERT comes from the xfs/xfs.h include, and we don't ever define > > > > DEBUG so we never get asserts built in. We want asserts built in for > > > > testing, but not for distro packages. The debian package already > > > > tries to do this by using "export DEBUG=-DNDEBUG" for the build > > > > context, but seeing as we pull in #define ASSERT(ex) (0) from the > > > > XFS headers it's a no-op. > > > > > > > > Convert all the ASSERT calls to assert to remove this conflict with > > > > the xfsprogs headers and so local developer builds are built with > > > > asserts enabled. > > > > > > > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx> > > > > --- > > > > > > My initial concern when reading this was that asserts were now > > > unconditionally enabled. According to the man page, assert() is enabled > > > unless NDEBUG is defined at include time. The debian builder apparently > > > does this, but is this standard for other such utils? > > > > For other XFS utilities? Yes. For other packages I don't maintain? I > > don't know, I don't care, and I don't think it's relevant at all. > > How the distro builds and packages a utility is up to the distro > > maintainers - if they define -NDEBUG, then there is absolutely no > > change in behaviour of xfsdump. I think the binary is effectively > > unchanged as assert() is defined out in that case... > > > > I'm talking about common packaging tools (e.g., yum/dnf being the > obvious next example) and XFS packages, not random other packages. If > they don't define NDEBUG, then apparently there is a change in behavior. If they don't define that, then they've been building the packages incorrectly... ;) > > > If the > > > problem is that asserts cannot be enabled, I'm wondering why the > > > appropriate fix isn't to define DEBUG somewhere for debug-enabled builds > > > rather than unconditionally convert all of the ASSERT() calls into > > > actual assert()'s..? > > > > Becuase it has to build with both old and new xfsprogs userspace > > headers. Hence we cannot use ASSERT reliably in xfsdump at all, > > because it's owned by the old xfsprogs headers and it's behaviour is > > determined by whatever that xfsprogs installation defined...... > > > > Then why not try to undef/redef in xfsdump or just rename the #define > that's used? I don't care too much either way, I just don't follow why > there's a need to change behavior at all to fix a naming conflict. > > Are we saying that ASSERT() probably shouldn't exist in userspace (incl. > xfsprogs) and we should always use the generic assert() mechanism? Or > are we saying ASSERT() can exist in userspace, but it's purely a libxfs > thing and should not be exported beyond that (e.g., libxfs can use > ASSERT(), actual userspace tools like repair, etc. should eventually use > assert())..? Precisely this. ASSERT is used in the kernel side libxfs code, and so only exists in userspace to support the libxfs code in userspace. We don't want the libxfs code in userspace aborting on corrupt structures or invalid situations, because we need to handle/parse/repair such brokenness. IOWS, the ASSERTs in libxfs/ in userspace need to be turned off for userspace to work correctly. That's what the DEBUG define in libxfs does. That leads to the xfsprogs userspace still needing an assert facility - that comes from using assert() and NDEBUG to turn it off and on. Essentially, ASSERT is internal to libxfs and should not be exported anywhere. assert() should used outside libxfs in xfsprogs and other XFS code bases. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
