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

2019-10-17 Thread shuah 

On 10/16/19 11:26 PM, GwanYeong Kim wrote:

On Wed, 16 Oct 2019 20:33:39 -0600
shuah  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 
---
   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

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

2019-10-16 Thread GwanYeong Kim 
On Wed, 16 Oct 2019 20:33:39 -0600
shuah  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 
> > ---
> >   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

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

2019-10-16 Thread shuah 

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 
---
  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

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

2019-10-16 Thread GwanYeong Kim 
cannot be less than 0 - fread() returns 0 on error.

Signed-off-by: GwanYeong Kim 
---
 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;
 
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);
 
-- 
2.17.1