linux-kernel - Re: [RFC PATCH 26/26] UBIFS: include FS to compilation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <84144f020804010433xafc6f28o5a5375ce82903a7a@mail.gmail.com>
Date:	Tue, 1 Apr 2008 14:33:37 +0300
From:	"Pekka Enberg" <penberg@...helsinki.fi>
To:	Artem.Bityutskiy@...ia.com
Cc:	"Artem Bityutskiy" <dedekind@...dex.ru>,
	LKML <linux-kernel@...r.kernel.org>,
	"Adrian Hunter" <ext-adrian.hunter@...ia.com>,
	"David Woodhouse" <dwmw2@...radead.org>
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@...ia.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@...ia.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/