drivers/usb/core/sysfs.c Fix variable 'retval' set but not used
in [Kernel]
|
Prev: [PATCH 1/9] v4 Move the find_memory_block() routine up
Next: [2.6.36, patch] unprotected access to task credentials in waitid()
From: Justin P. Mattock on 3 Aug 2010 09:40 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 3 Aug 2010 10:40 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 3 Aug 2010 10:50 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 3 Aug 2010 11:40 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 3 Aug 2010 12:00
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/ |