check device_create_file() return code in usb_create_sysfs_intf_files()
Hello,
I just noticed that usb_create_sysfs_intf_files() ignores device_create_file() return code and sets intf->sysfs_files_created to 1, even if sysfs_add_file_mode() returned -ENOMEM (or later sysfs_add_one() returned -EEXIST).
Shouldn't we check retval for 0 before setting intf->sysfs_files_created?
---
drivers/usb/core/sysfs.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
drivers/usb/core/sysfs.c +4 -2
@@ -856,8 +856,10 @@ int usb_create_sysfs_intf_files(struct usb_interface *intf) alt->string =usb_cache_string (udev, alt->desc.iInterface);if (alt->string) retval =device_create_file (&intf->dev, &dev_attr_interface); - intf->sysfs_files_created =1 ; -return 0 ; +if (retval ==0 ) + intf->sysfs_files_created =1 ; + +return retval; }void usb_remove_sysfs_intf_files (struct usb_interface *intf)
Quoted 6 lines · Sergey Senozhatsky
drivers/usb/core/sysfs.c +0
-0@@ -856,8 +856,10 @@ int usb_create_sysfs_intf_files(struct usb_interface *intf)
drivers/usb/core/sysfs.c int usb_create_sysfs_intf_files(struct · line 864
Quoted 10 lines
drivers/usb/core/sysfs.c +4
-2alt->string =usb_cache_string (udev, alt->desc.iInterface);if (alt->string) retval =device_create_file (&intf->dev, &dev_attr_interface); - intf->sysfs_files_created =1 ; -return 0 ; +if (retval ==0 ) + intf->sysfs_files_created =1 ; + +return retval; }
retval may be uninitialised. You need to zero it at the beginning of the function (or somewhere).
Quoted 6 lines
drivers/usb/core/sysfs.c +0
-1void usb_remove_sysfs_intf_files (struct usb_interface *intf) --To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 4 lines
Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michal "mina86" Nazarewicz (o o) ooo +-----<email/xmpp: [email protected]>-----ooO--(_)--Ooo--
Quoted 7 lines · Michal Nazarewicz
Quoted 7 lines
(quoted diff) +0
-2@@ -856,8 +856,10 @@ int usb_create_sysfs_intf_files(struct usb_interface *intf) alt->string = usb_cache_string(udev, alt->desc.iInterface); if (alt->string) retval = device_create_file(&intf->dev, &dev_attr_interface); - intf->sysfs_files_created = 1; - return 0;
Quoted 9 lines
Review int usb_create_sysfs_intf_files(struct · line 864
Quoted 5 lines
(quoted diff) +4
-0+ if (retval == 0) + intf->sysfs_files_created = 1; + + return retval; }retval may be uninitialised. You need to zero it at the beginning of the function (or somewhere).
Oops... Yes, indeed. Thanks.
---
drivers/usb/core/sysfs.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
drivers/usb/core/sysfs.c +5 -3
@@ -846,7 +846,7 @@ int usb_create_sysfs_intf_files(struct usb_interface *intf) {struct usb_device *udev =interface_to_usbdev (intf);struct usb_host_interface *alt = intf->cur_altsetting; -int retval; +int retval =-1 ;if (intf->sysfs_files_created || intf->unregistering)return 0 ;
@@ -856,8 +856,10 @@ int usb_create_sysfs_intf_files(struct usb_interface *intf) alt->string =usb_cache_string (udev, alt->desc.iInterface);if (alt->string) retval =device_create_file (&intf->dev, &dev_attr_interface); - intf->sysfs_files_created =1 ; -return 0 ; +if (retval ==0 ) + intf->sysfs_files_created =1 ; + +return retval; }void usb_remove_sysfs_intf_files (struct usb_interface *intf)
drivers/usb/core/sysfs.c int usb_create_sysfs_intf_files(struct usb_interface · line 850
Quoted 11 lines · Sergey Senozhatsky
drivers/usb/core/sysfs.c +1
-1@@ -846,7 +846,7 @@ int usb_create_sysfs_intf_files(struct usb_interface *intf) {struct usb_device *udev =interface_to_usbdev (intf);struct usb_host_interface *alt = intf->cur_altsetting; -int retval; +int retval =-1 ;
That should be 0 or some other meaningful value. A literal -1 is definitely not a good choice. This is just for future since Alan has explained this patch is not needed.
Quoted 2 lines
drivers/usb/core/sysfs.c +0
-0if (intf->sysfs_files_created || intf->unregistering)return 0 ;
Signature · 4 lines
Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michal "mina86" Nazarewicz (o o) ooo +-----<email/xmpp: [email protected]>-----ooO--(_)--Ooo--
Quoted 7 lines · Sergey Senozhatsky
Hello,
I just noticed that
usb_create_sysfs_intf_files()ignoresdevice_create_file()return code and setsintf->sysfs_files_createdto 1, even ifsysfs_add_file_mode()returned-ENOMEM(or latersysfs_add_one()returned-EEXIST).Shouldn't we check retval for 0 before setting
intf->sysfs_files_created?
No. We want this routine to succeed even if the sysfs files can't be created. The interface string attribute is more or less optional.
It would be okay to add a comment explaining this, so that other people don't make the same mistake (which has already happened -- you're not the first).
Alan Stern
Quoted 4 lines · Alan Stern
Quoted 4 lines
I just noticed that
usb_create_sysfs_intf_files()ignoresdevice_create_file()return code and setsintf->sysfs_files_createdto 1, even ifsysfs_add_file_mode()returned-ENOMEM(or latersysfs_add_one()returned-EEXIST).
Quoted 9 lines
Quoted 1 line
Shouldn't we check retval for 0 before setting
intf->sysfs_files_created?No. We want this routine to succeed even if the sysfs files can't be created. The interface string attribute is more or less optional.
It would be okay to add a comment explaining this, so that other people don't make the same mistake (which has already happened -- you're not the first).
Thanks. Sorry for the noise then.
Well, in that case, I guess, `int retval;' can be removed, since it's unused.
Best,
Sergey