[PATCH 2/2] kobjects: Transmit return value of kobject_uevent_env() to caller

The error signal shouldn't be dropped.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
diff --git a/lib/kobject.c b/lib/kobject.c
index 718e510..e5de71e 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -430,7 +430,7 @@ int kobject_rename(struct kobject *kobj, const char *new_name)
* Some hotplug package track interfaces by their name and
* therefore want to know when the name is changed by the user. */
if (!error)
- kobject_uevent_env(kobj, KOBJ_MOVE, envp);
+ error = kobject_uevent_env(kobj, KOBJ_MOVE, envp);

out:
kfree(devpath_string);
@@ -482,7 +482,7 @@ int kobject_move(struct kobject *kobj, struct kobject *new_parent)
kobj->parent = new_parent;
new_parent = NULL;
kobject_put(old_parent);
- kobject_uevent_env(kobj, KOBJ_MOVE, envp);
+ error = kobject_uevent_env(kobj, KOBJ_MOVE, envp);
out:
kobject_put(new_parent);
kobject_put(kobj);



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

On Tue, 24 Jun 2008 16:59:06 +0800,
Wang Chen <wangchen@cn.fujitsu.com> wrote:
[color=blue]
> The error signal shouldn't be dropped.
>
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
> ---
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 718e510..e5de71e 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -430,7 +430,7 @@ int kobject_rename(struct kobject *kobj, const char *new_name)
> * Some hotplug package track interfaces by their name and
> * therefore want to know when the name is changed by the user. */
> if (!error)
> - kobject_uevent_env(kobj, KOBJ_MOVE, envp);
> + error = kobject_uevent_env(kobj, KOBJ_MOVE, envp);
>
> out:
> kfree(devpath_string);
> @@ -482,7 +482,7 @@ int kobject_move(struct kobject *kobj, struct kobject *new_parent)
> kobj->parent = new_parent;
> new_parent = NULL;
> kobject_put(old_parent);
> - kobject_uevent_env(kobj, KOBJ_MOVE, envp);
> + error = kobject_uevent_env(kobj, KOBJ_MOVE, envp);
> out:
> kobject_put(new_parent);
> kobject_put(kobj);
>
>
>[/color]

This looks wrong. If everything went right except sending the uevent,
you'll make the caller believe that the whole operation failed, while
in reality the sysfs operation succeeded. Either just drop the error
again (or print a warning), or undo the previous operations on error
(which may fail).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

Cornelia Huck said the following on 2008-6-24 22:24:[color=blue][color=green]
> > On Tue, 24 Jun 2008 16:59:06 +0800,
> > Wang Chen <wangchen@cn.fujitsu.com> wrote:
> >[color=darkred]
>> >> The error signal shouldn't be dropped.
>> >>
>> >> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
>> >> ---
>> >> diff --git a/lib/kobject.c b/lib/kobject.c
>> >> index 718e510..e5de71e 100644
>> >> --- a/lib/kobject.c
>> >> +++ b/lib/kobject.c
>> >> @@ -430,7 +430,7 @@ int kobject_rename(struct kobject *kobj, const char *new_name)
>> >> * Some hotplug package track interfaces by their name and
>> >> * therefore want to know when the name is changed by the user. */
>> >> if (!error)
>> >> - kobject_uevent_env(kobj, KOBJ_MOVE, envp);
>> >> + error = kobject_uevent_env(kobj, KOBJ_MOVE, envp);
>> >>
>> >> out:
>> >> kfree(devpath_string);
>> >> @@ -482,7 +482,7 @@ int kobject_move(struct kobject *kobj, struct kobject *new_parent)
>> >> kobj->parent = new_parent;
>> >> new_parent = NULL;
>> >> kobject_put(old_parent);
>> >> - kobject_uevent_env(kobj, KOBJ_MOVE, envp);
>> >> + error = kobject_uevent_env(kobj, KOBJ_MOVE, envp);
>> >> out:
>> >> kobject_put(new_parent);
>> >> kobject_put(kobj);
>> >>
>> >>
>> >>[/color]
> >
> > This looks wrong. If everything went right except sending the uevent,
> > you'll make the caller believe that the whole operation failed, while
> > in reality the sysfs operation succeeded. Either just drop the error
> > again (or print a warning), or undo the previous operations on error
> > (which may fail).
> >[/color][/color]

It's depended on how important sending uevent is.
If the sysfs operation succeeds, but user-space don't
get the notification. Is it ok?


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

On Wed, 25 Jun 2008 17:08:37 +0800,
Wang Chen <wangchen@cn.fujitsu.com> wrote:
[color=blue]
> It's depended on how important sending uevent is.
> If the sysfs operation succeeds, but user-space don't
> get the notification. Is it ok?[/color]

Given that (a) there may be no listener, (b) sending via netlink may
succeed but calling uevent_helper may fail (highly unlikely the way
modern distros are set up, though), and (c) you would have to audit a
myriad places that don't check for the return code of kobject_uevent()
today as well, it looks like the best way is to simply add some
pr_debug()s to kobject_uevent_env() where something fails.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

Cornelia Huck said the following on 2008-7-1 18:51:[color=blue]
> On Wed, 25 Jun 2008 17:08:37 +0800,
> Wang Chen <wangchen@cn.fujitsu.com> wrote:
>[color=green]
>> It's depended on how important sending uevent is.
>> If the sysfs operation succeeds, but user-space don't
>> get the notification. Is it ok?[/color]
>
> Given that (a) there may be no listener, (b) sending via netlink may
> succeed but calling uevent_helper may fail (highly unlikely the way
> modern distros are set up, though), and (c) you would have to audit a
> myriad places that don't check for the return code of kobject_uevent()
> today as well, it looks like the best way is to simply add some
> pr_debug()s to kobject_uevent_env() where something fails.
>[/color]

Fair enough. Thanks Cornelia.
Greg, please ignore this patch.
But please take a look at "[PATCH 1/2] kobjects: Transmit return value of call_usermodehelper() to caller".

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