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 2020/05/04 09:05:52 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
xiaoxiang781216 opened a new pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965
Signed-off-by: Xiang Xiao <xi...@xiaomi.com>
## Summary
## Impact
## Testing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#discussion_r420120217
##########
File path: syscall/syscall_stublookup.c
##########
@@ -189,13 +189,9 @@ uintptr_t STUB_nx_vsyslog(int nbr, uintptr_t parm1, uintptr_t parm2,
*/
uintptr_t STUB_close(int nbr, uintptr_t parm1);
-#ifdef CONFIG_LIBC_IOCTL_VARIADIC
-uintptr_t STUB_fs_ioctl(int nbr, uintptr_t parm1, uintptr_t parm2,
- uintptr_t parm3);
-#else
uintptr_t STUB_ioctl(int nbr, uintptr_t parm1, uintptr_t parm2,
- uintptr_t parm3);
-#endif
+ uintptr_t parm3, uintptr_t parm4, uintptr_t parm5,
+ uintptr_t parm6);
Review comment:
This is terrible. I am very strongly opposed to this and will not merge this PR with this kind of code. Very bad.
Please follow the model of syslog, vsyslog, and nx_vsyslog. That will work. This is bad.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#discussion_r420170575
##########
File path: syscall/syscall.csv
##########
@@ -42,7 +41,7 @@
"if_indextoname","net/if.h","defined(CONFIG_NETDEV_IFINDEX)","FAR char *","unsigned int","FAR char *"
"if_nametoindex","net/if.h","defined(CONFIG_NETDEV_IFINDEX)","unsigned int","FAR const char *"
"insmod","nuttx/module.h","defined(CONFIG_MODULE)","FAR void *","FAR const char *","FAR const char *"
-"ioctl","sys/ioctl.h","!defined(CONFIG_LIBC_IOCTL_VARIADIC)","int","int","int","unsigned long"
+"ioctl","sys/ioctl.h","","int","int","int","..."
Review comment:
Since all these functions just accept one more argument from standard, we can modify mksyscall not to hard code the max number to 7 instead geting it from syscall.cvs like this:
```
"open","fcntl.h","","int","const char*","int","...1"
```
and change STUB_open to:
```
SYSCALL_LOOKUP(open, 3, STUB_open)
uintptr_t STUB_open(int nbr, uintptr_t parm1, uintptr_t parm2,
uintptr_t parm3);
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#issuecomment-624091310
Well, I did merge this because the precedent. But those are all very bad designs. Passing 6 arguments on every stub call is very inefficient. And how does the ... get converted into the parameters? I think it is wrong.
I wrong those old open, mq_open, etc. calls long time ago. Perhaps I knew something that that I don't know know. Or perhaps the mode parameter is not being propagated correctly? We will find out if ioctl works this this terrible design soon.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#issuecomment-626067622
>Hi @xiaoxiang781216,
>
>I've just noticed that the following commit affects dhcp client.
I found the root cause. I'll send another PR later.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#discussion_r420196167
##########
File path: syscall/syscall.csv
##########
@@ -42,7 +41,7 @@
"if_indextoname","net/if.h","defined(CONFIG_NETDEV_IFINDEX)","FAR char *","unsigned int","FAR char *"
"if_nametoindex","net/if.h","defined(CONFIG_NETDEV_IFINDEX)","unsigned int","FAR const char *"
"insmod","nuttx/module.h","defined(CONFIG_MODULE)","FAR void *","FAR const char *","FAR const char *"
-"ioctl","sys/ioctl.h","!defined(CONFIG_LIBC_IOCTL_VARIADIC)","int","int","int","unsigned long"
+"ioctl","sys/ioctl.h","","int","int","int","..."
Review comment:
Yes, this is what come up my mind, we can even drop 1 from the list like this:
```
"open","fcntl.h","","int","const char*","int","...","mode_t"
```
The ... just indicate the arguments after ... need get from va_xxx function indirectly.
The support of old form should be removed to avoid the wrong usage in the furture. The function with unlimited number/type arguments must pass the argumnet through va_list* like syslog.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#issuecomment-624096579
> Thanks for merge. Anyway, we can change to the implementation of syslog in a batch.
I still think that the change is bad. But since there is a precedent and special logic in mksyscall.c, I suppose we can live with it for now.
We can do better, however. vsyslog is a better design. It does not suffer from the ineffiencies and problems with the typing that open, mq_open, ... and now ioctl have.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#discussion_r420148898
##########
File path: syscall/syscall_stublookup.c
##########
@@ -189,13 +189,9 @@ uintptr_t STUB_nx_vsyslog(int nbr, uintptr_t parm1, uintptr_t parm2,
*/
uintptr_t STUB_close(int nbr, uintptr_t parm1);
-#ifdef CONFIG_LIBC_IOCTL_VARIADIC
-uintptr_t STUB_fs_ioctl(int nbr, uintptr_t parm1, uintptr_t parm2,
- uintptr_t parm3);
-#else
uintptr_t STUB_ioctl(int nbr, uintptr_t parm1, uintptr_t parm2,
- uintptr_t parm3);
-#endif
+ uintptr_t parm3, uintptr_t parm4, uintptr_t parm5,
+ uintptr_t parm6);
Review comment:
The all of those are wrong too.
I will not merge this PR with that implementation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#discussion_r420144812
##########
File path: syscall/syscall.csv
##########
@@ -42,7 +41,7 @@
"if_indextoname","net/if.h","defined(CONFIG_NETDEV_IFINDEX)","FAR char *","unsigned int","FAR char *"
"if_nametoindex","net/if.h","defined(CONFIG_NETDEV_IFINDEX)","unsigned int","FAR const char *"
"insmod","nuttx/module.h","defined(CONFIG_MODULE)","FAR void *","FAR const char *","FAR const char *"
-"ioctl","sys/ioctl.h","!defined(CONFIG_LIBC_IOCTL_VARIADIC)","int","int","int","unsigned long"
+"ioctl","sys/ioctl.h","","int","int","int","..."
Review comment:
It must work, otherwise how open can work with the same impelemntation?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#issuecomment-623523930
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#discussion_r420185363
##########
File path: syscall/syscall.csv
##########
@@ -42,7 +41,7 @@
"if_indextoname","net/if.h","defined(CONFIG_NETDEV_IFINDEX)","FAR char *","unsigned int","FAR char *"
"if_nametoindex","net/if.h","defined(CONFIG_NETDEV_IFINDEX)","unsigned int","FAR const char *"
"insmod","nuttx/module.h","defined(CONFIG_MODULE)","FAR void *","FAR const char *","FAR const char *"
-"ioctl","sys/ioctl.h","!defined(CONFIG_LIBC_IOCTL_VARIADIC)","int","int","int","unsigned long"
+"ioctl","sys/ioctl.h","","int","int","int","..."
Review comment:
> Since all these functions just accept one more argument from standard, we can modify mksyscall not to hard code the max number to 7 instead geting it from syscall.cvs like this:
>
> ```
> "open","fcntl.h","","int","const char*","int","...1"
I think that is a good idea. That would greatly improve performance. It doesn't resolve the issue of the type of the argument; it would still assume that it is a uintptr_t even though in reality it is a mode_t or unsigned int.
That would not work for something like syslog() which has an arbitrarily long list of arguments of various types, perhaps as many as 20 or more.
"..." must always be the last argument. So perhaps "...N" could be followed by a list of N types like: "...1","mode_t" could mean that there is at most one argument of type mode_t. That would also solve the typing issue.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#discussion_r420123122
##########
File path: syscall/syscall_stublookup.c
##########
@@ -189,13 +189,9 @@ uintptr_t STUB_nx_vsyslog(int nbr, uintptr_t parm1, uintptr_t parm2,
*/
uintptr_t STUB_close(int nbr, uintptr_t parm1);
-#ifdef CONFIG_LIBC_IOCTL_VARIADIC
-uintptr_t STUB_fs_ioctl(int nbr, uintptr_t parm1, uintptr_t parm2,
- uintptr_t parm3);
-#else
uintptr_t STUB_ioctl(int nbr, uintptr_t parm1, uintptr_t parm2,
- uintptr_t parm3);
-#endif
+ uintptr_t parm3, uintptr_t parm4, uintptr_t parm5,
+ uintptr_t parm6);
Review comment:
It is amateurish and inefficient and the worst solution to the problem that I could possible imagine.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#discussion_r420143499
##########
File path: syscall/syscall_stublookup.c
##########
@@ -189,13 +189,9 @@ uintptr_t STUB_nx_vsyslog(int nbr, uintptr_t parm1, uintptr_t parm2,
*/
uintptr_t STUB_close(int nbr, uintptr_t parm1);
-#ifdef CONFIG_LIBC_IOCTL_VARIADIC
-uintptr_t STUB_fs_ioctl(int nbr, uintptr_t parm1, uintptr_t parm2,
- uintptr_t parm3);
-#else
uintptr_t STUB_ioctl(int nbr, uintptr_t parm1, uintptr_t parm2,
- uintptr_t parm3);
-#endif
+ uintptr_t parm3, uintptr_t parm4, uintptr_t parm5,
+ uintptr_t parm6);
Review comment:
But, I just follow open/fcntl/sem_open/mq_open/prctl approach. Both method(open .v.s. syslog) work correctly, I select the current one just because ioctl is more like these functions than syslog. I guess that syslog use a different way just because syslog may pass arguments more than 6, but other just need one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#issuecomment-624097349
This would not work, for example, on 8-bit MCUs where sizeof(uintptr_t) == 2, sizeof(int) == 2, but sizeof(unsigned long) == 4. But those won't use system calls anyway.
I had to fix this case explicitly for x86_64 in with wdog_t:
#if UINTPTR_MAX >= UINT32_MAX
typedef uintptr_t wdparm_t;
#else
typedef uint32_t wdparm_t;
#endif
In that case sizeof(uinptr_t) is 8.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#issuecomment-626047511
Hi @xiaoxiang781216,
I've just noticed that the following commit affects dhcp client.
Actually dhcp client can receive the DHCP offer message correctly but it seems that the client does not set IPv4 addresses correctly. (i.e. 0.0.0.0 addresses are set via ioctl)
```
commit 6604cdb3f28999b2a1090b4eaf9b92b2fd4f1b8e
Author: Xiang Xiao <xi...@xiaomi.com>
Date: Mon May 4 17:00:51 2020 +0800
fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Signed-off-by: Xiang Xiao <xi...@xiaomi.com>
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#issuecomment-624097349
This would not work, for example, on 8-bit MCUs where sizeof(uintptr_t) == 2 but sizeof(unsigned long) == 4. But those won't use system calls anyway.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#issuecomment-624106216
Yes, since the conversion is done by mksyscall, the conversion will break if variable argument has the different type from what mksyscall assume.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#issuecomment-623501742
> One nxstyle complaint is normal and to be expected:
But there are several others in include/nuttx/power/bq2429x.h
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#issuecomment-624065996
That change cannot be merged with the terrible implementation of the ioctl() system call. That is unacceptable. Please follow the proven model of nx_vsyslog
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#discussion_r420161523
##########
File path: syscall/syscall.csv
##########
@@ -42,7 +41,7 @@
"if_indextoname","net/if.h","defined(CONFIG_NETDEV_IFINDEX)","FAR char *","unsigned int","FAR char *"
"if_nametoindex","net/if.h","defined(CONFIG_NETDEV_IFINDEX)","unsigned int","FAR const char *"
"insmod","nuttx/module.h","defined(CONFIG_MODULE)","FAR void *","FAR const char *","FAR const char *"
-"ioctl","sys/ioctl.h","!defined(CONFIG_LIBC_IOCTL_VARIADIC)","int","int","int","unsigned long"
+"ioctl","sys/ioctl.h","","int","int","int","..."
Review comment:
I still don't like the way that that works. the way that vsyslog works if much cleaner.
This will also assume that every argument is sizeof(uintptr_t). That could be a problem. It would be better to pass the va_list all the way to the driver ioctl method and it can properly handle types of differing size.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#issuecomment-623347739
One nxstyle complaint is normal and to be expected:
syscall/syscall_stublookup.c:433:3: warning: #include outside of 'Included Files' section
This is a case where the included file is not a normal header file but is C data used to populate an array. Hence, it is included in the "Public Data" section where the array must be initialized.
This is acceptable and should be considered.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#issuecomment-624094518
Thanks for merge. Anyway, we can change to the implementation of syslog in a batch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#discussion_r420159393
##########
File path: syscall/syscall.csv
##########
@@ -42,7 +41,7 @@
"if_indextoname","net/if.h","defined(CONFIG_NETDEV_IFINDEX)","FAR char *","unsigned int","FAR char *"
"if_nametoindex","net/if.h","defined(CONFIG_NETDEV_IFINDEX)","unsigned int","FAR const char *"
"insmod","nuttx/module.h","defined(CONFIG_MODULE)","FAR void *","FAR const char *","FAR const char *"
-"ioctl","sys/ioctl.h","!defined(CONFIG_LIBC_IOCTL_VARIADIC)","int","int","int","unsigned long"
+"ioctl","sys/ioctl.h","","int","int","int","..."
Review comment:
Here is how it works: There is special logic in tools/mksyscall.c:
static bool is_vararg(const char *type, int ndx, int nparms) checks for "..."
The logic at line 310 generates the va_arg() calls.
This is really inefficient. It could be make a little better using nparms < 6
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#discussion_r420119455
##########
File path: syscall/syscall.csv
##########
@@ -42,7 +41,7 @@
"if_indextoname","net/if.h","defined(CONFIG_NETDEV_IFINDEX)","FAR char *","unsigned int","FAR char *"
"if_nametoindex","net/if.h","defined(CONFIG_NETDEV_IFINDEX)","unsigned int","FAR const char *"
"insmod","nuttx/module.h","defined(CONFIG_MODULE)","FAR void *","FAR const char *","FAR const char *"
-"ioctl","sys/ioctl.h","!defined(CONFIG_LIBC_IOCTL_VARIADIC)","int","int","int","unsigned long"
+"ioctl","sys/ioctl.h","","int","int","int","..."
Review comment:
... will not work in system calls. You must follow the patter of vsyslog:
"nx_vsyslog","nuttx/syslog/syslog.h","","int","int","FAR const IPTR char*","FAR va_list*"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #965: fs: Remove all LIBC_IOCTL_VARIADIC related stuff
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #965:
URL: https://github.com/apache/incubator-nuttx/pull/965#issuecomment-624097349
This would not work, for example, on 8-bit MCUs where sizeof(uintptr_t) == 2, sizeof(int) == 2, but sizeof(unsigned long) == 4. But those won't use system calls anyway.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org