* [PATCH] usbip: tools: Fix read_usb_vudc_device() error path handling
@ 2019-10-15 13:14 GwanYeong Kim
2019-10-15 23:14 ` shuah
0 siblings, 1 reply; 10+ messages in thread
From: GwanYeong Kim @ 2019-10-15 13:14 UTC (permalink / raw)
To: valentina.manea.m
Cc: shuah, gregkh, allison, opensource, changcheng.liu, tglx,
linux-usb, GwanYeong Kim
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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c b/tools/usb/usbip/libsrc/usbip_device_driver.c
index 051d7d3f443b..49760b98aabc 100644
--- a/tools/usb/usbip/libsrc/usbip_device_driver.c
+++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
@@ -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 != sizeof(descr))
goto err;
fclose(fd);
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] usbip: tools: Fix read_usb_vudc_device() error path handling
2019-10-15 13:14 [PATCH] usbip: tools: Fix read_usb_vudc_device() error path handling GwanYeong Kim
@ 2019-10-15 23:14 ` shuah
2019-10-16 4:38 ` GwanYeong Kim
0 siblings, 1 reply; 10+ messages in thread
From: shuah @ 2019-10-15 23:14 UTC (permalink / raw)
To: GwanYeong Kim, valentina.manea.m
Cc: gregkh, allison, opensource, changcheng.liu, tglx, linux-usb,
shuah
On 10/15/19 7:14 AM, GwanYeong Kim wrote:
> 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c b/tools/usb/usbip/libsrc/usbip_device_driver.c
> index 051d7d3f443b..49760b98aabc 100644
> --- a/tools/usb/usbip/libsrc/usbip_device_driver.c
> +++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
> @@ -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 != sizeof(descr))
Are you sure this check is correct? fread() returns the number
of elements read, # elements = 1 in this case.
fread() returns 0 when size or # of elements is 0 and in other
error cases it will return < # of elements. I would think you
want to check ret != 1 here.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] usbip: tools: Fix read_usb_vudc_device() error path handling
2019-10-15 23:14 ` shuah
@ 2019-10-16 4:38 ` GwanYeong Kim
2019-10-16 13:18 ` Greg KH
0 siblings, 1 reply; 10+ messages in thread
From: GwanYeong Kim @ 2019-10-16 4:38 UTC (permalink / raw)
To: shuah
Cc: valentina.manea.m, gregkh, allison, opensource, changcheng.liu,
tglx, linux-usb
On Tue, 15 Oct 2019 17:14:32 -0600
shuah <shuah@kernel.org> wrote:
> On 10/15/19 7:14 AM, GwanYeong Kim wrote:
> > 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 | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c
> > b/tools/usb/usbip/libsrc/usbip_device_driver.c index
> > 051d7d3f443b..49760b98aabc 100644 ---
> > a/tools/usb/usbip/libsrc/usbip_device_driver.c +++
> > b/tools/usb/usbip/libsrc/usbip_device_driver.c @@ -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 != sizeof(descr))
>
> Are you sure this check is correct? fread() returns the number
> of elements read, # elements = 1 in this case.
Thank you for pointing this out.
Sorry for my mistake.
> fread() returns 0 when size or # of elements is 0 and in other
> error cases it will return < # of elements. I would think you
> want to check ret != 1 here.
You're right.
This should be changed to "ret != 1".
Should I send a new patch?
Regards,
GwanYeong Kim
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] usbip: tools: Fix read_usb_vudc_device() error path handling
2019-10-16 4:38 ` GwanYeong Kim
@ 2019-10-16 13:18 ` Greg KH
2019-10-17 2:25 ` [PATCH v2] " GwanYeong Kim
0 siblings, 1 reply; 10+ messages in thread
From: Greg KH @ 2019-10-16 13:18 UTC (permalink / raw)
To: GwanYeong Kim
Cc: shuah, valentina.manea.m, allison, opensource, changcheng.liu,
tglx, linux-usb
On Wed, Oct 16, 2019 at 01:38:25PM +0900, GwanYeong Kim wrote:
> On Tue, 15 Oct 2019 17:14:32 -0600
> shuah <shuah@kernel.org> wrote:
>
> > On 10/15/19 7:14 AM, GwanYeong Kim wrote:
> > > 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 | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tools/usb/usbip/libsrc/usbip_device_driver.c
> > > b/tools/usb/usbip/libsrc/usbip_device_driver.c index
> > > 051d7d3f443b..49760b98aabc 100644 ---
> > > a/tools/usb/usbip/libsrc/usbip_device_driver.c +++
> > > b/tools/usb/usbip/libsrc/usbip_device_driver.c @@ -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 != sizeof(descr))
> >
> > Are you sure this check is correct? fread() returns the number
> > of elements read, # elements = 1 in this case.
>
> Thank you for pointing this out.
> Sorry for my mistake.
>
> > fread() returns 0 when size or # of elements is 0 and in other
> > error cases it will return < # of elements. I would think you
> > want to check ret != 1 here.
>
> You're right.
> This should be changed to "ret != 1".
>
> Should I send a new patch?
If you want to have it applied, yes.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] usbip: tools: Fix read_usb_vudc_device() error path handling
2019-10-16 13:18 ` Greg KH
@ 2019-10-17 2:25 ` GwanYeong Kim
2019-10-17 2:33 ` shuah
0 siblings, 1 reply; 10+ messages in thread
From: GwanYeong Kim @ 2019-10-17 2:25 UTC (permalink / raw)
To: valentina.manea.m
Cc: shuah, gregkh, allison, opensource, changcheng.liu, tglx,
linux-usb, GwanYeong Kim
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(-)
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] usbip: tools: Fix read_usb_vudc_device() error path handling
2019-10-17 2:25 ` [PATCH v2] " GwanYeong Kim
@ 2019-10-17 2:33 ` shuah
2019-10-17 5:26 ` GwanYeong Kim
0 siblings, 1 reply; 10+ messages in thread
From: shuah @ 2019-10-17 2:33 UTC (permalink / raw)
To: GwanYeong Kim, valentina.manea.m
Cc: gregkh, allison, opensource, changcheng.liu, tglx, linux-usb,
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 <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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] usbip: tools: Fix read_usb_vudc_device() error path handling
2019-10-17 2:33 ` shuah
@ 2019-10-17 5:26 ` GwanYeong Kim
2019-10-17 13:00 ` shuah
0 siblings, 1 reply; 10+ messages in thread
From: GwanYeong Kim @ 2019-10-17 5:26 UTC (permalink / raw)
To: shuah
Cc: valentina.manea.m, gregkh, allison, opensource, changcheng.liu,
tglx, linux-usb
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] usbip: tools: Fix read_usb_vudc_device() error path handling
2019-10-17 5:26 ` GwanYeong Kim
@ 2019-10-17 13:00 ` shuah
2019-10-18 3:22 ` [PATCH v3] " GwanYeong Kim
0 siblings, 1 reply; 10+ messages in thread
From: shuah @ 2019-10-17 13:00 UTC (permalink / raw)
To: GwanYeong Kim
Cc: valentina.manea.m, gregkh, allison, opensource, changcheng.liu,
tglx, linux-usb, shuah
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
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3] usbip: tools: Fix read_usb_vudc_device() error path handling
2019-10-17 13:00 ` shuah
@ 2019-10-18 3:22 ` GwanYeong Kim
2019-10-21 18:59 ` shuah
0 siblings, 1 reply; 10+ messages in thread
From: GwanYeong Kim @ 2019-10-18 3:22 UTC (permalink / raw)
To: valentina.manea.m
Cc: shuah, gregkh, allison, opensource, changcheng.liu, tglx,
linux-usb, GwanYeong Kim
This isn't really accurate right. fread() doesn't always
return 0 in error. It could return < number of elements
and set errno.
Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
---
tools/usb/usbip/libsrc/usbip_device_driver.c | 6 ++++--
1 file changed, 4 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..927a151fa9aa 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;
plat = udev_device_get_parent(sdev);
path = udev_device_get_syspath(plat);
@@ -79,8 +79,10 @@ 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) {
+ err("Cannot read vudc device descr file: %s", strerror(errno));
goto err;
+ }
fclose(fd);
copy_descr_attr(dev, &descr, bDeviceClass);
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v3] usbip: tools: Fix read_usb_vudc_device() error path handling
2019-10-18 3:22 ` [PATCH v3] " GwanYeong Kim
@ 2019-10-21 18:59 ` shuah
0 siblings, 0 replies; 10+ messages in thread
From: shuah @ 2019-10-21 18:59 UTC (permalink / raw)
To: GwanYeong Kim, valentina.manea.m
Cc: gregkh, allison, opensource, changcheng.liu, tglx, linux-usb,
shuah
On 10/17/19 9:22 PM, GwanYeong Kim wrote:
> This isn't really accurate right. fread() doesn't always
> return 0 in error. It could return < number of elements
> and set errno.
>
> Signed-off-by: GwanYeong Kim <gy741.kim@gmail.com>
> ---
> tools/usb/usbip/libsrc/usbip_device_driver.c | 6 ++++--
> 1 file changed, 4 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..927a151fa9aa 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;
>
> plat = udev_device_get_parent(sdev);
> path = udev_device_get_syspath(plat);
> @@ -79,8 +79,10 @@ 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) {
> + err("Cannot read vudc device descr file: %s", strerror(errno));
> goto err;
> + }
> fclose(fd);
>
> copy_descr_attr(dev, &descr, bDeviceClass);
>
Looks good.
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-10-21 19:00 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-10-15 13:14 [PATCH] usbip: tools: Fix read_usb_vudc_device() error path handling GwanYeong Kim
2019-10-15 23:14 ` shuah
2019-10-16 4:38 ` GwanYeong Kim
2019-10-16 13:18 ` Greg KH
2019-10-17 2:25 ` [PATCH v2] " GwanYeong Kim
2019-10-17 2:33 ` shuah
2019-10-17 5:26 ` GwanYeong Kim
2019-10-17 13:00 ` shuah
2019-10-18 3:22 ` [PATCH v3] " GwanYeong Kim
2019-10-21 18:59 ` shuah
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).