[busybox:00323] Re: [PATCH 4/8] busybox -- libselinux utilities applets

[KaiGai Kohei]

Bernhard, Thanks for your comments.

The attached patch fixes following items:
- avcstat and togglesebool applet were removed
- xis_selinux_enabled() was added at libbb/xfuncs.c
- unneccesary headers were removed.
- bb_error_msg_and_die() + strerror() were replaced
   with bb_perror_msg_and_die()
- "Selinux Utilities" at menuconfig got dependency with CONFIG_SELINUX
- some cleanups.

>> Index: selinux/getsebool.c
>> =================================>> --- selinux/getsebool.c	(revision 0)
>> +++ selinux/getsebool.c	(revision 0)
>> @@ -0,0 +1,83 @@
>> +/*
>> + * getsebool
>> + *
>> + * Based on libselinux 1.33.1
>> + * Port to BusyBox  Hiroshi Shinji <shiroshi at my.email.ne.jp>
>> + *
>> + */
>> +
>> +#include "busybox.h"
>> +#include <selinux/selinux.h>
>> +
>> +#define GETSEBOOL_OPT_ALL	1
>> +
>> +int getsebool_main(int argc, char **argv)
>> +{
>> +	int i, rc >> +	char **names;
>> +	unsigned long opt;
>> +
>> +	opt >> +
>> +	if(opt & GETSEBOOL_OPT_ALL) {
> 
> missing space after "if"

Fixed, and confirmed a space is placed after any 'if' and 'for'
in front of '('.

>> +		if (argc > 2)
>> +			bb_show_usage();
>> +		if (is_selinux_enabled() <>> +			bb_error_msg_and_die("SELinux is disabled");
> 
> You're doing this alot. Please move this out to a
> int xis_selinux_enabled(void) {
> 	smallint ret > 	if (ret !> 		bb_error_msg_and_die("SELinux is disabled");
> 	return ret;
> }
> in e.g. libbb/xfuncs.c and use it in your other SElinux applets, too.

I added xis_selinux_enabled() at libbb/xfuncs.c to die if SELinux was
disabled. Some similar implementations are replaced.

>> +		}
>> +		errno > 
> hm?

removed it.

>> +		rc >> +		if (rc) {
> ->+			bb_error_msg_and_die("cannot get boolean names:  %s",
> ->+					     strerror(errno));
> 
> bb_perror_msg_and_die("cannot get boolean name");
> should do too

The combination of bb_error_msg_and_die() and strerror() was replaced
by bb_perror_msg_and_die()

>> +		}
>> +		if (!len) {
>> +			printf("No booleans\n");
> 
> puts smaller?

Agreed. It was replaced with puts().

>> +			return 0;
>> +		}
>> +	}
> 
> See how you didn't use opt much?
> I'd rather say 
> xis_selinux_enabled();
> opt_complementary="-1";/* need at least 1 non-option arg*/
> if (getopt32(argc, argv, "a")) {
> 	rc > 	if (rc ...
> }

When we use '-a' option, any other non-option arguments are
not allowed. Thus, we cannot use the above opt_complementary.

>> +
>> +	if (is_selinux_enabled() <>> +		bb_error_msg_and_die("SELinux is disabled");
> 
> That can't be right, no?
> You called security_get_boolean_names() before checking if selinux is
> enabled or not. Does this work?

No, the above security_get_boolean_names() was called after checking
if selinux is enabled or not in the 'if (opt & GETSEBOOL_OPT_ALL) {...}'
block.

> What about removing that is_selinux_enabled block here, move the call to 
> xis_selinux_enabled from the "if(opt & GETSEBOOL_OPT_ALL) {" block to
> below the "if (opt..)" block so you check for enabled only once (before
> get_boolean_nam())

I agreed it.
xis_selinux_enabled() was moved at light after getopt32().
It will be done only once.

Thanks,
-- 
Open Source Software Promotion Center, NEC
KaiGai Kohei <kaigai at ak.jp.nec.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-libselinux.v3.patch
Type: text/x-patch
Size: 15404 bytes
Desc: not available
Url : http://busybox.net/lists/busybox/attachments/20070131/522d3784/attachment-0001.bin