[PATCH] policycoreutils/setfiles: set up a logging callback for libselinux

[PATCH] policycoreutils/setfiles: set up a logging callback for libselinux

[Stephen Smalley]

Define a logging callback for libselinux so that any informational
or error messages generated by libselinux functions are properly
prefixed with the program name and routed to the proper output stream.

Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
 policycoreutils/setfiles/setfiles.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/policycoreutils/setfiles/setfiles.c b/policycoreutils/setfiles/setfiles.c
index 1a2d711..d767131 100644
--- a/policycoreutils/setfiles/setfiles.c
+++ b/policycoreutils/setfiles/setfiles.c
@@ -138,6 +138,19 @@ static void maybe_audit_mass_relabel(int mass_relabel, int mass_relabel_errs)
 #endif
 }
 
+static int __attribute__ ((format(printf, 2, 3)))
+log_callback(int type, const char *fmt, ...)
+{
+	int rc;
+	FILE *out = (type == SELINUX_INFO) ? stdout : stderr;
+	va_list ap;
+	fprintf(out, "%s: ", r_opts.progname);
+	va_start(ap, fmt);
+	rc = vfprintf(out, fmt, ap);
+	va_end(ap);
+	return rc;
+}
+
 int main(int argc, char **argv)
 {
 	struct stat sb;
@@ -151,6 +164,7 @@ int main(int argc, char **argv)
 	const char *ropts = "e:f:hiIDlmno:pqrsvFRW0";
 	const char *sopts = "c:de:f:hiIDlmno:pqr:svFR:W0";
 	const char *opts;
+	union selinux_callback cb;
 
 	/* Initialize variables */
 	memset(&r_opts, 0, sizeof(r_opts));
@@ -383,6 +397,9 @@ int main(int argc, char **argv)
 			mass_relabel = 1;
 	}
 
+	cb.func_log = log_callback;
+	selinux_set_callback(SELINUX_CB_LOG, cb);
+
 	if (!iamrestorecon) {
 		if (policyfile) {
 			if (optind != (argc - 1))
@@ -401,8 +418,8 @@ int main(int argc, char **argv)
 		 * we can support either checking against the active policy or
 		 * checking against a binary policy file.
 		 */
-		selinux_set_callback(SELINUX_CB_VALIDATE,
-				     (union selinux_callback)&canoncon);
+		cb.func_validate = canoncon;
+		selinux_set_callback(SELINUX_CB_VALIDATE, cb);
 
 		if (stat(argv[optind], &sb) < 0) {
 			perror(argv[optind]);
-- 
2.7.4

Re: [PATCH] policycoreutils/setfiles: set up a logging callback for libselinux

[Alan Jenkins]

On 24/01/17 19:39, Stephen Smalley wrote:
> Define a logging callback for libselinux so that any informational
> or error messages generated by libselinux functions are properly
> prefixed with the program name and routed to the proper output stream.

Makes sense.

It's a shame it makes the info lines a bit longer.  I don't think they 
need it - compare `ls` or `cp -rv`.  Admittedly the version from my 
distro - before libselinux took over this function - already printed it 
as 'restorecon reset ...'.

I think the longer lines are tolerable. Though, another effect of the 
old format was to make the error messages more visually distinct.

I'm fine with it either way, but my suggestion is

        if (type != SELINUX_INFO)
                fprintf(out, "%s: ", r_opts.progname);


I notice the messages still in `setfiles.c` are a bit inconsistent about 
including the program name. Potentially they could use logging calls as 
well, instead of using fprintf() and referencing r_opts.progname each time.

>
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
>   policycoreutils/setfiles/setfiles.c | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/policycoreutils/setfiles/setfiles.c b/policycoreutils/setfiles/setfiles.c
> index 1a2d711..d767131 100644
> --- a/policycoreutils/setfiles/setfiles.c
> +++ b/policycoreutils/setfiles/setfiles.c
> @@ -138,6 +138,19 @@ static void maybe_audit_mass_relabel(int mass_relabel, int mass_relabel_errs)
>   #endif
>   }
>   
> +static int __attribute__ ((format(printf, 2, 3)))
> +log_callback(int type, const char *fmt, ...)
> +{
> +	int rc;
> +	FILE *out = (type == SELINUX_INFO) ? stdout : stderr;
> +	va_list ap;
> +	fprintf(out, "%s: ", r_opts.progname);
> +	va_start(ap, fmt);
> +	rc = vfprintf(out, fmt, ap);
> +	va_end(ap);
> +	return rc;
> +}
> +
>   int main(int argc, char **argv)
>   {
>   	struct stat sb;
> @@ -151,6 +164,7 @@ int main(int argc, char **argv)
>   	const char *ropts = "e:f:hiIDlmno:pqrsvFRW0";
>   	const char *sopts = "c:de:f:hiIDlmno:pqr:svFR:W0";
>   	const char *opts;
> +	union selinux_callback cb;
>   
>   	/* Initialize variables */
>   	memset(&r_opts, 0, sizeof(r_opts));
> @@ -383,6 +397,9 @@ int main(int argc, char **argv)
>   			mass_relabel = 1;
>   	}
>   
> +	cb.func_log = log_callback;
> +	selinux_set_callback(SELINUX_CB_LOG, cb);
> +
>   	if (!iamrestorecon) {
>   		if (policyfile) {
>   			if (optind != (argc - 1))
> @@ -401,8 +418,8 @@ int main(int argc, char **argv)
>   		 * we can support either checking against the active policy or
>   		 * checking against a binary policy file.
>   		 */
> -		selinux_set_callback(SELINUX_CB_VALIDATE,
> -				     (union selinux_callback)&canoncon);
> +		cb.func_validate = canoncon;
> +		selinux_set_callback(SELINUX_CB_VALIDATE, cb);
>   
>   		if (stat(argv[optind], &sb) < 0) {
>   			perror(argv[optind]);