On Saturday, February 16, 2013 07:01:29 AM vi0oss wrote: > > 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...
That is my thinking too and the array version gives us the ability to do a bit more sanity checking which is why I like it a bit more at this point. > >> +/** 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. Yes, I like real comments, especially when it comes to the public API. In some cases that is all the documentation people look at so I want to make sure it has a reasonable level of quality. > > 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. Unfortunately I don't think there is a single universal tool to validate coding style as each project, and sometimes each file inside a project, can have a different style. The only bit of advice I can give you is that when you are editing an existing file you should try to mimic the style of that file as much as possible. This isn't a foolproof rule, but it has served me well so far. > > ... I really like consistent style ... > > After N iterations of propose_patch -> review -> resend_patch I expect > it to be consistent... :) Generally if you get "close" on the style and there are no significant issues with the patch I'll go ahead and merge it and do the minor style cleanups when I do the merge. So, while it is good to try and abide by the existing libseccomp style, if you get everything else sorted out don't worry about having to respin a patch because you missed a tab. > > 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)... I think we've covered most of the style stuff, so no more comments there except to say that I consider tabs to be eight spaces. Also, I should say that you might notice the Python/Cython stuff having its own style, that is because the Python/Cython code tries to abide by all the Python PEP standards for style. > 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...) Okay, thanks again for sticking with this; if you have any questions don't hesitate to ask. Good luck! -- paul moore security and virtualization @ redhat ------------------------------------------------------------------------------ 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
