One option, conforming standard, would be that you just always give O_RDWR (same flags as what linux devices have), but then when calling read/write you check if the pointer is non-null. If the driver doesn't define read or write, those operations are allowed on the device, but act as no-op.
- Jukka Jukka Laitinen kirjoitti perjantai 1. huhtikuuta 2022: > In my opinion 0, if you are asking that, but it is strictly not conforming > the standard. > > Posix says that "Applications shall specify exactly one of the first > five...", so there is no correct standard conforming way. All five would be > wrong, imho. > > -Jukka > > Petro Karashchenko kirjoitti perjantai 1. huhtikuuta 2022: > > Yes Jukka what you are saying is absolutely correct. The main item under > > discussion are the drivers that are pure ioctl (that means do not have > > neither read nor write handlers). Should RD or WR flag be passed to open > > call in such case of or 0 should be passed. > > > > Best regards, > > Petro > > > > On Fri, Apr 1, 2022, 6:54 PM Jukka Laitinen <[email protected]> wrote: > > > > > Different posix implementations have different values for these flags, so > > > I think it is ok not to have the same as what linux has. > > > > > > Posix (2017) specifies thar exactly one of the following is provided for > > > open: O_EXEC, O_RDWR, O_RDONLY, O_SEARCH and O_WRONLY, and other flags are > > > bitwise OR'd to that. The spec says nothing about that you can just give > > > "0" afaik, it just happens to be that in Linux O_RDONLY happens to be 0. > > > > > > Maybe just fix the open having O_RDONLY in places where you really open > > > the file as read-only, O_WRONLY for write only and O? That should be > > > according to the standard at least. > > > > > > Just my 2 cents > > > > > > - Jukka > > > > > > > > > > > > Alan Carvalho de Assis kirjoitti perjantai 1. huhtikuuta 2022: > > > > More about it here: > > > > > > > https://stackoverflow.com/questions/61923703/how-to-make-sense-of-o-rdonly-0 > > > > > > > > So, I agree with the comment that said "calling it flag is misnomer > > > > and misleading" > > > > > > > > On Unix/Linux O_RDONLY | O_WRONLY != O_RDWR, on NuttX is it explicitly > > > this way: > > > > > > > > #define O_RDWR (O_RDOK|O_WROK) /* Open for both read & write access > > > */ > > > > > > > > Now, I'm also confuse about the right thing to do: > > > > > > > > 1) Fix include/fcntl.h to follow Unix/Linux > > > > 2) Keep things we they are and don't accept opening a file if it > > > > doesn't have RDONLY flag (and change all the drivers, event ioctl only > > > > drivers, to include reading flag) > > > > 3) Remove the flag checking. > > > > > > > > Probably we should do 1) because NuttX follows Unix/Linux approach, > > > > although I agree that Unix/Linux are completely non-sense on this > > > > subject, oflag should be a flag like it is on NuttX :-) > > > > > > > > BR, > > > > > > > > Alan > > > > > > > > On 4/1/22, Alan Carvalho de Assis <[email protected]> wrote: > > > > > Hmm, I think oflag equal 0 on Unix(so MacOS)/Linux works because of > > > > > it: > > > > > > > > > > #define O_RDONLY 00000000 > > > > > > > > > > So, in fact on Linux/Mac when we are opening a file with oflag 0 we > > > > > are opening it for reading only. > > > > > > > > > > On NuttX the value 0 is not defined, O_RDONLY is 1: > > > > > > > > > > #define O_RDONLY (1 << 0) > > > > > > > > > > BR, > > > > > > > > > > Alan > > > > > > > > > > On 4/1/22, Petro Karashchenko <[email protected]> wrote: > > > > >> Hello, > > > > >> > > > > >> Here I'm talking not about driver registration permission, but more > > > about > > > > >> the "oflag" parameter to "open()" call. > > > > >> > > > > >> I just tried a quick example on MAC > > > > >> > > > > >> #include <fcntl.h> > > > > >> #include <stdio.h> > > > > >> > > > > >> int main(void) > > > > >> { > > > > >> int fd = open("test.txt", 0); > > > > >> if (fd < 0) > > > > >> printf("A\n"); > > > > >> else > > > > >> printf("B\n"); > > > > >> > > > > >> return 0; > > > > >> } > > > > >> > > > > >> The "B" is printed if the file exists. If you know the system that > > > will > > > > >> run > > > > >> this sample code and will print "A", please let me know. > > > > >> > > > > >> Best regards, > > > > >> Petro > > > > >> > > > > >> пт, 1 квіт. 2022 р. о 16:27 Alan Carvalho de Assis <[email protected] > > > > > > > > >> пише: > > > > >> > > > > >>> I think the device file shouldn't be created with permission 000. > > > > >>> > > > > >>> Look inside your Linux /dev all device files have RW permission for > > > > >>> root, some give only R for group and others. > > > > >>> > > > > >>> So, probably we need to fix the device register creation, not > > > removing > > > > >>> the flag check. > > > > >>> > > > > >>> BR, > > > > >>> > > > > >>> Alan > > > > >>> > > > > >>> On 4/1/22, Xiang Xiao <[email protected]> wrote: > > > > >>> > It's better to check ioctl callback too since ioctl means the > > > driver > > > > >>> > has > > > > >>> > the compatibility of read(i)and write(o). > > > > >>> > > > > > >>> > On Fri, Apr 1, 2022 at 9:15 PM Petro Karashchenko < > > > > >>> > [email protected]> wrote: > > > > >>> > > > > > >>> >> So Alan do you suggest to remove inode_checkflags? > > > > >>> >> > > > > >>> >> On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis > > > > >>> >> <[email protected]> > > > > >>> >> wrote: > > > > >>> >> > > > > >>> >> > Hi Petro, > > > > >>> >> > > > > > >>> >> > I saw your PR #1117 but I think opening a device file with flag > > > 0 > > > > >>> >> > is > > > > >>> >> > not correct, please see the open man-pages: > > > > >>> >> > > > > > >>> >> > alan@dev:/tmp$ man 2 open > > > > >>> >> > > > > > >>> >> > The argument flags must include one of the following > > > > >>> >> > access > > > > >>> >> > modes: O_RDONLY, O_WRONLY, or > > > > >>> >> > O_RDWR. These request opening the file read-only, > > > > >>> >> > write-only, > > > > >>> >> > or read/write, respectively. > > > > >>> >> > > > > > >>> >> > Also the opengroup say something similar: > > > > >>> >> > > > > > >>> >> > > > > https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html > > > > >>> >> > > > > > >>> >> > "Values for oflag are constructed by a bitwise-inclusive OR of > > > > >>> >> > flags > > > > >>> >> > from the following list, defined in <fcntl.h>. Applications > > > shall > > > > >>> >> > specify exactly one of the first five values (file access > > > > >>> >> > modes) > > > > >>> >> > below > > > > >>> >> > in the value of oflag:" > > > > >>> >> > > > > > >>> >> > The man pages uses "MUST", the OpenGroups uses "SHALL", but > > > > >>> >> > according > > > > >>> >> > to RFC2119 they are equivalents: > > > > >>> >> > > > > > >>> >> > https://www.ietf.org/rfc/rfc2119.txt > > > > >>> >> > > > > > >>> >> > BR, > > > > >>> >> > > > > > >>> >> > Alan > > > > >>> >> > > > > > >>> >> > On 4/1/22, Petro Karashchenko <[email protected]> > > > wrote: > > > > >>> >> > > Hi, > > > > >>> >> > > > > > > >>> >> > > I want to resume this thread again because after reexamined > > > code > > > > >>> >> > carefully > > > > >>> >> > > I found that VFS layer has an API > > > > >>> >> > > > > > > >>> >> > > int inode_checkflags(FAR struct inode *inode, int oflags) > > > > >>> >> > > { > > > > >>> >> > > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) || > > > > >>> >> > > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write)) > > > > >>> >> > > { > > > > >>> >> > > return -EACCES; > > > > >>> >> > > } > > > > >>> >> > > else > > > > >>> >> > > { > > > > >>> >> > > return OK; > > > > >>> >> > > } > > > > >>> >> > > } > > > > >>> >> > > > > > > >>> >> > > That checks if read and write handlers are available, so all > > > our > > > > >>> >> > discussion > > > > >>> >> > > about R/W mode for IOCTL does not make any sense. We either > > > need > > > > >>> >> > > to > > > > >>> >> > remove > > > > >>> >> > > this check or register VFS nodes with proper permissions and > > > open > > > > >>> >> > > files > > > > >>> >> > > with correct flags. So if the driver does not have neither > > > read > > > > >>> >> > > nor > > > > >>> >> write > > > > >>> >> > > handlers the "0000" mode should be used and "0" should be > > > > >>> >> > > used > > > > >>> during > > > > >>> >> > > opening of a file. Or we need to remove "inode_checkflags()". > > > > >>> >> > > > > > > >>> >> > > Best regards, > > > > >>> >> > > Petro > > > > >>> >> > > > > > > >>> >> > > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko > > > > >>> >> > > <[email protected]> > > > > >>> >> > > пише: > > > > >>> >> > > > > > > >>> >> > >> I see. Thank you for the feedback. I will rework changes to > > > get > > > > >>> back > > > > >>> >> > >> read permissions. > > > > >>> >> > >> > > > > >>> >> > >> Best regards, > > > > >>> >> > >> Petro > > > > >>> >> > >> > > > > >>> >> > >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis > > > > >>> >> > >> <[email protected] > > > > >>> >> > > > > > >>> >> > >> пише: > > > > >>> >> > >> > > > > > >>> >> > >> > Hi Petro, > > > > >>> >> > >> > > > > > >>> >> > >> > The read permission is needed even when you just want to > > > open > > > > >>> >> > >> > a > > > > >>> >> file: > > > > >>> >> > >> > > > > > >>> >> > >> > $ vim noreadfile > > > > >>> >> > >> > > > > > >>> >> > >> > $ chmod 0000 noreadfile > > > > >>> >> > >> > > > > > >>> >> > >> > $ ls -l noreadfile > > > > >>> >> > >> > ---------- 1 user user 5 jan 28 09:24 noreadfile > > > > >>> >> > >> > > > > > >>> >> > >> > $ cat noreadfile > > > > >>> >> > >> > cat: noreadfile: Permission denied > > > > >>> >> > >> > > > > > >>> >> > >> > You can even try to create a C program just to open it, > > > and it > > > > >>> >> > >> > will > > > > >>> >> > >> > fail. > > > > >>> >> > >> > > > > > >>> >> > >> > See the man page of open function: > > > > >>> >> > >> > > > > > >>> >> > >> > The argument flags *must* include one of the > > > following > > > > >>> >> access > > > > >>> >> > >> > modes: O_RDONLY, O_WRONLY, or > > > > >>> >> > >> > O_RDWR. These request opening the file read-only, > > > > >>> >> write-only, > > > > >>> >> > >> > or read/write, respectively. > > > > >>> >> > >> > > > > > >>> >> > >> > This man page makes it clear you must include an access > > > mode, > > > > >>> >> > >> > but > > > > >>> >> > >> > I > > > > >>> >> > >> > passed 0 to the access mode flag of open() and it was > > > > >>> >> > >> > accepted, > > > > >>> >> > >> > but > > > > >>> >> > >> > when the file has permission 0000 it returns -EPERM: > > > "Failed > > > > >>> >> > >> > to > > > > >>> >> > >> > open > > > > >>> >> > >> > file: error -1" > > > > >>> >> > >> > > > > > >>> >> > >> > BR, > > > > >>> >> > >> > > > > > >>> >> > >> > Alan > > > > >>> >> > >> > > > > > >>> >> > >> > On 1/28/22, Petro Karashchenko < > > > [email protected]> > > > > >>> >> wrote: > > > > >>> >> > >> > > Hello, > > > > >>> >> > >> > > > > > > >>> >> > >> > > Yes, but how does this relate to "0000" mode for > > > > >>> >> > "register_driver()"? > > > > >>> >> > >> > > Maybe you can describe some use case so it will become > > > more > > > > >>> >> > >> > > clear? > > > > >>> >> > >> > > Currently ioctl works fine if driver is registered with > > > > >>> >> > >> > > "0000" > > > > >>> >> > >> permission > > > > >>> >> > >> > > mode. > > > > >>> >> > >> > > > > > > >>> >> > >> > > Best regards, > > > > >>> >> > >> > > Petro > > > > >>> >> > >> > > > > > > >>> >> > >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao > > > > >>> >> > >> > > <[email protected] > > > > >>> >> > > > > > >>> >> > >> пише: > > > > >>> >> > >> > >> > > > > >>> >> > >> > >> If we want to do the correct permission check, the > > > > >>> >> > >> > >> ioctl > > > > >>> >> > >> > >> handler > > > > >>> >> > >> needs to > > > > >>> >> > >> > >> check R/W bit by itself based on how the ioctl is > > > > >>> >> > >> > >> implemented. > > > > >>> >> > >> > >> Or follow up how Linux encode the needed permission > > > > >>> >> > >> > >> into > > > > >>> >> > >> > >> each > > > > >>> >> > IOCTL: > > > > >>> >> > >> > >> > > > > >>> >> > >> > > > > >>> >> > > > > > >>> >> > > > > >>> > > > https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91 > > > > >>> >> > >> > >> and let's VFS layer do the check for each driver. > > > > >>> >> > >> > >> > > > > >>> >> > >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko < > > > > >>> >> > >> > >> [email protected]> wrote: > > > > >>> >> > >> > >> > > > > >>> >> > >> > >> > Hello team, > > > > >>> >> > >> > >> > > > > > >>> >> > >> > >> > Recently I have noticed that there are many places in > > > > >>> >> > >> > >> > code > > > > >>> >> where > > > > >>> >> > >> > >> > register_driver() is called with non-zero mode with > > > file > > > > >>> >> > operation > > > > >>> >> > >> > >> > structures that have neither read nor write APIs > > > > >>> implemented. > > > > >>> >> For > > > > >>> >> > >> > >> > example "ret = register_driver(path, &opamp_fops, > > > 0444, > > > > >>> >> > >> > >> > dev);" > > > > >>> >> > >> > >> > while > > > > >>> >> > >> > >> > opamp_fops has only "opamp_open", "opamp_close" and > > > > >>> >> "opamp_ioctl" > > > > >>> >> > >> > >> > implemented. I made a PR to fix it > > > > >>> >> > >> > >> > https://github.com/apache/incubator-nuttx/pull/5347 > > > and > > > > >>> >> > >> > >> > change > > > > >>> >> > >> > >> > mode > > > > >>> >> > >> > >> > from "0444" to "0000", but want to ask if anyone sees > > > any > > > > >>> >> > drawback > > > > >>> >> > >> in > > > > >>> >> > >> > >> > such an approach? Maybe I'm missing something? > > > > >>> >> > >> > >> > > > > > >>> >> > >> > >> > Best regards, > > > > >>> >> > >> > >> > Petro > > > > >>> >> > >> > >> > > > > > >>> >> > >> > > > > > > >>> >> > >> > > > > >>> >> > > > > > > >>> >> > > > > > >>> >> > > > > >>> > > > > > >>> > > > > >> > > > > > > > > > >
