Re: [PATCH 04/13] autofs4 - change printks AUTOFS defined prints

2014-11-05 Thread Ian Kent 
On Wed, 2014-11-05 at 15:20 -0800, Joe Perches wrote:
> 
> > But idea of using pr_xxx() and pr_fmt() (actually that's too open to
> > name clashes so it would need to be named something like autofs_pr_fmt()
> > anyway) looks like it results in less readable code so I'd really prefer
> > not to do that.
> 
> Using pr_info/pr_debug (or any other pr_) is a
> generic mechanism in the kernel.  Adding a
> #define pr_fmt is also generic thing that works with
> all the pr_ uses in a specific compilation unit.

LOL, so you recommend I look more closely, I will.
In the meantime I'll send the other patches to Andrew.

Ian

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 04/13] autofs4 - change printks AUTOFS defined prints

2014-11-05 Thread Joe Perches 
On Thu, 2014-11-06 at 07:02 +0800, Ian Kent wrote:
> On Mon, 2014-11-03 at 06:33 -0800, Joe Perches wrote:
> > 
> > That's fine.  I left out the trailing semicolon/space.
> > The pr_fmt could be something like:
> > #define pr_fmt(fmt) KBUILD_MODNAME ":%d:%s: " fmt, current->pid, __func__
> > or add a "pid:" descriptor prefix if you like too:
> > #define pr_fmt(fmt) KBUILD_MODNAME ":pid:%d:%s: " fmt, current->pid, 
> > __func__
> > 
> > > > it's better to use a consistent style for
> > > > these logging functions ideally with terminating
> > > > newlines so there isn't a mix of code with
> > > > and without those newlines.  That inconsistency
> > > > leads to unintended defects.
> > > 
> > > The idea here was to make the logging consistent throughout.
> > 
> > Mine too.
> > 
> > > I have become used of not using the new-line terminator in logging over
> > > the years and tend to favour that myself. You recommend not doing that
> > > probably from a kernel wide consistency perspective? Maybe that is
> > > better ...
> > 
> > Yes, kernel style consistency is the rationale.
> > 
> > Over time, people come along and add messages
> > while not reading the code very closely so using
> > the kernel style with newlines can help avoid
> > these trivial defects.
> 
> I can see how not including the trailing newline in the macros is a good
> thing and I'll forward a couple more patches to Andrew for this and fix
> the inconsistencies.

OK, great.

> But idea of using pr_xxx() and pr_fmt() (actually that's too open to
> name clashes so it would need to be named something like autofs_pr_fmt()
> anyway) looks like it results in less readable code so I'd really prefer
> not to do that.

Using pr_info/pr_debug (or any other pr_) is a
generic mechanism in the kernel.  Adding a
#define pr_fmt is also generic thing that works with
all the pr_ uses in a specific compilation unit.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 04/13] autofs4 - change printks AUTOFS defined prints

2014-11-05 Thread Ian Kent 
On Mon, 2014-11-03 at 06:33 -0800, Joe Perches wrote:
> 
> That's fine.  I left out the trailing semicolon/space.
> The pr_fmt could be something like:
> #define pr_fmt(fmt) KBUILD_MODNAME ":%d:%s: " fmt, current->pid, __func__
> or add a "pid:" descriptor prefix if you like too:
> #define pr_fmt(fmt) KBUILD_MODNAME ":pid:%d:%s: " fmt, current->pid, __func__
> 
> > > it's better to use a consistent style for
> > > these logging functions ideally with terminating
> > > newlines so there isn't a mix of code with
> > > and without those newlines.  That inconsistency
> > > leads to unintended defects.
> > 
> > The idea here was to make the logging consistent throughout.
> 
> Mine too.
> 
> > I have become used of not using the new-line terminator in logging over
> > the years and tend to favour that myself. You recommend not doing that
> > probably from a kernel wide consistency perspective? Maybe that is
> > better ...
> 
> Yes, kernel style consistency is the rationale.
> 
> Over time, people come along and add messages
> while not reading the code very closely so using
> the kernel style with newlines can help avoid
> these trivial defects.

I can see how not including the trailing newline in the macros is a good
thing and I'll forward a couple more patches to Andrew for this and fix
the inconsistencies.

But idea of using pr_xxx() and pr_fmt() (actually that's too open to
name clashes so it would need to be named something like autofs_pr_fmt()
anyway) looks like it results in less readable code so I'd really prefer
not to do that.

But thanks for pointing out the problems with the logging calls and for
you constructive suggestions.

Ian

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 04/13] autofs4 - change printks AUTOFS defined prints

2014-11-05 Thread Ian Kent 
On Mon, 2014-11-03 at 06:33 -0800, Joe Perches wrote:
 
 That's fine.  I left out the trailing semicolon/space.
 The pr_fmt could be something like:
 #define pr_fmt(fmt) KBUILD_MODNAME :%d:%s:  fmt, current-pid, __func__
 or add a pid: descriptor prefix if you like too:
 #define pr_fmt(fmt) KBUILD_MODNAME :pid:%d:%s:  fmt, current-pid, __func__
 
   it's better to use a consistent style for
   these logging functions ideally with terminating
   newlines so there isn't a mix of code with
   and without those newlines.  That inconsistency
   leads to unintended defects.
  
  The idea here was to make the logging consistent throughout.
 
 Mine too.
 
  I have become used of not using the new-line terminator in logging over
  the years and tend to favour that myself. You recommend not doing that
  probably from a kernel wide consistency perspective? Maybe that is
  better ...
 
 Yes, kernel style consistency is the rationale.
 
 Over time, people come along and add messages
 while not reading the code very closely so using
 the kernel style with newlines can help avoid
 these trivial defects.

I can see how not including the trailing newline in the macros is a good
thing and I'll forward a couple more patches to Andrew for this and fix
the inconsistencies.

But idea of using pr_xxx() and pr_fmt() (actually that's too open to
name clashes so it would need to be named something like autofs_pr_fmt()
anyway) looks like it results in less readable code so I'd really prefer
not to do that.

But thanks for pointing out the problems with the logging calls and for
you constructive suggestions.

Ian

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 04/13] autofs4 - change printks AUTOFS defined prints

2014-11-05 Thread Joe Perches 
On Thu, 2014-11-06 at 07:02 +0800, Ian Kent wrote:
 On Mon, 2014-11-03 at 06:33 -0800, Joe Perches wrote:
  
  That's fine.  I left out the trailing semicolon/space.
  The pr_fmt could be something like:
  #define pr_fmt(fmt) KBUILD_MODNAME :%d:%s:  fmt, current-pid, __func__
  or add a pid: descriptor prefix if you like too:
  #define pr_fmt(fmt) KBUILD_MODNAME :pid:%d:%s:  fmt, current-pid, 
  __func__
  
it's better to use a consistent style for
these logging functions ideally with terminating
newlines so there isn't a mix of code with
and without those newlines.  That inconsistency
leads to unintended defects.
   
   The idea here was to make the logging consistent throughout.
  
  Mine too.
  
   I have become used of not using the new-line terminator in logging over
   the years and tend to favour that myself. You recommend not doing that
   probably from a kernel wide consistency perspective? Maybe that is
   better ...
  
  Yes, kernel style consistency is the rationale.
  
  Over time, people come along and add messages
  while not reading the code very closely so using
  the kernel style with newlines can help avoid
  these trivial defects.
 
 I can see how not including the trailing newline in the macros is a good
 thing and I'll forward a couple more patches to Andrew for this and fix
 the inconsistencies.

OK, great.

 But idea of using pr_xxx() and pr_fmt() (actually that's too open to
 name clashes so it would need to be named something like autofs_pr_fmt()
 anyway) looks like it results in less readable code so I'd really prefer
 not to do that.

Using pr_info/pr_debug (or any other pr_level) is a
generic mechanism in the kernel.  Adding a
#define pr_fmt is also generic thing that works with
all the pr_level uses in a specific compilation unit.

cheers, Joe

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 04/13] autofs4 - change printks AUTOFS defined prints

2014-11-05 Thread Ian Kent 
On Wed, 2014-11-05 at 15:20 -0800, Joe Perches wrote:
 
  But idea of using pr_xxx() and pr_fmt() (actually that's too open to
  name clashes so it would need to be named something like autofs_pr_fmt()
  anyway) looks like it results in less readable code so I'd really prefer
  not to do that.
 
 Using pr_info/pr_debug (or any other pr_level) is a
 generic mechanism in the kernel.  Adding a
 #define pr_fmt is also generic thing that works with
 all the pr_level uses in a specific compilation unit.

LOL, so you recommend I look more closely, I will.
In the meantime I'll send the other patches to Andrew.

Ian

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 04/13] autofs4 - change printks AUTOFS defined prints

2014-11-03 Thread Joe Perches 
On Mon, 2014-11-03 at 17:20 +0800, Ian Kent wrote:
> On Mon, 2014-11-03 at 00:25 -0800, Joe Perches wrote:
> > On Mon, 2014-11-03 at 16:12 +0800, Ian Kent wrote:
> > > Use the AUTOFS_*() print defines instead of raw printks.

Hi again Ian

> > It's probably better to simply use
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > or
> > #define pr_fmt(fmt) KBUILD_MODNAME ":%d:%s" fmt, current->pid, __func__
> > if you _really_ want pid/func in the output
> 
> I do, yes.

That's fine.  I left out the trailing semicolon/space.
The pr_fmt could be something like:
#define pr_fmt(fmt) KBUILD_MODNAME ":%d:%s: " fmt, current->pid, __func__
or add a "pid:" descriptor prefix if you like too:
#define pr_fmt(fmt) KBUILD_MODNAME ":pid:%d:%s: " fmt, current->pid, __func__

> > it's better to use a consistent style for
> > these logging functions ideally with terminating
> > newlines so there isn't a mix of code with
> > and without those newlines.  That inconsistency
> > leads to unintended defects.
> 
> The idea here was to make the logging consistent throughout.

Mine too.

> I have become used of not using the new-line terminator in logging over
> the years and tend to favour that myself. You recommend not doing that
> probably from a kernel wide consistency perspective? Maybe that is
> better ...

Yes, kernel style consistency is the rationale.

Over time, people come along and add messages
while not reading the code very closely so using
the kernel style with newlines can help avoid
these trivial defects.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 04/13] autofs4 - change printks AUTOFS defined prints

2014-11-03 Thread Ian Kent 
On Mon, 2014-11-03 at 00:25 -0800, Joe Perches wrote:
> On Mon, 2014-11-03 at 16:12 +0800, Ian Kent wrote:
> > Use the AUTOFS_*() print defines instead of raw printks.
> 
> Please check the output of these conversions.
> 
> For instance:
> 
> > diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> []
> > @@ -162,7 +162,7 @@ static void autofs4_notify_daemon(struct autofs_sb_info 
> > *sbi,
> > break;
> > }
> > default:
> > -   printk("autofs4_notify_daemon: bad type %d!\n", type);
> > +   AUTOFS_WARN("autofs4_notify_daemon: bad type %d!", type);
> > mutex_unlock(>wq_mutex);
> > return;
> > }
> 
> The current #define is:
> 
> #define AUTOFS_WARN(fmt, ...)  \
>   pr_warn("pid %d: %s: " fmt "\n",\
>   current->pid, __func__, ##__VA_ARGS__)
>  
> So this duplicates the function name in the output.

Ahh, yes, I'll fix that.

> 
> It's probably better to simply use
>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> or
>   #define pr_fmt(fmt) KBUILD_MODNAME ":%d:%s" fmt, current->pid, __func__
> if you _really_ want pid/func in the output

I do, yes.

> 
> and just use pr_ instead of AUTOFS_ macros.
> 
> And it's better to use a consistent style for
> these logging functions ideally with terminating
> newlines so there isn't a mix of code with
> and without those newlines.  That inconsistency
> leads to unintended defects.

The idea here was to make the logging consistent throughout.

I have become used of not using the new-line terminator in logging over
the years and tend to favour that myself. You recommend not doing that
probably from a kernel wide consistency perspective? Maybe that is
better ...

Ian

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 04/13] autofs4 - change printks AUTOFS defined prints

2014-11-03 Thread Joe Perches 
On Mon, 2014-11-03 at 16:12 +0800, Ian Kent wrote:
> Use the AUTOFS_*() print defines instead of raw printks.

Please check the output of these conversions.

For instance:

> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
[]
> @@ -162,7 +162,7 @@ static void autofs4_notify_daemon(struct autofs_sb_info 
> *sbi,
>   break;
>   }
>   default:
> - printk("autofs4_notify_daemon: bad type %d!\n", type);
> + AUTOFS_WARN("autofs4_notify_daemon: bad type %d!", type);
>   mutex_unlock(>wq_mutex);
>   return;
>   }

The current #define is:

#define AUTOFS_WARN(fmt, ...)  \
pr_warn("pid %d: %s: " fmt "\n",\
current->pid, __func__, ##__VA_ARGS__)
 
So this duplicates the function name in the output.

It's probably better to simply use
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
or
#define pr_fmt(fmt) KBUILD_MODNAME ":%d:%s" fmt, current->pid, __func__
if you _really_ want pid/func in the output

and just use pr_ instead of AUTOFS_ macros.

And it's better to use a consistent style for
these logging functions ideally with terminating
newlines so there isn't a mix of code with
and without those newlines.  That inconsistency
leads to unintended defects.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 04/13] autofs4 - change printks AUTOFS defined prints

2014-11-03 Thread Ian Kent 
Use the AUTOFS_*() print defines instead of raw printks.

Signed-off-by: Ian Kent 
---
 fs/autofs4/inode.c |   13 +++--
 fs/autofs4/waitq.c |2 +-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index 80389af..7849591 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -266,14 +266,14 @@ int autofs4_fill_super(struct super_block *s, void *data, 
int silent)
if (parse_options(data, , _inode->i_uid, _inode->i_gid,
  , _set, >type, >min_proto,
  >max_proto)) {
-   printk("autofs: called with bogus options\n");
+   AUTOFS_ERROR("autofs: called with bogus options");
goto fail_dput;
}
 
if (pgrp_set) {
sbi->oz_pgrp = find_get_pid(pgrp);
if (!sbi->oz_pgrp) {
-   pr_warn("autofs: could not find process group %d\n",
+   AUTOFS_ERROR("autofs: could not find process group %d",
pgrp);
goto fail_dput;
}
@@ -290,8 +290,8 @@ int autofs4_fill_super(struct super_block *s, void *data, 
int silent)
/* Couldn't this be tested earlier? */
if (sbi->max_proto < AUTOFS_MIN_PROTO_VERSION ||
sbi->min_proto > AUTOFS_MAX_PROTO_VERSION) {
-   printk("autofs: kernel does not match daemon version "
-  "daemon (%d, %d) kernel (%d, %d)\n",
+   AUTOFS_ERROR("autofs: kernel does not match daemon version "
+"daemon (%d, %d) kernel (%d, %d)",
sbi->min_proto, sbi->max_proto,
AUTOFS_MIN_PROTO_VERSION, AUTOFS_MAX_PROTO_VERSION);
goto fail_dput;
@@ -308,7 +308,7 @@ int autofs4_fill_super(struct super_block *s, void *data, 
int silent)
pipe = fget(pipefd);
 
if (!pipe) {
-   printk("autofs: could not open pipe file descriptor\n");
+   AUTOFS_ERROR("autofs: could not open pipe file descriptor");
goto fail_dput;
}
ret = autofs_prepare_pipe(pipe);
@@ -328,7 +328,8 @@ int autofs4_fill_super(struct super_block *s, void *data, 
int silent)
 * Failure ... clean up.
 */
 fail_fput:
-   printk("autofs: pipe file descriptor does not contain proper ops\n");
+   AUTOFS_ERROR(
+   "autofs: pipe file descriptor does not contain proper ops");
fput(pipe);
/* fall through */
 fail_dput:
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index b7f2deb..3a28d29 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -162,7 +162,7 @@ static void autofs4_notify_daemon(struct autofs_sb_info 
*sbi,
break;
}
default:
-   printk("autofs4_notify_daemon: bad type %d!\n", type);
+   AUTOFS_WARN("autofs4_notify_daemon: bad type %d!", type);
mutex_unlock(>wq_mutex);
return;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 04/13] autofs4 - change printks AUTOFS defined prints

2014-11-03 Thread Ian Kent 
Use the AUTOFS_*() print defines instead of raw printks.

Signed-off-by: Ian Kent ra...@themaw.net
---
 fs/autofs4/inode.c |   13 +++--
 fs/autofs4/waitq.c |2 +-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index 80389af..7849591 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -266,14 +266,14 @@ int autofs4_fill_super(struct super_block *s, void *data, 
int silent)
if (parse_options(data, pipefd, root_inode-i_uid, root_inode-i_gid,
  pgrp, pgrp_set, sbi-type, sbi-min_proto,
  sbi-max_proto)) {
-   printk(autofs: called with bogus options\n);
+   AUTOFS_ERROR(autofs: called with bogus options);
goto fail_dput;
}
 
if (pgrp_set) {
sbi-oz_pgrp = find_get_pid(pgrp);
if (!sbi-oz_pgrp) {
-   pr_warn(autofs: could not find process group %d\n,
+   AUTOFS_ERROR(autofs: could not find process group %d,
pgrp);
goto fail_dput;
}
@@ -290,8 +290,8 @@ int autofs4_fill_super(struct super_block *s, void *data, 
int silent)
/* Couldn't this be tested earlier? */
if (sbi-max_proto  AUTOFS_MIN_PROTO_VERSION ||
sbi-min_proto  AUTOFS_MAX_PROTO_VERSION) {
-   printk(autofs: kernel does not match daemon version 
-  daemon (%d, %d) kernel (%d, %d)\n,
+   AUTOFS_ERROR(autofs: kernel does not match daemon version 
+daemon (%d, %d) kernel (%d, %d),
sbi-min_proto, sbi-max_proto,
AUTOFS_MIN_PROTO_VERSION, AUTOFS_MAX_PROTO_VERSION);
goto fail_dput;
@@ -308,7 +308,7 @@ int autofs4_fill_super(struct super_block *s, void *data, 
int silent)
pipe = fget(pipefd);
 
if (!pipe) {
-   printk(autofs: could not open pipe file descriptor\n);
+   AUTOFS_ERROR(autofs: could not open pipe file descriptor);
goto fail_dput;
}
ret = autofs_prepare_pipe(pipe);
@@ -328,7 +328,8 @@ int autofs4_fill_super(struct super_block *s, void *data, 
int silent)
 * Failure ... clean up.
 */
 fail_fput:
-   printk(autofs: pipe file descriptor does not contain proper ops\n);
+   AUTOFS_ERROR(
+   autofs: pipe file descriptor does not contain proper ops);
fput(pipe);
/* fall through */
 fail_dput:
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index b7f2deb..3a28d29 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -162,7 +162,7 @@ static void autofs4_notify_daemon(struct autofs_sb_info 
*sbi,
break;
}
default:
-   printk(autofs4_notify_daemon: bad type %d!\n, type);
+   AUTOFS_WARN(autofs4_notify_daemon: bad type %d!, type);
mutex_unlock(sbi-wq_mutex);
return;
}

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 04/13] autofs4 - change printks AUTOFS defined prints

2014-11-03 Thread Joe Perches 
On Mon, 2014-11-03 at 16:12 +0800, Ian Kent wrote:
 Use the AUTOFS_*() print defines instead of raw printks.

Please check the output of these conversions.

For instance:

 diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
[]
 @@ -162,7 +162,7 @@ static void autofs4_notify_daemon(struct autofs_sb_info 
 *sbi,
   break;
   }
   default:
 - printk(autofs4_notify_daemon: bad type %d!\n, type);
 + AUTOFS_WARN(autofs4_notify_daemon: bad type %d!, type);
   mutex_unlock(sbi-wq_mutex);
   return;
   }

The current #define is:

#define AUTOFS_WARN(fmt, ...)  \
pr_warn(pid %d: %s:  fmt \n,\
current-pid, __func__, ##__VA_ARGS__)
 
So this duplicates the function name in the output.

It's probably better to simply use
#define pr_fmt(fmt) KBUILD_MODNAME :  fmt
or
#define pr_fmt(fmt) KBUILD_MODNAME :%d:%s fmt, current-pid, __func__
if you _really_ want pid/func in the output

and just use pr_level instead of AUTOFS_LEVEL macros.

And it's better to use a consistent style for
these logging functions ideally with terminating
newlines so there isn't a mix of code with
and without those newlines.  That inconsistency
leads to unintended defects.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 04/13] autofs4 - change printks AUTOFS defined prints

2014-11-03 Thread Ian Kent 
On Mon, 2014-11-03 at 00:25 -0800, Joe Perches wrote:
 On Mon, 2014-11-03 at 16:12 +0800, Ian Kent wrote:
  Use the AUTOFS_*() print defines instead of raw printks.
 
 Please check the output of these conversions.
 
 For instance:
 
  diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
 []
  @@ -162,7 +162,7 @@ static void autofs4_notify_daemon(struct autofs_sb_info 
  *sbi,
  break;
  }
  default:
  -   printk(autofs4_notify_daemon: bad type %d!\n, type);
  +   AUTOFS_WARN(autofs4_notify_daemon: bad type %d!, type);
  mutex_unlock(sbi-wq_mutex);
  return;
  }
 
 The current #define is:
 
 #define AUTOFS_WARN(fmt, ...)  \
   pr_warn(pid %d: %s:  fmt \n,\
   current-pid, __func__, ##__VA_ARGS__)
  
 So this duplicates the function name in the output.

Ahh, yes, I'll fix that.

 
 It's probably better to simply use
   #define pr_fmt(fmt) KBUILD_MODNAME :  fmt
 or
   #define pr_fmt(fmt) KBUILD_MODNAME :%d:%s fmt, current-pid, __func__
 if you _really_ want pid/func in the output

I do, yes.

 
 and just use pr_level instead of AUTOFS_LEVEL macros.
 
 And it's better to use a consistent style for
 these logging functions ideally with terminating
 newlines so there isn't a mix of code with
 and without those newlines.  That inconsistency
 leads to unintended defects.

The idea here was to make the logging consistent throughout.

I have become used of not using the new-line terminator in logging over
the years and tend to favour that myself. You recommend not doing that
probably from a kernel wide consistency perspective? Maybe that is
better ...

Ian

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 04/13] autofs4 - change printks AUTOFS defined prints

2014-11-03 Thread Joe Perches 
On Mon, 2014-11-03 at 17:20 +0800, Ian Kent wrote:
 On Mon, 2014-11-03 at 00:25 -0800, Joe Perches wrote:
  On Mon, 2014-11-03 at 16:12 +0800, Ian Kent wrote:
   Use the AUTOFS_*() print defines instead of raw printks.

Hi again Ian

  It's probably better to simply use
  #define pr_fmt(fmt) KBUILD_MODNAME :  fmt
  or
  #define pr_fmt(fmt) KBUILD_MODNAME :%d:%s fmt, current-pid, __func__
  if you _really_ want pid/func in the output
 
 I do, yes.

That's fine.  I left out the trailing semicolon/space.
The pr_fmt could be something like:
#define pr_fmt(fmt) KBUILD_MODNAME :%d:%s:  fmt, current-pid, __func__
or add a pid: descriptor prefix if you like too:
#define pr_fmt(fmt) KBUILD_MODNAME :pid:%d:%s:  fmt, current-pid, __func__

  it's better to use a consistent style for
  these logging functions ideally with terminating
  newlines so there isn't a mix of code with
  and without those newlines.  That inconsistency
  leads to unintended defects.
 
 The idea here was to make the logging consistent throughout.

Mine too.

 I have become used of not using the new-line terminator in logging over
 the years and tend to favour that myself. You recommend not doing that
 probably from a kernel wide consistency perspective? Maybe that is
 better ...

Yes, kernel style consistency is the rationale.

Over time, people come along and add messages
while not reading the code very closely so using
the kernel style with newlines can help avoid
these trivial defects.

cheers, Joe

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/