ver6(Re: [PATCH 4/8] busybox -- SELinux option support for coreutils: ver3

[Yuichi Nakamura]

On Thu, 8 Mar 2007 13:45:34 +0100
Bernhard Fischer  wrote:
> Index: coreutils/stat.c
> ===================================================================
> --- coreutils/stat.c    (revision 17961)
> +++ coreutils/stat.c    (working copy)
> ...
> + * Copyright (C) 2006 by Yoshinori Sato <ysato at users.sourceforge.jp>
> 
> copyright year (email addr?)
He wrote this patch in 2006, so year is 2006.

> @@ -164,6 +167,14 @@
>                 strncat(pformat, "jd", buf_len);
>                 printf(pformat, (intmax_t) (statfsbuf->f_ffree));
>                 break;
> +#if ENABLE_SELINUX
> +       case 'C':
> +               if (flags & OPT_SELINUX) {
> +                       strcat(pformat, "s");
> +                       printf(scontext);
> +               }
> +               break;
> +#endif
> 
> You don't need strncat here, do you?
Fixed, using strncat.

> @@ -301,6 +313,14 @@                                
>                 strncat(pformat, TYPE_SIGNED(time_t) ? "ld" : "lu", buf_len);
>                 printf(pformat, (unsigned long int) statbuf->st_ctime);
>                 break;
> +#if ENABLE_SELINUX
> +       case 'C':
> +               if (flags & OPT_SELINUX) {
> +                       strcat(pformat, "s");
> +                       printf(pformat, scontext);
> +               }
> +               break;
> +#endif
> That sounds rather familiar. Deja vu? Any way around that duplication?
> @@ -360,11 +381,25 @@
>  }
>  #endif
> 
> +#define _getfilecon(filename, scontext) \
> +       (flags & OPT_DEREFERENCE ? \
> +        lgetfilecon(filename, scontext): \
> +        getfilecon(filename, scontext))
> +
> You use this define just in one place. Please expand it manually at the
> call-site.
Fixed.

> I'd prefer if you would just #if ENABLE_SELINUX and not duplicate the
> "format". Makes grepping for duplicates easier, imo.
Around there,
not only SELINUX part, but also other part have duplicating code...

Yuichi Nakamura

-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-coreutils-stat-04.v6.patch
Type: application/octet-stream
Size: 10947 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20070309/0089836d/attachment-0001.obj