You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/01/26 12:58:41 UTC
[GitHub] [incubator-nuttx] pkarashchenko opened a new pull request #5347: register_driver: fix driver modes accross the code
pkarashchenko opened a new pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347
## Summary
State of problem:
- Some drivers that do not have write handlers are registered
with write permissions
- Some drivers that do not have read handlers are registered
with read permissions
- Some drivers are registered with execute permissions
Solution:
- Iterate code where register_driver() is used and change 'mode'
parameter to reflect the actual read/write handlers described by
'struct file_operations'
- Remove execute permissions from 'mode' parameter
## Impact
All over the project and apps
## Testing
Pass CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5347: register_driver: fix driver modes accross the code
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347#discussion_r793218631
##########
File path: arch/arm/src/cxd56xx/cxd56_sph.c
##########
@@ -240,7 +240,7 @@ static inline int cxd56_sphdevinit(FAR const char *devname, int num)
snprintf(fullpath, sizeof(fullpath), "/dev/%s%d", devname, num);
- ret = register_driver(fullpath, &sph_fops, 0666, (FAR void *)priv);
+ ret = register_driver(fullpath, &sph_fops, 0000, (FAR void *)priv);
Review comment:
We need consider ioctl too, actually Linux define ioctl with read/write in the encoding to consolidate the the permission check in one place:
https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a change in pull request #5347: register_driver: fix driver modes accross the code
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347#discussion_r794854436
##########
File path: arch/arm/src/cxd56xx/cxd56_sph.c
##########
@@ -240,7 +240,7 @@ static inline int cxd56_sphdevinit(FAR const char *devname, int num)
snprintf(fullpath, sizeof(fullpath), "/dev/%s%d", devname, num);
- ret = register_driver(fullpath, &sph_fops, 0666, (FAR void *)priv);
+ ret = register_driver(fullpath, &sph_fops, 0000, (FAR void *)priv);
Review comment:
I think currently these flags are not enforced on NuttX, but since we get multi-user support it will be necessary to use the right access mode flag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5347: register_driver: fix driver modes accross the code
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347#discussion_r793417685
##########
File path: drivers/analog/dac.c
##########
@@ -224,16 +222,6 @@ static int dac_close(FAR struct file *filep)
return ret;
}
-/****************************************************************************
- * Name: dac_read
- ****************************************************************************/
-
-static ssize_t dac_read(FAR struct file *filep, FAR char *buffer,
- size_t buflen)
-{
- return 0;
-}
-
Review comment:
The driver is registered with `register_driver(path, &dac_fops, 0222, dev);` -- write only
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a change in pull request #5347: register_driver: fix driver modes accross the code
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347#discussion_r793001727
##########
File path: drivers/analog/dac.c
##########
@@ -224,16 +222,6 @@ static int dac_close(FAR struct file *filep)
return ret;
}
-/****************************************************************************
- * Name: dac_read
- ****************************************************************************/
-
-static ssize_t dac_read(FAR struct file *filep, FAR char *buffer,
- size_t buflen)
-{
- return 0;
-}
-
Review comment:
Good finding! I think a users will not read from DAC device! :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5347: register_driver: fix driver modes accross the code
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347#discussion_r793415499
##########
File path: arch/arm/src/cxd56xx/cxd56_sph.c
##########
@@ -240,7 +240,7 @@ static inline int cxd56_sphdevinit(FAR const char *devname, int num)
snprintf(fullpath, sizeof(fullpath), "/dev/%s%d", devname, num);
- ret = register_driver(fullpath, &sph_fops, 0666, (FAR void *)priv);
+ ret = register_driver(fullpath, &sph_fops, 0000, (FAR void *)priv);
Review comment:
I tried to use ioctl with drivers that are registered with `0000` mode and that works like a charm. I noticed the mode issue while I was working on https://github.com/apache/incubator-nuttx/pull/5345
The driver is registered with `register_driver(devpath, &automount_fops, 0000, priv);` and test code does
```
int fd = open("/var/mount/automount0", 0);
if (fd > 0)
{
...
ret = ioctl(fd, FIOC_NOTIFY,
(unsigned long)((uintptr_t)&automountevents));
if (ret < 0)
{
int errcode = errno;
ferr("ERROR: ioctl(FIOC_NOTIFY) failed: %d\n",
errcode);
}
else
{
/* Success */
}
}
```
The `ioctl` operation is always successful.
Maybe you can describe use case for your concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5347: register_driver: fix driver modes accross the code
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347#discussion_r806208692
##########
File path: arch/arm/src/cxd56xx/cxd56_sph.c
##########
@@ -240,7 +240,7 @@ static inline int cxd56_sphdevinit(FAR const char *devname, int num)
snprintf(fullpath, sizeof(fullpath), "/dev/%s%d", devname, num);
- ret = register_driver(fullpath, &sph_fops, 0666, (FAR void *)priv);
+ ret = register_driver(fullpath, &sph_fops, 0000, (FAR void *)priv);
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5347: register_driver: fix driver modes accross the code
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347#discussion_r793021938
##########
File path: arch/arm/src/cxd56xx/cxd56_sph.c
##########
@@ -240,7 +240,7 @@ static inline int cxd56_sphdevinit(FAR const char *devname, int num)
snprintf(fullpath, sizeof(fullpath), "/dev/%s%d", devname, num);
- ret = register_driver(fullpath, &sph_fops, 0666, (FAR void *)priv);
+ ret = register_driver(fullpath, &sph_fops, 0000, (FAR void *)priv);
Review comment:
Both `sph_fops.read` and `sph_fops.write` are `NULL` pointers. I do not think that read permission is needed. But maybe I'm lacking a vision of many things. `open` with mode `0` works perfectly, so this is not a limitation. I made this PR as draft and will initiate additional discussion over e-mail to get more input.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5347: register_driver: fix driver modes accross the code
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347#discussion_r806208692
##########
File path: arch/arm/src/cxd56xx/cxd56_sph.c
##########
@@ -240,7 +240,7 @@ static inline int cxd56_sphdevinit(FAR const char *devname, int num)
snprintf(fullpath, sizeof(fullpath), "/dev/%s%d", devname, num);
- ret = register_driver(fullpath, &sph_fops, 0666, (FAR void *)priv);
+ ret = register_driver(fullpath, &sph_fops, 0000, (FAR void *)priv);
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5347: register_driver: fix driver modes accross the code
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347#discussion_r793418758
##########
File path: arch/arm/src/cxd56xx/cxd56_sph.c
##########
@@ -240,7 +240,7 @@ static inline int cxd56_sphdevinit(FAR const char *devname, int num)
snprintf(fullpath, sizeof(fullpath), "/dev/%s%d", devname, num);
- ret = register_driver(fullpath, &sph_fops, 0666, (FAR void *)priv);
+ ret = register_driver(fullpath, &sph_fops, 0000, (FAR void *)priv);
Review comment:
I use `0000` for drivers that have neither `read` nor `write` interface
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5347: register_driver: fix driver modes accross the code
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347#discussion_r793415499
##########
File path: arch/arm/src/cxd56xx/cxd56_sph.c
##########
@@ -240,7 +240,7 @@ static inline int cxd56_sphdevinit(FAR const char *devname, int num)
snprintf(fullpath, sizeof(fullpath), "/dev/%s%d", devname, num);
- ret = register_driver(fullpath, &sph_fops, 0666, (FAR void *)priv);
+ ret = register_driver(fullpath, &sph_fops, 0000, (FAR void *)priv);
Review comment:
I tried to use ioctl with drivers that are registered with `0000` mode and that works like a charm. I noticed the mode issue while I was working on https://github.com/apache/incubator-nuttx/pull/5345
The driver is registered with `register_driver(devpath, &automount_fops, 0000, priv);` and test code does
```
int fd = open("/var/mount/automount0", 0);
if (fd > 0)
{
...
ret = ioctl(fd, FIOC_NOTIFY,
(unsigned long)((uintptr_t)&automountevents));
if (ret < 0)
{
int errcode = errno;
ferr("ERROR: ioctl(FIOC_NOTIFY) failed: %d\n",
errcode);
}
else
{
/* Success */
}
}
```
The `ioctl` operation is always successful.
Maybe you can describe use case for your concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #5347: register_driver: fix driver modes accross the code
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5347: register_driver: fix driver modes accross the code
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347#discussion_r795017536
##########
File path: arch/arm/src/cxd56xx/cxd56_sph.c
##########
@@ -240,7 +240,7 @@ static inline int cxd56_sphdevinit(FAR const char *devname, int num)
snprintf(fullpath, sizeof(fullpath), "/dev/%s%d", devname, num);
- ret = register_driver(fullpath, &sph_fops, 0666, (FAR void *)priv);
+ ret = register_driver(fullpath, &sph_fops, 0000, (FAR void *)priv);
Review comment:
Yes. I will rework changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5347: register_driver: fix driver modes accross the code
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347#discussion_r792973010
##########
File path: arch/arm/src/cxd56xx/cxd56_sph.c
##########
@@ -240,7 +240,7 @@ static inline int cxd56_sphdevinit(FAR const char *devname, int num)
snprintf(fullpath, sizeof(fullpath), "/dev/%s%d", devname, num);
- ret = register_driver(fullpath, &sph_fops, 0666, (FAR void *)priv);
+ ret = register_driver(fullpath, &sph_fops, 0000, (FAR void *)priv);
Review comment:
should the device have read permission at least?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5347: register_driver: fix driver modes accross the code
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5347:
URL: https://github.com/apache/incubator-nuttx/pull/5347#discussion_r793415499
##########
File path: arch/arm/src/cxd56xx/cxd56_sph.c
##########
@@ -240,7 +240,7 @@ static inline int cxd56_sphdevinit(FAR const char *devname, int num)
snprintf(fullpath, sizeof(fullpath), "/dev/%s%d", devname, num);
- ret = register_driver(fullpath, &sph_fops, 0666, (FAR void *)priv);
+ ret = register_driver(fullpath, &sph_fops, 0000, (FAR void *)priv);
Review comment:
I tried to use ioctl with drivers that are registered with `0000` mode and that works like a charm. I noticed the mode issue while I was working on https://github.com/apache/incubator-nuttx/pull/5345
The driver is registered with `register_driver(devpath, &automount_fops, 0000, priv);` and test code does
```
int fd = open("/var/mount/automount0", 0);
if (fd > 0)
{
...
ret = ioctl(fd, FIOC_NOTIFY,
(unsigned long)((uintptr_t)&automountevents));
if (ret < 0)
{
int errcode = errno;
ferr("ERROR: ioctl(FIOC_NOTIFY) failed: %d\n",
errcode);
}
else
{
/* Success */
}
}
```
The `ioctl` operation is always successful.
Maybe you can describe use case for your concern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org