Patchwork [libsas,v12,04/11] sysfs: handle 'parent deleted before child added'

login
register
mail settings
Submitter Dan Williams
Date March 23, 2012, 6:43 p.m.
Message ID <1332528209.23830.4.camel@ultramagnus.opencreations.com>
Download mbox | patch
Permalink /patch/148480/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Dan Williams - March 23, 2012, 6:43 p.m.
On Thu, 2012-03-22 at 07:39 -0700, Greg Kroah-Hartman wrote:
> And note, I hate pr_err(), what's wrong with printk() in this instance?

So how about an end run around the conversion to pr_err() and just make
these proper WARN()s since we end up calling dump_stack in both cases?

Something like:



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg KH - March 23, 2012, 8:54 p.m.
On Fri, Mar 23, 2012 at 11:43:28AM -0700, Dan Williams wrote:
> On Thu, 2012-03-22 at 07:39 -0700, Greg Kroah-Hartman wrote:
> > And note, I hate pr_err(), what's wrong with printk() in this instance?
> 
> So how about an end run around the conversion to pr_err() and just make
> these proper WARN()s since we end up calling dump_stack in both cases?
> 
> Something like:
> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index e5f86c0..1bd0893 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -192,15 +192,14 @@ static int kobject_add_internal(struct kobject *kobj)
>  
>  		/* be noisy on error issues */
>  		if (error == -EEXIST)
> -			pr_err("%s failed for %s with "
> -			       "-EEXIST, don't try to register things with "
> -			       "the same name in the same directory.\n",
> -			       __func__, kobject_name(kobj));
> +			WARN(1, "%s failed for %s with "
> +			     "-EEXIST, don't try to register things with "
> +			     "the same name in the same directory.\n",
> +			     __func__, kobject_name(kobj));
>  		else
> -			pr_err("%s failed for %s (error: %d parent: %s)\n",
> -			       __func__, kobject_name(kobj), error,
> -			       parent ? kobject_name(parent) : "'none'");
> -		dump_stack();
> +			WARN(1, "%s failed for %s (error: %d parent: %s)\n",
> +			     __func__, kobject_name(kobj), error,
> +			     parent ? kobject_name(parent) : "'none'");

I'm missing why this isn't the exact same thing that is currently
happening...

confused,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - March 23, 2012, 9:15 p.m.
On Fri, Mar 23, 2012 at 1:54 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Mar 23, 2012 at 11:43:28AM -0700, Dan Williams wrote:
>> On Thu, 2012-03-22 at 07:39 -0700, Greg Kroah-Hartman wrote:
>> > And note, I hate pr_err(), what's wrong with printk() in this instance?
>>
>> So how about an end run around the conversion to pr_err() and just make
>> these proper WARN()s since we end up calling dump_stack in both cases?
>>
>> Something like:
>>
>> diff --git a/lib/kobject.c b/lib/kobject.c
>> index e5f86c0..1bd0893 100644
>> --- a/lib/kobject.c
>> +++ b/lib/kobject.c
>> @@ -192,15 +192,14 @@ static int kobject_add_internal(struct kobject *kobj)
>>
>>               /* be noisy on error issues */
>>               if (error == -EEXIST)
>> -                     pr_err("%s failed for %s with "
>> -                            "-EEXIST, don't try to register things with "
>> -                            "the same name in the same directory.\n",
>> -                            __func__, kobject_name(kobj));
>> +                     WARN(1, "%s failed for %s with "
>> +                          "-EEXIST, don't try to register things with "
>> +                          "the same name in the same directory.\n",
>> +                          __func__, kobject_name(kobj));
>>               else
>> -                     pr_err("%s failed for %s (error: %d parent: %s)\n",
>> -                            __func__, kobject_name(kobj), error,
>> -                            parent ? kobject_name(parent) : "'none'");
>> -             dump_stack();
>> +                     WARN(1, "%s failed for %s (error: %d parent: %s)\n",
>> +                          __func__, kobject_name(kobj), error,
>> +                          parent ? kobject_name(parent) : "'none'");
>
> I'm missing why this isn't the exact same thing that is currently
> happening...
>
> confused,

WARN does wrap the printk and backtrace in "------------[ cut here
]------------\n" and "---[ end trace %016llx ]---\n" so automated
tools can pick it up, maybe that's useful in a core routine like this?

It's still modifying the else case to add the faulting child and
parent kobject names, and does so without setting a pr_ conversion
precedent in sysfs.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/lib/kobject.c b/lib/kobject.c
index e5f86c0..1bd0893 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -192,15 +192,14 @@  static int kobject_add_internal(struct kobject *kobj)
 
 		/* be noisy on error issues */
 		if (error == -EEXIST)
-			pr_err("%s failed for %s with "
-			       "-EEXIST, don't try to register things with "
-			       "the same name in the same directory.\n",
-			       __func__, kobject_name(kobj));
+			WARN(1, "%s failed for %s with "
+			     "-EEXIST, don't try to register things with "
+			     "the same name in the same directory.\n",
+			     __func__, kobject_name(kobj));
 		else
-			pr_err("%s failed for %s (error: %d parent: %s)\n",
-			       __func__, kobject_name(kobj), error,
-			       parent ? kobject_name(parent) : "'none'");
-		dump_stack();
+			WARN(1, "%s failed for %s (error: %d parent: %s)\n",
+			     __func__, kobject_name(kobj), error,
+			     parent ? kobject_name(parent) : "'none'");
 	} else
 		kobj->state_in_sysfs = 1;