Re: [PATCH] aufs: Do not refer to AUFS_NAME in pr_fmt

[sfjro@users.sourceforge.net]

Thorsten Glaser:
> >It introduces a new separated file include/linux/aufs_name.h.
>
> Isn=E2=80=99t that a bit overkill?

Hmm, I may have to agree with that.
Honestly speaking, I don't like this approach.
But embedding (expanding) AUFS_NAME is worse for me.


> >If the Makefile refers to the macro, perhaps the Makefile should
> >define it, and pass it with -D?
>
> Indeed. I like Ben=E2=80=99s patch better. But if it must be a separate
> file, please move the pr_fmt definition out of the Makefile and
> into that file, too. Code doesn=E2=80=99t belong into a Makefile IMHO.

You guys don't see "-imacros linux/aufs_name.h" in Makefile?
Or do I totaly miunderstand -imacros?

----------------------------------------------------------------------
> Ben Hutchings dixit:
>
> >-ccflags-y +=3D -D'pr_fmt(fmt)=3DAUFS_NAME"\040%s:%d:%s[%d]:\040"fmt,__fun=
> c__,__LINE__,current->comm,current->pid'
> >+ccflags-y +=3D -D'pr_fmt(fmt)=3D"aufs\040%s:%d:%s[%d]:\040"fmt,__func__,_=
> _LINE__,current->comm,current->pid'
>
> Sadly, this doesn=E2=80=99t work either:
	:::
> /tmp/buildd/linux-2.6-3.2~rc7/debian/build/source_m68k_none/arch/m68k/inclu=
> de/asm/hardirq.h:23:2: error: dereferencing pointer to incomplete type
	:::
> The difference between a static inline function and a pr=C3=A6processor
> macro makes the difference. I know, now, why I never used the former
> myself =E2=98=BA I guess =E2=80=9Ccurrent=E2=80=9D is not defined there.

That must be an error around the "current" macro which I was afraid.

----------------------------------------------------------------------

> What do you think of this (untested)? I omitted the Documentation/**
> patch as that part it not present in the Debian Linux kernel.
	:::
+#define AUFS_NAME		"aufs"
+#define pr_fmt(fmt)		AUFS_NAME"\040%s:%d:%s[%d]:\040"fmt,__func__,__LINE__,current->comm,current->pid

Here are the comments from me.
- aufs_name.h is used both in kernel-space and user-space, so we need
  "#ifdef __KERNEL__"
- the macro may already be defined, so we need "#ifndef pr_fmt"
  now we should decide which we force (choose), the pr_fmt macro we
  define in aufs or the one previously defined somewhere else.
And I chose "defined aufs aufs".
These are also reasons to put it in Makefile.

----------------------------------------------------------------------

> well, it compiles (with warnings, see below). I=E2=80=99ll know if it links
> and loads tomorrow, I guess (unless the other problems are still
> there).
>
> /tmp/buildd/linux-2.6-3.2~rc7/debian/build/source_m68k_none/include/linux/a=
> ufs_name.h:27:0: warning: "pr_fmt" redefined [enabled by default]
> /tmp/buildd/linux-2.6-3.2~rc7/debian/build/source_m68k_none/include/linux/p=
> rintk.h:152:0: note: this is the location of the previous definition
>
> Version 2 of this patch (attached) fixes this by #undef pr_fmt
> before defining it. Please decide whether you want to fix it

Yes, undefining will be necessary if we move the macro difinition into
the header file. Or we may add a condition "#ifndef pr_fmt" when we
define the macro in the header. In other words,
- adding a condition means giving precedence to the difinition in
  somewhere else.
- undefining means forcing the definition in aufs.
But this "forcing" is not enough since the macro can be defined _before_
including the header. I know you put "-include linux/aufs_name.h" in
Makefile, but it is not enough either since the macro _may_ be defined
in sched.h or somewhere else which will be included before aufs_name.h.
Also I understand it is not a big problem. But I hope you would agree
that unconditionally or blindly defined macro is effective (or less
subject to bug) in a single module.

So I still think it is better to define it in Makefile.
If I remove refering the "current" macro in the definition, then the
life will be easier, but it is still useful and I want to keep
it. Additonally it is not a essential problem I think.
Finally I'd like to add sched.h between aufs_name and pr_fmt (see the
attached patch).
How do you think?


J. R. Okajima

--- a/fs/aufs/Makefile
+++ b/fs/aufs/Makefile
@@ -10,6 +10,7 @@ endif
 ccflags-y += -DDEBUG
 # sparse doesn't allow spaces
 ccflags-y += -imacros linux/aufs_name.h
+ccflags-y += -include linux/sched.h
 ccflags-y += -D'pr_fmt(fmt)=AUFS_NAME"\040%s:%d:%s[%d]:\040"fmt,__func__,__LINE__,current->comm,current->pid'
 
 obj-$(CONFIG_AUFS_FS) += aufs.o