[patch]setfiles/restorecon applet

[Yuichi Nakamura]

On Thu, 19 Jul 2007 23:45:26 +0100
Denis Vlasenko wrote:
> Hi,
> 
> Unlike me, you aren't late with replies. Cool.
Thank you.
And thanks your quick reply too !


<snip>
> > 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".
Fixed. Using 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...
Fixed. 

> 
> 		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).
Fixed. Made it shorter.

> 
>         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).
OOps, I have forgotten to free my_file.
Fixed.

> 
> 
> 
> #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.
It was a big mistake. Fixed.

> 
> 
> 
> #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:")
Fixed.

> --
> vda

Attached is updated patch.

Regards,
-- 
Yuichi Nakamura
Hitachi Software Engineering Co., Ltd.
Japan SELinux Users Group(JSELUG): http://www.selinux.gr.jp/
SELinux Policy Editor: http://seedit.sourceforge.net/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 3.patch
Type: application/octet-stream
Size: 22010 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20070720/260e5873/attachment-0002.obj