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 |
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
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
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 --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);
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(-)