[systemd-devel] [PATCH] mount and initialize Smack

[Lennart Poettering]

On Tue, 05.03.13 15:24, Nathaniel Chen (nathaniel.chen at intel.com) wrote:

Heya,

A few comments on top of what Zbigniew already pointed out.

> +	smack = fopen("/sys/fs/smackfs/load2", "w");

Not that it would matter here, but out of principle we generally use "we"
instead of "w"...

> +	if (smack == NULL)  {
> +		m = mount("smackfs", "/smack", "smackfs",
> MS_NOSUID|MS_NOEXEC|MS_NODEV|MS_STRICTATIME, "smackfsdef=*");

Please drop this mount() call. Mounts should take place in
mount-setup.c. And the old /smack mount point is not acceptable in
systemd. We should only support the new mount point, and that's it. 

> +		if (m == -1) {
> +			log_error("Smack is not enabled in the kernel. error: %s", strerror(errno));
> +			return 0;

Suggestion: It's nicer to write %m rather than %s + strerror(errno)...

> +		}
> +		smack = fopen("/smack/load2", "w");
> +	}
> +
> +	/* write rules to load2 from every file in the directory */
> +	dir = opendir(ACCESSES_D_PATH);
> +	if (dir == NULL) {
> +		log_error("failed to open Smack policy directory %s, error: %s", ACCESSES_D_PATH, strerror(errno));
> +		fclose(smack);
> +		return -errno;
> +	}
> +	while ((entry = readdir(dir))) {
> +		if (entry->d_name[0] == '.')
> +			continue;

Suggestion: FOREACH_DIRENT() is a nice macro for iterating through
directories. It's a fairly new addition, and little used, but it does
make things a bit nicer to read. See src/shared/efivars.c for an example
how to use it.

> +
> +		snprintf(pol, PATH_MAX, ACCESSES_D_PATH"%s", entry->d_name);
> +		policy = fopen(pol, "r");

"re" is nicer than "r", see above.

> +		if (policy == NULL) {
> +			log_error("Failed to open Smack policy file %s, error: %s", pol, strerror(errno));
> +			fclose(smack);
> +			closedir(dir);
> +			return -errno;
> +		}

> +		/* load2 write rules in the kernel require a line buffered stream */
> +		while ((fgets(buf, NAME_MAX, policy))) {
> +			fprintf(smack, "%s", buf);
> +			fflush(smack);
> +			log_info("Smack rule written: %s", buf);
> +		}

Consider using the (also new) FOREACH_LINE macro for this iteration. For
an example look at get_process_id() in util.c.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.