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

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

[Wang Chen]

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);

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

[Cornelia Huck]

On Tue, 24 Jun 2008 16:59:06 +0800,
Wang Chen <wangchen@cn.fujitsu.com> wrote:

> 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);
> 
> 
> 

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).

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

[Wang Chen]

Cornelia Huck said the following on 2008-6-24 22:24:
> > On Tue, 24 Jun 2008 16:59:06 +0800,
> > Wang Chen <wangchen@cn.fujitsu.com> wrote:
> > 
>> >> 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);
>> >>
>> >>
>> >>
> > 
> > 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).
> > 

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?

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

[Cornelia Huck]

On Wed, 25 Jun 2008 17:08:37 +0800,
Wang Chen <wangchen@cn.fujitsu.com> wrote:

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

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.

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

[Wang Chen]

Cornelia Huck said the following on 2008-7-1 18:51:
> On Wed, 25 Jun 2008 17:08:37 +0800,
> Wang Chen <wangchen@cn.fujitsu.com> wrote:
> 
>> 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?
> 
> 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.
> 

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".