linux-kernel - Re: [PATCH 1/2] hfs/hfsplus: Convert dprint to hfs

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: <1365987181.4850.YahooMailClassic@web172306.mail.ir2.yahoo.com>
Date:	Mon, 15 Apr 2013 01:53:01 +0100 (BST)
From:	Hin-Tak Leung <htl10@...rs.sourceforge.net>
To:	Vyacheslav Dubeyko <slava@...eyko.com>,
	Joe Perches <joe@...ches.com>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] hfs/hfsplus: Convert dprint to hfs_dbg

--- On Mon, 8/4/13, Joe Perches <joe@...ches.com> wrote:

> Use a more current logging style.
> 
> Rename macro and uses.
> Add do {} while (0) to macro.
> Add DBG_ to macro.
> Add and use hfs_dbg_cont variant where appropriate.
> 
> Signed-off-by: Joe Perches <joe@...ches.com>

<a lot of dprint to hfs_dbg changes snipped>

> +++ b/fs/hfs/hfs_fs.h
> @@ -34,8 +34,18 @@
>  //#define DBG_MASK   
> (DBG_CAT_MOD|DBG_BNODE_REFS|DBG_INODE|DBG_EXTENT)
>  #define DBG_MASK    (0)
>  
> -#define dprint(flg, fmt, args...) \
> -    if (flg & DBG_MASK) printk(fmt , ##
> args)
> +#define hfs_dbg(flg, fmt, ...)   
>             \
> +do {       
>            
>         \
> +    if (DBG_##flg &
> DBG_MASK)       
>     \
> +        printk(KERN_DEBUG
> fmt, ##__VA_ARGS__);    \
> +} while (0)
> +
> +#define hfs_dbg_cont(flg, fmt, ...)   
>         \
> +do {       
>            
>         \
> +    if (DBG_##flg &
> DBG_MASK)       
>     \
> +        printk(KERN_CONT fmt,
> ##__VA_ARGS__);    \
> +} while (0)
> +
>  
>  /*
>   * struct hfs_inode_info

<a lot of dprint to hfs_dbg change snipped>

This set of change seems to be somewhat zealous - it doesn't offer any benefits other than possibly satisfying somebody's idea of code-purity.

FWIW, I have been sitting on a patch which changes this part of the code to dynamic debugging, and it is much simplier. Just:

=============================
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index e298b83..55d211d 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -45,8 +25,7 @@
 #define HFSPLUS_JOURNAL_SWAP           1

 #define dprint(flg, fmt, args...) \
-       if (flg & DBG_MASK) \
-               printk(fmt , ## args)
+               pr_debug(fmt , ## args)

 /* Runtime config options */
 #define HFSPLUS_DEF_CR_TYPE    0x3F3F3F3F  /* '????' */
=========================

(and you can then remove all the DBG_* defines before that, since they then don't have any effect any more).

The benefit of this alternative is that it does not break any out-of-tree patches, while make it easier to debug say patches... and I am still sitting on a rather substantial set of the journal change, plus all the other issues that come out of it, like the folder count patch for case-sensitive file systems.

I think one needs to think very carefully about make bulk changes like this, which serves no real purpose other than satisfying somebody's idea of code purity.

The problem with such bulk "stylistic" changes, is that it forces people who are working on real functionalities and bug fixes to rebase their work, and spend time on doing so, and also at the risk introducing new bugs while rebasing. I know I am writing on a somewhat selfish purpose: if I need to rebase my work due to other's bug fixes or enhancement, etc, then fair enough, but I'd prefer not to rebase for the purpose of other's preference of, and attempts at re-arranging the style of the debug statements, when the debugging output means little to them.

--
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/