apache
kernel/syscall: Fix if enable Net/socket/iperf but not enable iNet/IPv4.
Summary
Fix some case break if we enable Net & local socket & rpmsg socket, but did not enable iNet and ipv6, iNet is not a must have feature in Net, especially Asymmetric Multi-Processing devices.
Impact
After patch, able to open iperf to test rpmsg socket etc.
Testing
CI-test, qemu-arm-v7a ostest
Is there any chance you can split this into multiple PRs? 9 commits is a lot. Also, can you please provide more detail about what the internal CI tests?
There is an impact of this PR if you are modifying code. Please list what applications/tests are impacted by this change.
Is there any chance you can split this into multiple PRs? 9 commits is a lot. Also, can you please provide more detail about what the internal CI tests?
There is an impact of this PR if you are modifying code. Please list what applications/tests are impacted by this change.
Merge patch into one pr aim to save the CI resource. if split too much will cause more CI actions. If we create pr for specific topic, I agree with we should only include relative patchs.
Every commit is described in commit msg, maybe don't have to copy all from commit msg to github pr?
About internal CI, we are not only one codebase from github. when we using these code for project, patch already included and tested.
@jasonbu You can't assume that if something works in Xiaomi internal repo, it will also work with the upstream. Xiaomi repositories are not synchronized with the upstream. This has led to bugs before. Changes to Apache repositories should be tested with Apache repositories. Splitting PRs into smaller ones is safer because it's easier to do reviews, even if the cost is higher CI usage.
@jasonbu You can't assume that if something works in Xiaomi internal repo, it will also work with the upstream. Xiaomi repositories are not synchronized with the upstream. This has led to bugs before. Changes to Apache repositories should be tested with Apache repositories. Splitting PRs into smaller ones is safer because it's easier to do reviews, even if the cost is higher CI usage.
Sure, we have to re check to ensure no any break when upstream, as these change a all small and in apps, merged commits into one pr here.
Next pr will make it more clear and more relative and more convenient for reviewer。
Sure, we have to re check to ensure no any break when upstream, as these change a all small and in apps, merged commits into one pr here.
Yes, so can you please tell us what you tested, how you tested and include the log results?
Next pr will make it more clear and more relative and more convenient for reviewer。
No, all PRs need to be clear and convenient for the reviewers. Please update this PR so that it is more clear, as raiden suggested.
I really suggest all these commits be broken into multiple PRs, regardless of CI resources, because you have many unrelated fixes for multiple apps and they will all require their own summary, impact and test description.
@raiden00pl @cederom @linguini1 splited into #3225 #3226 #3227 #3228 #3229 hopes will make it more easy to understand, and be happy
It seems all the related PRs were merged. None of them include any test logs or good test descriptions, so I will still hold my request for testing information on this PR.
It seems all the related PRs were merged. None of them include any test logs or good test descriptions, so I will still hold my request for testing information on this PR.
could you review the change? this patch resovle the compiler error, so the change pass ci already indicate the modification is good. The change which fix the compiler issue, don't need runtime log to indicate wether the modification is correct or not.
The change which fix the compiler issue, don't need runtime log to indicate wether the modification is correct or not.
I disagree because this PR introduces compile time changes that conditionally removes logic. It would be good to see testing of the modified examples.
The change which fix the compiler issue, don't need runtime log to indicate wether the modification is correct or not.
I disagree because this PR introduces compile time changes that conditionally removes logic. It would be good to see testing of the modified examples.
hi, @linguini1 , rebased to the master code, and double check it still works as expected, and updated in the pr content. please check.
about the cmocka case test_nuttx_syscall_getsockopt01/test_nuttx_syscall_setsockopt01. the testcode using PF_INET/AF_INET, those is not supported if IPv4 not enabled, will cause net_sockif goto defaultNULL branch.
so to minimal change the case, just close when IPv4 not enabled should prefer.