Messages in this thread | | | Date | Tue, 1 Apr 2008 14:33:37 +0300 | From | "Pekka Enberg" <> | Subject | Re: [RFC PATCH 26/26] UBIFS: include FS to compilation |
| |
Hi Artem,
Pekka Enberg wrote: > > First and foremost, JFFS2 uses BUG_ON and doesn't invent it's own > > assert.
On Tue, Apr 1, 2008 at 1:26 PM, Artem Bityutskiy <Artem.Bityutskiy@nokia.com> wrote: > True. But it has checking code which may be enabled or disable. > An assert is just a special case of this. You do not say why > it hurts. For me it looks like your personal taste.
I don't know how many times I have to say this: you're doing it at the wrong level! The reason you want to compile them out is because you've added crap like this all over your code paths:
ubifs_assert(PageLocked(page)); ubifs_assert(!PageChecked(page)); ubifs_assert(!PagePrivate(page));
So instead of arguing about this you really ought to look at what SLUB, for example, does. It's perfectly okay to have _debugging checks_ compiled out (stuff like verify_inode and such) but at the assertion level it makes no sense whatsoever!
Pekka Enberg wrote: > > But perhaps the problem will go away after you inject some sanity to > > stuff like this: > > > > fs/ubifs/dir.c: dbg_gen("dent '%.*s' to ino %lu (nlink %d) in dir ino %lu", > > fs/ubifs/dir.c: dbg_gen("dent '%.*s' from ino %lu (nlink %d) in dir ino %lu", > > fs/ubifs/dir.c: dbg_gen("directory '%.*s', ino %lu in dir ino %lu", > > dentry->d_name.len, > > fs/ubifs/dir.c: dbg_gen("dent '%.*s', mode %#x in dir ino %lu", > > fs/ubifs/dir.c: dbg_gen("dent '%.*s' in dir ino %lu",
On Tue, Apr 1, 2008 at 1:26 PM, Artem Bityutskiy <Artem.Bityutskiy@nokia.com> wrote: > This means that when debugging is enabled, you'll have prints like: > UBIFS DBG (pid 28398): ubifs_create: dent 'file', mode 0x81a4 in dir ino 1 > or > UBIFS DBG (pid 28398): ubifs_setattr: ino 65, ia_valid 0x70
So what? It's still an ad hoc debugging printout with no particular meaning whatsoever.
But this discussion is getting nowhere and I have better things to do than argue about this over and over again. So to reiterate my review comments on this:
- Kill your home-grown assert - Fix up your logging messages to actually make sense - Perhaps introduce a ubifs_error() thingy and convert as much code to use that - Reduce the amount of debug Kconfig options
You can ignore these comments as my personal preferences all you want in which case I can only wish you good luck with merging this thing upstream.
Pekka
| |