As real device-nodes managed by udev whose nodes lived in /dev/bus/usb
are mostly used today, let testusb tool use that directory as one default
path make tool be more convenient to use.
Signed-off-by: Du Changbin <[email protected]>
---
tools/usb/testusb.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/tools/usb/testusb.c b/tools/usb/testusb.c
index 6e0f567..82d7c59 100644
--- a/tools/usb/testusb.c
+++ b/tools/usb/testusb.c
@@ -358,6 +358,7 @@ static const char *usbfs_dir_find(void)
{
static char usbfs_path_0[] = "/dev/usb/devices";
static char usbfs_path_1[] = "/proc/bus/usb/devices";
+ static char udev_usb_path[] = "/dev/bus/usb";
static char *const usbfs_paths[] = {
usbfs_path_0, usbfs_path_1
@@ -376,6 +377,10 @@ static const char *usbfs_dir_find(void)
}
} while (++it != end);
+ /* real device-nodes managed by udev */
+ if (access(udev_usb_path, F_OK) == 0)
+ return udev_usb_path;
+
return NULL;
}
--
1.7.9.5
As real device-nodes managed by udev whose nodes lived in /dev/bus/usb are mostly used today, let testusb tool use that directory as one default path make tool be more convenient to use.
Signed-off-by: Du Changbin <[email protected]>
---
tools/usb/testusb.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/tools/usb/testusb.c b/tools/usb/testusb.c index 6e0f567..82d7c59 100644
--- a/tools/usb/testusb.c
+++ b/tools/usb/testusb.c
@@ -358,6 +358,7 @@ static const char *usbfs_dir_find(void) {
static char usbfs_path_0[] = "/dev/usb/devices";
static char usbfs_path_1[] = "/proc/bus/usb/devices";
+ static char udev_usb_path[] = "/dev/bus/usb";
static char *const usbfs_paths[] = {
usbfs_path_0, usbfs_path_1
@@ -376,6 +377,10 @@ static const char *usbfs_dir_find(void)
}
} while (++it != end);
+ /* real device-nodes managed by udev */
+ if (access(udev_usb_path, F_OK) == 0)
+ return udev_usb_path;
+
return NULL;
}
--
1.7.9.5
On Tue, 22 May 2012, Du, ChangbinX wrote:
> As real device-nodes managed by udev whose nodes lived in /dev/bus/usb are mostly used today, let testusb tool use that directory as one default path make tool be more convenient to use.
>
> Signed-off-by: Du Changbin <[email protected]>
> ---
> tools/usb/testusb.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/tools/usb/testusb.c b/tools/usb/testusb.c index 6e0f567..82d7c59 100644
> --- a/tools/usb/testusb.c
> +++ b/tools/usb/testusb.c
> @@ -358,6 +358,7 @@ static const char *usbfs_dir_find(void) {
> static char usbfs_path_0[] = "/dev/usb/devices";
> static char usbfs_path_1[] = "/proc/bus/usb/devices";
> + static char udev_usb_path[] = "/dev/bus/usb";
>
> static char *const usbfs_paths[] = {
> usbfs_path_0, usbfs_path_1
> @@ -376,6 +377,10 @@ static const char *usbfs_dir_find(void)
> }
> } while (++it != end);
>
> + /* real device-nodes managed by udev */
> + if (access(udev_usb_path, F_OK) == 0)
> + return udev_usb_path;
> +
> return NULL;
> }
>
Two issues with this: F_OK only guarantees that the path exists, it does
not guarantee that it is readable like this function guarantees for
usbfs_paths, and access() shouldn't be used because of its security
implications, you're better off using open() and testing for fd.
On Tue, 22 May 2012, Du, ChangbinX wrote:
> As real device-nodes managed by udev whose nodes lived in /dev/bus/usb are mostly used today, let testusb tool use that directory as one default path make tool be more convenient to use.
>
> Signed-off-by: Du Changbin <[email protected]>
> ---
> tools/usb/testusb.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/tools/usb/testusb.c b/tools/usb/testusb.c index
> 6e0f567..82d7c59 100644
> --- a/tools/usb/testusb.c
> +++ b/tools/usb/testusb.c
> @@ -358,6 +358,7 @@ static const char *usbfs_dir_find(void) {
> static char usbfs_path_0[] = "/dev/usb/devices";
> static char usbfs_path_1[] = "/proc/bus/usb/devices";
> + static char udev_usb_path[] = "/dev/bus/usb";
>
> static char *const usbfs_paths[] = {
> usbfs_path_0, usbfs_path_1
> @@ -376,6 +377,10 @@ static const char *usbfs_dir_find(void)
> }
> } while (++it != end);
>
> + /* real device-nodes managed by udev */
> + if (access(udev_usb_path, F_OK) == 0)
> + return udev_usb_path;
> +
> return NULL;
> }
>
Two issues with this: F_OK only guarantees that the path exists, it does not guarantee that it is readable like this function guarantees for usbfs_paths, and access() shouldn't be used because of its security implications, you're better off using open() and testing for fd.
Hello, David. I think this function doesn't need check the permission. What this function need do is to find the usbfs mount point. If the path exists but cannot read, we cannot report as it's not exits. And when we read files, open() will return an error and user can check if it's need to upgrade his access right.
On Tue, 22 May 2012, Du, ChangbinX wrote:
> > diff --git a/tools/usb/testusb.c b/tools/usb/testusb.c index
> > 6e0f567..82d7c59 100644
> > --- a/tools/usb/testusb.c
> > +++ b/tools/usb/testusb.c
> > @@ -358,6 +358,7 @@ static const char *usbfs_dir_find(void) {
> > static char usbfs_path_0[] = "/dev/usb/devices";
> > static char usbfs_path_1[] = "/proc/bus/usb/devices";
> > + static char udev_usb_path[] = "/dev/bus/usb";
> >
> > static char *const usbfs_paths[] = {
> > usbfs_path_0, usbfs_path_1
> > @@ -376,6 +377,10 @@ static const char *usbfs_dir_find(void)
> > }
> > } while (++it != end);
> >
> > + /* real device-nodes managed by udev */
> > + if (access(udev_usb_path, F_OK) == 0)
> > + return udev_usb_path;
> > +
> > return NULL;
> > }
> >
>
> Two issues with this: F_OK only guarantees that the path exists, it
> does not guarantee that it is readable like this function guarantees for
> usbfs_paths, and access() shouldn't be used because of its security
> implications, you're better off using open() and testing for fd.
>
> Hello, David. I think this function doesn't need check the permission.
> What this function need do is to find the usbfs mount point. If the path
> exists but cannot read, we cannot report as it's not exits. And when we
> read files, open() will return an error and user can check if it's need
> to upgrade his access right.
>
Your email client doesn't seem to be quoting messages correctly :)
Anyway, this patch is inconsistent with the rest of the function. The
other two paths are checked with open(O_RDONLY) followed by a close() to
address my second comment and we certainly wouldn't want to return a path
that exists by is unreadable: we'd want to fallback to one of the other
possibilities. So if anybody is going to extend this in the future like
you have, it would be possible to return /dev/bus/usb even though we can't
read it. That's a bad result.
Please consider doing it the proper way: by doing open(O_RDONLY), close()
instead of access() -- if you don't understand why, read access(2) -- and
in an extendable way.
> On Tue, 22 May 2012, Du, ChangbinX wrote:
>
> > > diff --git a/tools/usb/testusb.c b/tools/usb/testusb.c index
> > > 6e0f567..82d7c59 100644
> > > --- a/tools/usb/testusb.c
> > > +++ b/tools/usb/testusb.c
> > > @@ -358,6 +358,7 @@ static const char *usbfs_dir_find(void) {
> > > static char usbfs_path_0[] = "/dev/usb/devices";
> > > static char usbfs_path_1[] = "/proc/bus/usb/devices";
> > > + static char udev_usb_path[] = "/dev/bus/usb";
> > >
> > > static char *const usbfs_paths[] = {
> > > usbfs_path_0, usbfs_path_1
> > > @@ -376,6 +377,10 @@ static const char *usbfs_dir_find(void)
> > > }
> > > } while (++it != end);
> > >
> > > + /* real device-nodes managed by udev */
> > > + if (access(udev_usb_path, F_OK) == 0)
> > > + return udev_usb_path;
> > > +
> > > return NULL;
> > > }
> > >
> >
> > Two issues with this: F_OK only guarantees that the path exists, it
> > does not guarantee that it is readable like this function guarantees
> > for usbfs_paths, and access() shouldn't be used because of its
> > security implications, you're better off using open() and testing for fd.
> >
> > Hello, David. I think this function doesn't need check the permission.
> > What this function need do is to find the usbfs mount point. If the
> > path exists but cannot read, we cannot report as it's not exits. And
> > when we read files, open() will return an error and user can check if
> > it's need to upgrade his access right.
> >
>
> Your email client doesn't seem to be quoting messages correctly :)
>
> Anyway, this patch is inconsistent with the rest of the function. The other two
> paths are checked with open(O_RDONLY) followed by a close() to address my
> second comment and we certainly wouldn't want to return a path that exists by
> is unreadable: we'd want to fallback to one of the other possibilities. So if
> anybody is going to extend this in the future like you have, it would be possible
> to return /dev/bus/usb even though we can't read it. That's a bad result.
>
> Please consider doing it the proper way: by doing open(O_RDONLY), close()
> instead of access() -- if you don't understand why, read access(2) -- and in an
> extendable way.
I see. Thanks for your comments. I will correct and resend it again.