Fix warning form checkpatch, use pr_warn replace KERN_WARNING
Signed-off-by: Bo YU <[email protected]>
---
changes in v2:
According to Joe's suggestion,drop newline from msg, otherwise
it can be unterminated with newline.
---
lib/kobject_uevent.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 63d0816ab23b..1837765ebf01 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -195,12 +195,12 @@ int kobject_synth_uevent(struct kobject *kobj, const char *buf, size_t count)
enum kobject_action action;
const char *action_args;
struct kobj_uevent_env *env;
- const char *msg = NULL, *devpath;
+ const char *msg = NULL;
int r;
r = kobject_action_type(buf, count, &action, &action_args);
if (r) {
- msg = "unknown uevent action string\n";
+ msg = "unknown uevent action string";
goto out;
}
@@ -212,7 +212,7 @@ int kobject_synth_uevent(struct kobject *kobj, const char *buf, size_t count)
r = kobject_action_args(action_args,
count - (action_args - buf), &env);
if (r == -EINVAL) {
- msg = "incorrect uevent action arguments\n";
+ msg = "incorrect uevent action arguments";
goto out;
}
@@ -223,8 +223,9 @@ int kobject_synth_uevent(struct kobject *kobj, const char *buf, size_t count)
kfree(env);
out:
if (r) {
- devpath = kobject_get_path(kobj, GFP_KERNEL);
- printk(KERN_WARNING "synth uevent: %s: %s",
+ char *devpath = kobject_get_path(kobj, GFP_KERNEL);
+
+ pr_warn("synth uevent: %s: %s\n",
devpath ?: "unknown device",
msg ?: "failed to send uevent");
kfree(devpath);
--
2.11.0
On Tue, Nov 6, 2018 at 3:42 AM Bo YU <[email protected]> wrote:
>
> Fix warning form checkpatch, use pr_warn replace KERN_WARNING
>
> Signed-off-by: Bo YU <[email protected]>
First off, IMO, you should not change the existing code just in order
to make checkpatch happy about it. That alone is not a good enough
reason for modifying it.
If the checkpatch warning indicates an issue like broken white space
(and I mean really broken and not lines longer than 80 chars etc),
then that may be a reason to modify the existing code, depending.
> ---
> changes in v2:
> According to Joe's suggestion,drop newline from msg, otherwise
> it can be unterminated with newline.
> ---
> lib/kobject_uevent.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index 63d0816ab23b..1837765ebf01 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -195,12 +195,12 @@ int kobject_synth_uevent(struct kobject *kobj, const char *buf, size_t count)
> enum kobject_action action;
> const char *action_args;
> struct kobj_uevent_env *env;
> - const char *msg = NULL, *devpath;
> + const char *msg = NULL;
> int r;
>
> r = kobject_action_type(buf, count, &action, &action_args);
> if (r) {
> - msg = "unknown uevent action string\n";
> + msg = "unknown uevent action string";
> goto out;
> }
>
> @@ -212,7 +212,7 @@ int kobject_synth_uevent(struct kobject *kobj, const char *buf, size_t count)
> r = kobject_action_args(action_args,
> count - (action_args - buf), &env);
> if (r == -EINVAL) {
> - msg = "incorrect uevent action arguments\n";
> + msg = "incorrect uevent action arguments";
Second, this change is not what the changelog of your patch is talking about.
> goto out;
> }
>
> @@ -223,8 +223,9 @@ int kobject_synth_uevent(struct kobject *kobj, const char *buf, size_t count)
> kfree(env);
> out:
> if (r) {
> - devpath = kobject_get_path(kobj, GFP_KERNEL);
> - printk(KERN_WARNING "synth uevent: %s: %s",
> + char *devpath = kobject_get_path(kobj, GFP_KERNEL);
> +
> + pr_warn("synth uevent: %s: %s\n",
> devpath ?: "unknown device",
> msg ?: "failed to send uevent");
> kfree(devpath);
> --
> 2.11.0
>
On Tue, 2018-11-06 at 08:49 +0100, Rafael J. Wysocki wrote:
> On Tue, Nov 6, 2018 at 3:42 AM Bo YU <[email protected]> wrote:
> > Fix warning form checkpatch, use pr_warn replace KERN_WARNING
> >
> > Signed-off-by: Bo YU <[email protected]>
>
> First off, IMO, you should not change the existing code just in order
> to make checkpatch happy about it. That alone is not a good enough
> reason for modifying it.
>
> If the checkpatch warning indicates an issue like broken white space
> (and I mean really broken and not lines longer than 80 chars etc),
> then that may be a reason to modify the existing code, depending.
Existing code is slightly broken.
There is currently a missing terminating newline in the
non-switch case match, when msg == NULL;
It might be simpler to initialize msg like
const char *msg = "failed to send uevent";
and always use msg and not msg ?: in the printk.
> > changes in v2:
> > According to Joe's suggestion,drop newline from msg, otherwise
> > it can be unterminated with newline.ch
> > ---
> > lib/kobject_uevent.c | 11 ++++++-----
> > 1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> > index 63d0816ab23b..1837765ebf01 100644
> > --- a/lib/kobject_uevent.c
> > +++ b/lib/kobject_uevent.c
> > @@ -195,12 +195,12 @@ int kobject_synth_uevent(struct kobject *kobj, const char *buf, size_t count)
> > enum kobject_action action;
> > const char *action_args;
> > struct kobj_uevent_env *env;
> > - const char *msg = NULL, *devpath;
> > + const char *msg = NULL;
> > int r;
> >
> > r = kobject_action_type(buf, count, &action, &action_args);
> > if (r) {
> > - msg = "unknown uevent action string\n";
> > + msg = "unknown uevent action string";
> > goto out;
> > }
> >
> > @@ -212,7 +212,7 @@ int kobject_synth_uevent(struct kobject *kobj, const char *buf, size_t count)
> > r = kobject_action_args(action_args,
> > count - (action_args - buf), &env);
> > if (r == -EINVAL) {
> > - msg = "incorrect uevent action arguments\n";
> > + msg = "incorrect uevent action arguments";
>
> Second, this change is not what the changelog of your patch is talking about.
>
> > goto out;
> > }
> >
> > @@ -223,8 +223,9 @@ int kobject_synth_uevent(struct kobject *kobj, const char *buf, size_t count)
> > kfree(env);
> > out:
> > if (r) {
> > - devpath = kobject_get_path(kobj, GFP_KERNEL);
> > - printk(KERN_WARNING "synth uevent: %s: %s",
> > + char *devpath = kobject_get_path(kobj, GFP_KERNEL);
> > +
> > + pr_warn("synth uevent: %s: %s\n",
> > devpath ?: "unknown device",
> > msg ?: "failed to send uevent");
> > kfree(devpath);
> > --
> > 2.11.0
> >
On Tue, Nov 6, 2018 at 8:58 AM Joe Perches <[email protected]> wrote:
>
> On Tue, 2018-11-06 at 08:49 +0100, Rafael J. Wysocki wrote:
> > On Tue, Nov 6, 2018 at 3:42 AM Bo YU <[email protected]> wrote:
> > > Fix warning form checkpatch, use pr_warn replace KERN_WARNING
> > >
> > > Signed-off-by: Bo YU <[email protected]>
> >
> > First off, IMO, you should not change the existing code just in order
> > to make checkpatch happy about it. That alone is not a good enough
> > reason for modifying it.
> >
> > If the checkpatch warning indicates an issue like broken white space
> > (and I mean really broken and not lines longer than 80 chars etc),
> > then that may be a reason to modify the existing code, depending.
>
> Existing code is slightly broken.
> There is currently a missing terminating newline in the
> non-switch case match, when msg == NULL;
OK, so this should be explained in the patch changelog.
Saying "I do this to make checkpatch happy" in the changelog is just
not enough IMO (if not outright misleading).
On Tue, Nov 06, 2018 at 09:09:15AM +0100, Rafael J. Wysocki wrote:
>On Tue, Nov 6, 2018 at 8:58 AM Joe Perches <[email protected]> wrote:
>>
>> On Tue, 2018-11-06 at 08:49 +0100, Rafael J. Wysocki wrote:
>> > On Tue, Nov 6, 2018 at 3:42 AM Bo YU <[email protected]> wrote:
>> > > Fix warning form checkpatch, use pr_warn replace KERN_WARNING
>> > >
>> > > Signed-off-by: Bo YU <[email protected]>
>> >
>> > First off, IMO, you should not change the existing code just in order
>> > to make checkpatch happy about it. That alone is not a good enough
>> > reason for modifying it.
>> >
>> > If the checkpatch warning indicates an issue like broken white space
>> > (and I mean really broken and not lines longer than 80 chars etc),
>> > then that may be a reason to modify the existing code, depending.
>>
>> Existing code is slightly broken.
>> There is currently a missing terminating newline in the
>> non-switch case match, when msg == NULL;
>
>OK, so this should be explained in the patch changelog.
>
>Saying "I do this to make checkpatch happy" in the changelog is just
>not enough IMO (if not outright misleading).
Using pr_* code become more and more in kernel when i read code and then i ask
for help from checkpatch. In commit log i just copy the what to to but not my
explations that why to do such.
I will send V3, thanks.