Hi Artem,

Pekka Enberg wrote:
quoted text
> > 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:
quoted text
>  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:
quoted text
> > 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:
quoted text
> 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
--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/