lkml.org 
[lkml]   [2008]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2008-04-01 13:35    [W:0.226 / U:0.152 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site