[patch]setfiles/restorecon applet

[Denis Vlasenko]

Hi,

Unlike me, you aren't late with replies. Cool.

On Thursday 19 July 2007 02:17, Yuichi Nakamura wrote:
> > Sorry, late reply :((
> > 
> > On Monday 02 July 2007 06:29, Yuichi Nakamura wrote:
> > > I would like to submit setfiles/restorecon applet.
> > > setfiles and restorecon are SELinux commands that  
> > > label files according to configuration file
> > > (in configuration file, relationship between file and label is
> > > described).
> > > 
> > > These applets are very important for SELinux enabled system.
> > > Please review and consider merging this patch.
> > 
> > Took a look:
> > 
> > * Trimmed help text a bit
> > * Removed \n at the end of bb_[p]error
> > * Removed progname (using applet_name)
> > * Removed restorecon.c - USE_RESTORECON(APPLET_ODDNAME...) handles that
> > * int -> smallint for many flag variables
> >   This needs improvement - many of those flags are already accessible
> >   as (option_mask32 & BIT_MASK), you don't need separate variables
> > 
> > Please take a look at:
> > * Do you really have to have fork()?
> I looked code again and found it unnecessary so removed.
> 
> 
> > * Maybe use xstrdup instead of strdupa
> Fixed.
> 
> > * nftw() is not used in bbox. We have recursive_action().
> >   It will be better if we won't pull in nftw() into busybox
> >   just for this applet.
> Fixed to use recursive_action instead of nftw.
> 
> > * Do not do "if (applet_is_restorecon && option_x) bb_show_usage()"
> >   Just remove 'x' flag from optarg32 instead.
> Fixed.
> 
> > 
> > If some of the above is not feasible, explain that.
> > 
> > Please see attached updated patch.
> > --
> > vda
> 
> In addition, I am using struct globals for static data.

> Attached is revised patch, please look at.

See attached patch. bb_common_bufsiz1 trick must have compile-time check
(unless it's trivially obvious that it is ok, like "I have 10 static ints only")

unsigned long long count;

Provided that you rarely see more than 4000000000 files and even then
count overflow only upset star display (cosmetics), please use just "unsigned".

static int restore(const char *file)
{
        char *my_file = strdupa(file);

Convert to xstrdup please.

                if (count % 1000 == 0) {
                        if (count % 80000 == 0)

Maybe ((count & 0x3ff) == 0) /* every 1024 files */
- code may be smaller & faster...

		bb_error_msg("%s not reset customized by admin to %s",

"FILE not reset customized by admin to XXXX" - I think colon and/or
commas need to added. Maybe

"FILE: not reset, customized by admin to XXXX"

but I still don't 100% understand the message. Maybe "File's context not reset"?
And what does "customized by root" mean - file's context is set in some /etc/xxx file?
Try to provide a clear message. (Sometimes it takes quite a time
to come up with clear _and_ not very long message. Take that time).

        if (context)
                freecon(context);

if() is redundant.

        ret = lsetfilecon(my_file, newcon);
        if (ret) {
                bb_perror_msg("lsetfileconon(%s,%s)", my_file, newcon);
                goto out;
        }
 out:
        freecon(newcon);
        return 0;
 err:
        freecon(newcon);
        return -1;

Maybe it makes sense to group all three free() here, ensure that variables
are allocated or NULL:

	retval = 0;
        ...
        ...
 err:
	retval--; /* == -1 */
 out:
	free(my_file);
	freecon(context);
	freecon(newcon);
	return retval;
}

and you will have single exit point (and probably smaller code - 3 free[con]
instead of 5 you have now).



#define OPT_c           (1<<0)
...
        if (applet_name[0] == 'r') { /* restorecon */
                flags = getopt32(argc, argv, "de:f:ilnpqrsvo:FRW", &exclude_dir, &input_filename, &out_filename, &verbose);
        } else { /* setfiles */
                flags = getopt32(argc, argv, "c:de:f:ilnpqr:svo:FW", &policyfile, &exclude_dir, &input_filename, &rootpath, &out_filename,
        }

For restorecon OPT_c will mean "-d"!
In such cases you just need to move all common flags to the beginning
of string, the rest should be after. IOW:

- "de:f:ilnpqrsvo:FRW"
- "c:de:f:ilnpqr:svo:FW"
+ "de:f:ilnpqrsvo:FWR"
+ "de:f:ilnpqr:svo:FWc:"

with OPT_xxx renumbered accordingly.



#if ENABLE_FEATURE_SETFILES_CHECK_OPTION
...
#else
                bb_error_msg_and_die("-c is not supported");
#endif

Just add "c:" to getopt32 string conditionally instead:
	"xxxxxxxxxxxxxxxx" USE_FEATURE_SETFILES_CHECK_OPTION("c:")
--
vda
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 2.patch
Type: text/x-diff
Size: 22178 bytes
Desc: not available
Url : http://busybox.net/lists/busybox/attachments/20070719/1824cf07/attachment-0001.bin