> Hi Vitaly, Thanks again for your patience, comments below ... > Unless you can think of a very compelling reason why we need a _valist() > version of these functions right now that can't be solved by the _array() > version below I think we should drop this function. Just the idea that every "..." should have "va_list" sibling, to implement wrappers around the function. I was thinking to also include it in a test.
With array version it is actually not very necessary... > >> +/** array version of seccomp_rule_add */ >> +int seccomp_rule_add_array(scmp_filter_ctx ctx, >> + uint32_t action, int syscall, unsigned int arg_cnt, >> + const struct scmp_arg_cmp *arg_array); > This prototype should have a comment header similar to the others. While we > currently don't have doxygen built into the build process I like to stick to > the doxygen comment formatting. I thought a bit about how to keep DRY and avoid pumping up lines count for such a minor change... If needed, the fully fledged comment can be provided. > See above comments for both these functions. I'm also thinking this > function should be named "seccomp_rule_add_exact_array()". OK. > A couple of style nits ... OK, although I don't like messing this this, tabs vs spaces are usually just chaotic and uncontrolled on my system (especially when I use multiple text editors). Internally I consider most whitespace fixing a work for bots, not for humans... I also don't know the reliable and universal (for all projects) method to check if my patch's style is OK or not. > Also, we should probably do a "arg_array != NULL" check up near the > top of this function where we do the other -EINVAL checks. OK. > ... I really like consistent style ... After N iterations of propose_patch -> review -> resend_patch I expect it to be consistent... > We should use ARG_COUNT_MAX here to remove some of the "magic number" > problems. So now I know about ARG_COUNT_MAX and will use it. > See my comments above about this function; we can probably mark it as > static and preface the name with a "_". Whitespace problems, use tabs > when possible. Brace problems, don't use them if you don't need them. I remember changing spaces to tabs in some places... Each tabs vs space problem occurrence is not obvious for me how to solve currently. (Spaces everywhere seems to be easier and more predictable scheme; Is tab expected to take 4 spaces for this project?). Good thing we don't have LF/CRLF line endings problem here (which should also just not exist in the world)... Will provide the "rule_add_..._array patch series v2" in some time (the most complicated part is probably to decide what should be in the doc and find out how to add tests...) ------------------------------------------------------------------------------ The Go Parallel Website, sponsored by Intel - in partnership with Geeknet, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials, tech docs, whitepapers, evaluation guides, and opinion stories. Check out the most recent posts - join the conversation now. http://goparallel.sourceforge.net/ _______________________________________________ libseccomp-discuss mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/libseccomp-discuss
