----- Original Message -----
> From: [email protected]
> To: "Alexey Kodanev" <[email protected]>
> Cc: "vasily isaenko" <[email protected]>,
> [email protected]
> Sent: Wednesday, 15 May, 2013 2:38:26 PM
> Subject: Re: [LTP] [PATCH] New core test of cgroups and extended attributes
>
> Hi!
> > +++ b/testcases/kernel/controllers/cgroup_xattr/cgroup_xattr.c
> > @@ -0,0 +1,343 @@
> > +/*
> > + * Copyright (c) 2013 Oracle and/or its affiliates. All Rights Reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it would be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write the Free Software Foundation,
> > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> > + *
> > + * Author:
> > + * Alexey Kodanev <[email protected]>
> > + *
> > + * Test checks following preconditions:
> > + * since Linux kernel 3.7 it is possible to set extended attributes
> > + * to cgroup files.
> > + */
> > +
> > +#include <sys/stat.h>
> > +#include <sys/mount.h>
> > +#include <sys/types.h>
> > +#include <sys/xattr.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <fcntl.h>
> > +#include <dirent.h>
> > +#include <unistd.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +
> > +#include "test.h"
> > +#include "usctest.h"
> > +#include "safe_macros.h"
> > +
> > +char *TCID = "cgroup_xattr";
> > +
> > +static const char cgrp_point[] = "/sys/fs/cgroup";
> > +static const char cgrp_name[] = "cgrp_test";
> > +static const char subdir_name[] = "test";
> > +
> > +struct tst_key {
> > + const char *name;
> > + int good;
> > +};
> > +
> > +/* only security.* & trusted.* is valid key names */
> ^ are
> > +static const struct tst_key tkeys[] = {
> > + { .name = "trusted.test", .good = 1, },
> > + { .name = "security.", .good = 1, },
> > + { .name = "user.", .good = 0, },
> > + { .name = "system.", .good = 0, },
> > +};
> > +
> > +#define VALUE_SIZE 8
> > +
> > +/*
> > + * values that can be written to xattr keys,
> > + * their can be anything
> ^ their value?
> > + */
> > +struct tst_val {
> > + char buf[VALUE_SIZE];
> > + size_t size;
> > +};
> > +
> > +/* cleanup flags */
> > +static int cgrp_mounted;
> > +static int subdir_created;
> > +
> > +/* test options */
> > +static int skip_cleanup;
> > +static int verbose;
> > +static const option_t options[] = {
> > + {"s", &skip_cleanup, NULL},
> > + {"v", &verbose, NULL},
> > + {NULL, NULL, NULL}
> > +};
> > +
> > +/* save to change back in the end */
> > +static char start_work_dir[PATH_MAX];
> > +
> > +static void help(void);
> > +static void setup(int argc, char *argv[]);
> > +static void test_run(void);
> > +static void cleanup(void);
> > +
> > +static int set_xattrs(const char *file, const struct tst_val *val);
> > +static char *get_xattr(const char *file, const char *key_name, size_t
> > *size);
> > +static int get_xattrs(const char *file, const struct tst_val *val);
> > +/*
> > + * set or get xattr recursively
> > + *
> > + * @path: start directory
> > + * @id: start char to fill in value
> > + * @xattr_operation: can be set_xattrs() or get_xattrs()
> > + */
> > +static int cgrp_files_walking(const char *path, char *id,
> > + int (*xattr_operation)(const char *, const struct tst_val *));
> > +static char *get_hex_value(const char *value, size_t size);
> > +static void fill_test_value(char *id, struct tst_val *val);
> > +
> > +
> > +int main(int argc, char *argv[])
> > +{
> > + setup(argc, argv);
> > +
> > + test_run();
> > +
> > + cleanup();
> > +
> > + tst_exit();
> > +}
> > +
> > +static void help(void)
> > +{
> > + printf(" -s Skip cleanup\n");
> > + printf(" -v Verbose\n");
> > +}
> > +
> > +void setup(int argc, char *argv[])
> > +{
> > + char *msg;
> > + msg = parse_opts(argc, argv, options, help);
> > + if (msg != NULL)
> > + tst_brkm(TBROK, NULL, "OPTION PARSING ERROR - %s", msg);
> > +
> > + tst_require_root(NULL);
> > +
> > + if (access("/proc/cgroups", F_OK) == -1)
> > + tst_brkm(TCONF, cleanup, "Kernel doesn't support for cgroups");
> > +
> > + if (tst_kvercmp(3, 7, 0) < 0) {
> > + tst_brkm(TCONF, cleanup,
> > + "Test must be run with kernel 3.7 or newer");
> > + }
> > +
> > + tst_sig(FORK, DEF_HANDLER, cleanup);
> > +
> > + /* mount all available subsystems (cpu, cpuset, memory, ...) */
> > + if (mount(cgrp_name, cgrp_point, "cgroup", 0, "xattr") == -1)
> > + tst_brkm(TBROK, cleanup, "Can't mount: %s", cgrp_point);
>
> Here we mount cgroups under /sys/fs/cgroup which later causes problems
> as you need to switch to another directory to unmount.
>
> I'm not that much familiar with cgroups, is mounting it to /sys/
> required or is it customary? Wouldn't that interfere with rest of the
> system?
It's customary, but It could interfere with systemd, which mounts
some subsytems at boot:
cgroup on /sys/fs/cgroup/systemd type cgroup
(rw,nosuid,nodev,noexec,relatime,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=systemd)
cgroup on /sys/fs/cgroup/cpuset type cgroup
(rw,nosuid,nodev,noexec,relatime,cpuset)
cgroup on /sys/fs/cgroup/cpu,cpuacct type cgroup
(rw,nosuid,nodev,noexec,relatime,cpuacct,cpu)
cgroup on /sys/fs/cgroup/devices type cgroup
(rw,nosuid,nodev,noexec,relatime,devices)
cgroup on /sys/fs/cgroup/freezer type cgroup
(rw,nosuid,nodev,noexec,relatime,freezer)
cgroup on /sys/fs/cgroup/net_cls type cgroup
(rw,nosuid,nodev,noexec,relatime,net_cls)
cgroup on /sys/fs/cgroup/blkio type cgroup
(rw,nosuid,nodev,noexec,relatime,blkio)
cgroup on /sys/fs/cgroup/perf_event type cgroup
(rw,nosuid,nodev,noexec,relatime,perf_event)
Trying to mount different combination of subsystems will likely fail:
# mount -t cgroup -ocpu none /mnt/test
mount: none is already mounted or /mnt/test busy
Quoting Documentation/cgroups/cgroups.txt:
"
If an active hierarchy with exactly the same set of subsystems already
exists, it will be reused for the new mount. If no existing hierarchy
matches, and any of the requested subsystems are in use in an existing
hierarchy, the mount will fail with -EBUSY. Otherwise, a new hierarchy
is activated, associated with the requested subsystems.
"
Regards,
Jan
>
> What about calling tst_tmpdir(), creating a directory to mount the
> cgroups there and then simply doing chdir to test temporary directory
> before the unmount. Would that work?
>
> > + cgrp_mounted = 1;
> > +
> > + /* save current working directory */
> > + SAFE_GETCWD(cleanup, start_work_dir, PATH_MAX);
> > + SAFE_CHDIR(cleanup, cgrp_point);
> > +
> > + /* create new hierarchy */
> > + SAFE_MKDIR(cleanup, subdir_name, 0755);
> > + subdir_created = 1;
> > +}
> > +
> > +static void test_run()
>
> void in params is missing
>
> > +{
> > + char id;
> > + /* set xattr to each file in cgroup fs */
> > + id = 0;
> > + int set_res = cgrp_files_walking(cgrp_point, &id, set_xattrs);
>
> Here you are passing pointer to the value, modify it by the function but
> never use the resulting value.
>
> > + /* get & check xattr from each file in cgroup fs */
> > + id = 0;
> > + int get_res = cgrp_files_walking(cgrp_point, &id, get_xattrs);
> > +
> > + /* final results */
> > + tst_resm(TINFO, "All test-cases have been completed, summary:");
> > + tst_resm(TINFO, "Set tests result: %s", (set_res) ? "FAIL" : "PASS");
> > + tst_resm(TINFO, "Get tests result: %s", (get_res) ? "FAIL" : "PASS");
> > +}
> > +
> > +static void cleanup(void)
> > +{
> > + if (skip_cleanup)
> > + return;
> > +
> > + fflush(stdout);
>
> What is this fflush() for?
>
> > + if (subdir_created) {
> > + SAFE_CHDIR(NULL, cgrp_point);
> > + if (rmdir(subdir_name) == -1) {
> > + tst_brkm(TBROK, NULL, "Can't remove dir, error: %s",
> > + strerror(errno));
> > + }
> > + }
> > +
> > + if (strlen(start_work_dir) > 0)
> > + SAFE_CHDIR(NULL, start_work_dir);
> > +
> > + if (cgrp_mounted) {
> > + if (umount(cgrp_point) == -1)
> > + tst_brkm(TBROK, NULL, "Can't unmount: %s", cgrp_point);
> > + }
> > +
> > + TEST_CLEANUP;
> > +}
> > +
> > +static int set_xattrs(const char *file, const struct tst_val *val)
> > +{
> > + int i, res;
> > + res = 0;
> > + for (i = 0; i < ARRAY_SIZE(tkeys); ++i) {
> > + int good = setxattr(file, tkeys[i].name,
> > + (const void *)val->buf, val->size, 0) != -1;
> > +
> > + int fail = good != tkeys[i].good;
> > + res |= fail;
> > +
> > + tst_resm((fail) ? TFAIL : TPASS,
> > + "Expect: %s set xattr key '%s' to file '%s'",
> > + (tkeys[i].good) ? "can" : "can't",
> > + tkeys[i].name, file);
> > +
> > + if (verbose && tkeys[i].good) {
> > + char *rval = get_hex_value(val->buf, val->size);
> > + tst_resm(TINFO, "value =%s", rval);
> > + free(rval);
> > + }
> > + }
> > + return res;
> > +}
> > +
> > +static char *get_xattr(const char *file, const char *key_name, size_t
> > *size)
> > +{
> > + char *xval = NULL;
> > + /* get value size */
> > + ssize_t xval_size = getxattr(file, key_name, (void *)xval, 0);
>
> Pass NULL here, instead of the xval initialized to NULL.
>
> > + if (xval_size == -1)
> > + return NULL;
> > +
> > + xval = SAFE_MALLOC(cleanup, xval_size);
> > +
> > + if (getxattr(file, key_name, (void *)xval, xval_size) == -1) {
> > + free(xval);
> > + return NULL;
> > + }
> > + *size = xval_size;
> > + return xval;
> > +}
> > +
> > +static int get_xattrs(const char *file, const struct tst_val *val)
> > +{
> > + int i, fail, res;
> > + res = 0;
> > + for (i = 0; i < ARRAY_SIZE(tkeys); ++i) {
> > + size_t xval_size = 0;
> > + char *xval;
> > +
> > + xval = get_xattr(file, tkeys[i].name, &xval_size);
> > + fail = (xval == NULL && tkeys[i].good);
> > + res |= fail;
> > +
> > + tst_resm((fail) ? TFAIL : TPASS,
> > + "Expect: %s read xattr %s of file '%s'",
> > + (xval == NULL) ? "can't" : "can", tkeys[i].name, file);
> > + if (xval == NULL)
> > + continue;
> > +
> > + if (fail) {
> > + free(xval);
> > + continue;
> > + }
> > +
> > + fail |= val->size != xval_size ||
> > + strncmp(val->buf, xval, val->size) != 0;
> > + res |= fail;
> > +
> > + tst_resm((fail) ? TFAIL : TPASS, "Expect: values equal");
> > +
> > + if (verbose && fail) {
> > + char *rval = get_hex_value(xval, xval_size);
> > + tst_resm(TINFO, "Read xattr value:%s", rval);
> > + free(rval);
> > + char *cval = get_hex_value(val->buf, val->size);
> > + tst_resm(TINFO, "Expected value:%s", cval);
> > + free(cval);
> > + }
> > + free(xval);
> > + }
> > + return res;
> > +}
> > +
> > +static int cgrp_files_walking(const char *path, char *id,
> > + int (*xattr_operation)(const char *, const struct tst_val *))
> > +{
> > + int res = 0;
> > + struct dirent *entry;
> > + DIR *dir;
> > + dir = opendir(path);
> > + SAFE_CHDIR(cleanup, path);
> > + tst_resm(TINFO, "In dir %s", path);
> > + errno = 0;
> > + while ((entry = readdir(dir)) != NULL) {
> > + const char *file = entry->d_name;
> > + /* skip current and up directories */
> > + if (!strcmp(file, "..") || !strcmp(file, "."))
> > + continue;
> > +
> > + struct stat stat_buf;
> > + TEST(lstat(file, &stat_buf));
> > + if (TEST_RETURN != -1 && S_ISDIR(stat_buf.st_mode)) {
> > + /* proceed to subdir */
> > + res |= cgrp_files_walking(file, id, xattr_operation);
> > + /* change directory back */
> > + SAFE_CHDIR(cleanup, "..");
> > + tst_resm(TINFO, "In dir %s", path);
> > + }
> > + struct tst_val val;
> > + fill_test_value(id, &val);
> > + res |= xattr_operation(file, &val);
> > + errno = 0;
> > + }
> > + if (errno && !entry)
> > + tst_brkm(TWARN, cleanup, "%s", strerror(errno));
>
> Use TERRNO and also describe the warning more verbosely.
>
> > + if (closedir(dir) == -1)
> > + tst_brkm(TWARN, cleanup, "Failed to close dir '%s'", path);
> > +
> > + return res;
> > +}
> > +
> > +static char *get_hex_value(const char *value, size_t size)
> > +{
> > + const size_t symb_num = 5; /* space + 0xXX*/
> > + char *buf = SAFE_MALLOC(cleanup, size * symb_num + 1);
> > + size_t i;
> > + for (i = 0; i < size; ++i) {
> > + sprintf(buf + i * symb_num, " 0x%02X",
> > + (unsigned char) *(value++));
> > + }
> > + return buf;
> > +}
>
> This function is too ugly. You should rather do something like
> print_xattr() that would both preprare the string and call the
> tst_resm() so that you don't need to pass allocated buffers around.
>
> void print_xattr(const char *msg, const char *val, size_t size)
> {
> char buf[size + sybm_num + 1];
> ...
>
> tst_resm(TINFO, "%s%s", msg, buf);
> }
>
> Or we can create a tst_xxx function to print a buffer of bytes, I guess
> that there are more cases where such function could be used. But that
> would require a little more work to desing the interface right.
>
> > +static void fill_test_value(char *id, struct tst_val *val)
> > +{
> > + int i;
> > + for (i = 0; i < VALUE_SIZE; ++i)
> > + val->buf[i] = *id;
>
> What about using memset()?
>
> > + val->size = VALUE_SIZE;
> > + ++(*id);
>
> This is quite cryptic, why aren't you modifying the id in the loop that
> calls this function?
>
> What about just defining the id in the walk functions and incrementing
> it each time fill_test_value() was called?
>
> > +}
>
> --
> Cyril Hrubis
> [email protected]
>
> ------------------------------------------------------------------------------
> AlienVault Unified Security Management (USM) platform delivers complete
> security visibility with the essential security capabilities. Easily and
> efficiently configure, manage, and operate all of your security controls
> from a single console and one unified framework. Download a free trial.
> http://p.sf.net/sfu/alienvault_d2d
> _______________________________________________
> Ltp-list mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/ltp-list
>
------------------------------------------------------------------------------
AlienVault Unified Security Management (USM) platform delivers complete
security visibility with the essential security capabilities. Easily and
efficiently configure, manage, and operate all of your security controls
from a single console and one unified framework. Download a free trial.
http://p.sf.net/sfu/alienvault_d2d
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list
