On Wednesday, February 13, 2013 11:42:57 PM [email protected] wrote:
> From: Vitaly _Vi Shukela <[email protected]>
Hi Vitaly,
Thanks again for your patience, comments below ...
> diff --git a/include/seccomp.h.in b/include/seccomp.h.in
> index b21205c..e2003ce 100644
> --- a/include/seccomp.h.in
> +++ b/include/seccomp.h.in
> @@ -25,6 +25,7 @@
> #include <inttypes.h>
> #include <asm/unistd.h>
> #include <linux/audit.h>
> +#include <stdarg.h>
>
> #ifdef __cplusplus
> extern "C" {
> @@ -389,6 +390,16 @@ int seccomp_syscall_priority(scmp_filter_ctx ctx,
> int seccomp_rule_add(scmp_filter_ctx ctx,
> uint32_t action, int syscall, unsigned int arg_cnt, ...);
>
> +/** va_list analogue of seccomp_rule_add */
> +int seccomp_rule_add_valist(scmp_filter_ctx ctx,
> + uint32_t action, int syscall, unsigned int arg_cnt,
> + va_list arg_list);
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.
> +/** 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.
> @@ -407,6 +418,15 @@ int seccomp_rule_add(scmp_filter_ctx ctx,
> int seccomp_rule_add_exact(scmp_filter_ctx ctx, uint32_t action,
> int syscall, unsigned int arg_cnt, ...);
>
> +/** va_list analogue of seccomp_rule_add_exact */
> +int seccomp_rule_add_valist_exact(scmp_filter_ctx ctx, uint32_t action,
> + int syscall, unsigned int arg_cnt, va_list arg_list);
> +
> +/** array version of seccomp_rule_add_exact */
> +int seccomp_rule_add_array_exact(scmp_filter_ctx ctx,
> + uint32_t action, int syscall, unsigned int arg_cnt,
> + const struct scmp_arg_cmp *arg_array);
See above comments for both these functions. I'm also thinking this function
should be named "seccomp_rule_add_exact_array()".
> /**
> * Generate seccomp Pseudo Filter Code (PFC) and export it to a file
> * @param ctx the filter context
> diff --git a/src/api.c b/src/api.c
> index 5072afc..c524fa9 100644
> --- a/src/api.c
> +++ b/src/api.c
> @@ -366,7 +366,8 @@ syscall_priority_failure:
> */
> static int _seccomp_rule_add(struct db_filter_col *col,
> unsigned int strict, uint32_t action, int syscall,
> - unsigned int arg_cnt, va_list arg_list)
> + unsigned int arg_cnt,
> + const struct scmp_arg_cmp* arg_array)
A couple of style nits ... 1) put the '*' up against the parameter/variable
and not against the type 2) I may be wrong about this, but it looks like you
used spaces instead of tabs here; used tabs when possible and spaces if the
distance needed is less than a single tab.
Also, we should probably do a "arg_array != NULL" check up near the top of
this function where we do the other -EINVAL checks.
> +/* NOTE - function header comment in include/seccomp.h */
> +int seccomp_rule_add_valist(scmp_filter_ctx ctx,
> + uint32_t action, int syscall, unsigned int arg_cnt,
> + va_list arg_list)
> +{
> + int i;
I'm being very nit picky here but since you need to respin the patch anyway,
how about changing this from "i" to "iter"? It matches the rest of the code
better ... I really like consistent style ...
> + struct scmp_arg_cmp arg_array[6];
We should use ARG_COUNT_MAX here to remove some of the "magic number"
problems.
> + if (arg_cnt > sizeof(arg_array)/sizeof(arg_array[0]) || arg_cnt<0) {
> + return -1;
> + }
What is wrong with "(arg_cnt < 0 || arg_cnt > ARG_COUNT_MAX)"?
> + for (i=0; i<arg_cnt; ++i) {
> + arg_array[i] = va_arg(arg_list, struct scmp_arg_cmp);
> + }
Spaces around the "<" and "=" operators.
> + return seccomp_rule_add_array(ctx, action, syscall, arg_cnt ...
> +}
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.
> +/* NOTE - function header comment in include/seccomp.h */
> +int seccomp_rule_add_valist_exact(scmp_filter_ctx ctx,
> + uint32_t action, int syscall, unsigned int arg_cnt,
> + va_list arg_list)
> +{
> + int i;
> + struct scmp_arg_cmp arg_array[6];
> +
> + if (arg_cnt > sizeof(arg_array)/sizeof(arg_array[0]) || arg_cnt<0) {
> + return -1;
> + }
> +
> + for (i=0; i<arg_cnt; ++i) {
> + arg_array[i] = va_arg(arg_list, struct scmp_arg_cmp);
> + }
> +
> + return seccomp_rule_add_array_exact(ctx, action, syscall, arg_cnt,
> + arg_array);
> +}
See my earlier comments.
--
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
