diff mbox series

[v2] usbip: tools: Fix read_usb_vudc_device() error path handling

Message ID 20191017022512.3809-1-gy741.kim@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v2] usbip: tools: Fix read_usb_vudc_device() error path handling | expand

Commit Message

GwanYeong Kim Oct. 17, 2019, 2:25 a.m. UTC
cannot be less than 0 - fread() returns 0 on error.

Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
---
 tools/usb/usbip/libsrc/usbip_device_driver.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Shuah Oct. 17, 2019, 2:33 a.m. UTC | #1
On 10/16/19 8:25 PM, GwanYeong Kim wrote:
> cannot be less than 0 - fread() returns 0 on error.
> 

This isn't really accurate right. fread() doesn't always
return 0 in error. It could return < number of elements
and set errno.

Please make changes to reflect that.

> Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
> ---
>   tools/usb/usbip/libsrc/usbip_device_driver.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c b/tools/usb/usbip/libsrc/usbip_device_driver.c
> index 051d7d3f443b..959bb29d0477 100644
> --- a/tools/usb/usbip/libsrc/usbip_device_driver.c
> +++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
> @@ -69,7 +69,7 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
>   	FILE *fd = NULL;
>   	struct udev_device *plat;
>   	const char *speed;
> -	int ret = 0;
> +	size_t ret = 0;

You don't need to initialize this.

>   
>   	plat = udev_device_get_parent(sdev);
>   	path = udev_device_get_syspath(plat);
> @@ -79,7 +79,7 @@ int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
>   	if (!fd)
>   		return -1;
>   	ret = fread((char *) &descr, sizeof(descr), 1, fd);
> -	if (ret < 0)
> +	if (ret != 1)

Why not print error message?

>   		goto err;
>   	fclose(fd);
>   
> 

thanks,
-- Shuah
GwanYeong Kim Oct. 17, 2019, 5:26 a.m. UTC | #2
On Wed, 16 Oct 2019 20:33:39 -0600
shuah <shuah@kernel.org> wrote:

> On 10/16/19 8:25 PM, GwanYeong Kim wrote:
> > cannot be less than 0 - fread() returns 0 on error.
> > 
> 
> This isn't really accurate right. fread() doesn't always
> return 0 in error. It could return < number of elements
> and set errno.
> 
> Please make changes to reflect that.

Will reflect this.

> 
> > Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
> > ---
> >   tools/usb/usbip/libsrc/usbip_device_driver.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c
> > b/tools/usb/usbip/libsrc/usbip_device_driver.c index
> > 051d7d3f443b..959bb29d0477 100644 ---
> > a/tools/usb/usbip/libsrc/usbip_device_driver.c +++
> > b/tools/usb/usbip/libsrc/usbip_device_driver.c @@ -69,7 +69,7 @@
> > int read_usb_vudc_device(struct udev_device *sdev, struct
> > usbip_usb_device *dev) FILE *fd = NULL; struct udev_device *plat;
> >   	const char *speed;
> > -	int ret = 0;
> > +	size_t ret = 0;
> 
> You don't need to initialize this.

Exactly, because "ret" variable receives a "fread()" return value,

> 
> >   
> >   	plat = udev_device_get_parent(sdev);
> >   	path = udev_device_get_syspath(plat);
> > @@ -79,7 +79,7 @@ int read_usb_vudc_device(struct udev_device
> > *sdev, struct usbip_usb_device *dev) if (!fd)
> >   		return -1;
> >   	ret = fread((char *) &descr, sizeof(descr), 1, fd);
> > -	if (ret < 0)
> > +	if (ret != 1)
> 
> Why not print error message?


Sorry, I'll add a message.

How about this?

err("Cannot read vudc device descr file");


> 
> >   		goto err;
> >   	fclose(fd);
> >   
> > 
> 
> thanks,
> -- Shuah
Shuah Oct. 17, 2019, 1 p.m. UTC | #3
On 10/16/19 11:26 PM, GwanYeong Kim wrote:
> On Wed, 16 Oct 2019 20:33:39 -0600
> shuah <shuah@kernel.org> wrote:
> 
>> On 10/16/19 8:25 PM, GwanYeong Kim wrote:
>>> cannot be less than 0 - fread() returns 0 on error.
>>>
>>
>> This isn't really accurate right. fread() doesn't always
>> return 0 in error. It could return < number of elements
>> and set errno.
>>
>> Please make changes to reflect that.
> 
> Will reflect this.
> 
>>
>>> Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
>>> ---
>>>    tools/usb/usbip/libsrc/usbip_device_driver.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c
>>> b/tools/usb/usbip/libsrc/usbip_device_driver.c index
>>> 051d7d3f443b..959bb29d0477 100644 ---
>>> a/tools/usb/usbip/libsrc/usbip_device_driver.c +++
>>> b/tools/usb/usbip/libsrc/usbip_device_driver.c @@ -69,7 +69,7 @@
>>> int read_usb_vudc_device(struct udev_device *sdev, struct
>>> usbip_usb_device *dev) FILE *fd = NULL; struct udev_device *plat;
>>>    	const char *speed;
>>> -	int ret = 0;
>>> +	size_t ret = 0;
>>
>> You don't need to initialize this.
> 
> Exactly, because "ret" variable receives a "fread()" return value,
> 
>>
>>>    
>>>    	plat = udev_device_get_parent(sdev);
>>>    	path = udev_device_get_syspath(plat);
>>> @@ -79,7 +79,7 @@ int read_usb_vudc_device(struct udev_device
>>> *sdev, struct usbip_usb_device *dev) if (!fd)
>>>    		return -1;
>>>    	ret = fread((char *) &descr, sizeof(descr), 1, fd);
>>> -	if (ret < 0)
>>> +	if (ret != 1)
>>
>> Why not print error message?
> 
> 
> Sorry, I'll add a message.
> 
> How about this?
> 
> err("Cannot read vudc device descr file");

Using strerror() with errno gives you more information. Add that
to them essage you proposed.

thanks,
-- Shuah
diff mbox series

Patch

diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c b/tools/usb/usbip/libsrc/usbip_device_driver.c
index 051d7d3f443b..959bb29d0477 100644
--- a/tools/usb/usbip/libsrc/usbip_device_driver.c
+++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
@@ -69,7 +69,7 @@  int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
 	FILE *fd = NULL;
 	struct udev_device *plat;
 	const char *speed;
-	int ret = 0;
+	size_t ret = 0;
 
 	plat = udev_device_get_parent(sdev);
 	path = udev_device_get_syspath(plat);
@@ -79,7 +79,7 @@  int read_usb_vudc_device(struct udev_device *sdev, struct usbip_usb_device *dev)
 	if (!fd)
 		return -1;
 	ret = fread((char *) &descr, sizeof(descr), 1, fd);
-	if (ret < 0)
+	if (ret != 1)
 		goto err;
 	fclose(fd);