From: Justin P. Mattock on
On 08/03/2010 04:28 AM, Valdis.Kletnieks(a)vt.edu wrote:
> On Mon, 02 Aug 2010 21:26:28 PDT, "Justin P. Mattock" said:
>> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
>
>> if (alt->string)
>> - retval = device_create_file(&intf->dev,&dev_attr_interface);
>> + device_create_file(&intf->dev,&dev_attr_interface);
>> intf->sysfs_files_created = 1;
>> return 0;
>
> What should the code do if device_create_file() manages to fail? Yes, ignoring
> the return value is one option, but is it the best one? 'return ret;' might be
> another one. Somebody who understands this code and has more caffeine than me
> should look this over.
>
ignoring the return value is one option, but is it the best one?
probably not. As for return ret; the option did cross my mind, but
figured to do what I had done by removing the retval

> (Nothing personal Justin - it's just my opinion that *anytime* we have a patch
> that remove a check for a return code, it needs to justify that ignoring the
> return code is appropriate).
>

nothing personal taken.. in fact I agree with that whole paragraph.
looking back I should of really explained why I was removing this code
besides a warning message.

Thanks for the response and info on this..

Justin P. Mattock

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on
On Tue, 3 Aug 2010 Valdis.Kletnieks(a)vt.edu wrote:

> On Mon, 02 Aug 2010 21:26:28 PDT, "Justin P. Mattock" said:
> > diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
>
> > if (alt->string)
> > - retval = device_create_file(&intf->dev, &dev_attr_interface);
> > + device_create_file(&intf->dev, &dev_attr_interface);
> > intf->sysfs_files_created = 1;
> > return 0;

Justin, did you try compiling your new code? Those unused values are
there because device_create_file is declared as __must_check.

> What should the code do if device_create_file() manages to fail? Yes, ignoring
> the return value is one option, but is it the best one? 'return ret;' might be
> another one. Somebody who understands this code and has more caffeine than me
> should look this over.

Failure to create a file in sysfs is almost never fatal and usually not
even dangerous. Ignoring the error is generally better than failing
the entire operation.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Justin P. Mattock on
On 08/03/2010 07:29 AM, Alan Stern wrote:
> On Tue, 3 Aug 2010 Valdis.Kletnieks(a)vt.edu wrote:
>
>> On Mon, 02 Aug 2010 21:26:28 PDT, "Justin P. Mattock" said:
>>> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
>>
>>> if (alt->string)
>>> - retval = device_create_file(&intf->dev,&dev_attr_interface);
>>> + device_create_file(&intf->dev,&dev_attr_interface);
>>> intf->sysfs_files_created = 1;
>>> return 0;
>
> Justin, did you try compiling your new code? Those unused values are
> there because device_create_file is declared as __must_check.
>

I went as far as compiling, once I saw no warning then figured o.k
I'll send out what I have for feedback then go from there.
(and just for the record I want to thank those who took the time to go
through and give feedback).


>> What should the code do if device_create_file() manages to fail? Yes, ignoring
>> the return value is one option, but is it the best one? 'return ret;' might be
>> another one. Somebody who understands this code and has more caffeine than me
>> should look this over.
>
> Failure to create a file in sysfs is almost never fatal and usually not
> even dangerous. Ignoring the error is generally better than failing
> the entire operation.
>
> Alan Stern
>
>

ahh.. you made this more clear for me.. cool thanks!

Justin P. Mattock
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on
On Tue, 3 Aug 2010 Valdis.Kletnieks(a)vt.edu wrote:

> > Failure to create a file in sysfs is almost never fatal and usually not
> > even dangerous. Ignoring the error is generally better than failing
> > the entire operation.
>
> Then why the __must_check attribute if it's usually ignorable? I thought
> that was reserved for functions that you damned sight better well check
> for errors because bad things are afoot otherwise?

That's a good question. Perhaps Greg KH knows the answer.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Greg KH on
On Tue, Aug 03, 2010 at 11:34:26AM -0400, Alan Stern wrote:
> On Tue, 3 Aug 2010 Valdis.Kletnieks(a)vt.edu wrote:
>
> > > Failure to create a file in sysfs is almost never fatal and usually not
> > > even dangerous. Ignoring the error is generally better than failing
> > > the entire operation.
> >
> > Then why the __must_check attribute if it's usually ignorable? I thought
> > that was reserved for functions that you damned sight better well check
> > for errors because bad things are afoot otherwise?
>
> That's a good question. Perhaps Greg KH knows the answer.

You should check the return value for that function. To not do that is
a bug. This one looks like it snuck through. Fixing it properly is the
correct thing to do.

thanks,

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