Ver.6 (Re: [PATCH 7/8] busybox -- SELinux option support for coreutils: ver3)
KaiGai Kohei
kaigai at kaigai.gr.jp
Sat Mar 10 05:43:01 UTC 2007
Hello, Thanks for your reviewing.
>> * '\n' was replaced by '0xff'
>> * Checks for exclusive options were added. chcon applet will will abort
>> if --reference and '-u','-r','-t','-l' are appeared concurrently,
>
> Please move the change_dir_context() function around to remove the
> forward decl.
I replaced change_dir_context() function by recursive_action(), not only
its declaration.
> +static context_t compute_context_from_mask(security_context_t context,
> + context_free (new_context);
>
> surplus space
> Smaller if you goto ret where
> ret:
> return NULL;
> }
As you say, we can share the above function between runcon and chcon,
so I removed the compute_context_from_mask() and added a new function
in libbb/selinux_common.c.
See, busybox-coreutils-libbb-09.v6.patch
> + context_string = context_str(context);
> + if (file_context == NULL || strcmp(context_string, file_context) != 0) {
>
> Can context_str() return NULL?
Yes, it can return NULL, if malloc() used internally didn't succeed.
So, I added the following check:
+ context_string = context_str(context);
+ if (!context_string) {
+ bb_error_msg("couldn't obtain security context in text expression");
+ goto skip;
+ }
> + int fail = 0;
>
> superfluous set. Remove initialization to 0
Fixed,
> +
> + if (opts & OPT_NODEREFERENCE) {
> + fail = lsetfilecon (fname, context_string);
> + } else {
> + fail = setfilecon (fname, context_string);
> + }
> + if ((opts & OPT_VERBOSE)
> + || ((opts & OPT_CHANHES) && !fail)) {
> + printf(!fail
>
> bb_perror_msg() instead of printf?
bb_perror_msg() prints a message into stderr, although upstreamed chcon
prints a message into stdout. It will make an incompatibility.
So, I didn't use bb_error_msg() here.
> + ? "context of %s changed to %s\n"
> + : "failed to change context of %s to
> %s\n",
> + fname, context_string);
> + }
>
> change_dir_context() sounds a bit overdone. Don't we have an opendir
> wrapper with diagnostics? Perhaps this is a candidate for
> recursive_action()
It was replaced by recursive_action().
> Manually inlining compute_context_from_mask() smaller?
It was moved into libbb/selinux_common.o to share with 'runcon'.
> Smaller to resuse the global option_mask32 rather than using opts in
> chcon_main() ?
I replaced any local 'opts' by option_mask32.
> + ":?:f--v:v--f" /* 'verbose' and 'quiet' are exclusive */
> Sounds like there is the word mutual missing.
'-f' is a option for the silent purpose.
It is a definition of upstreamed chcon.
Thanks,
--
KaiGai Kohei <kaigai at kaigai.gr.jp>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-coreutils-libbb-09.v6.patch
Type: text/x-diff
Size: 2083 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20070310/812157ed/attachment-0006.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-coreutils-runcon-08.v6.patch
Type: text/x-diff
Size: 4150 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20070310/812157ed/attachment-0007.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-coreutils-chcon-07.v6.patch
Type: text/x-diff
Size: 5322 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20070310/812157ed/attachment-0008.bin
More information about the busybox
mailing list