On 06/11/2011 09:43 AM, Michal Hocko wrote:
> On Fri 10-06-11 13:12:32, Jan Safranek wrote:
>> On 06/09/2011 05:25 PM, Michal Hocko wrote:
hmmm, how does one import only the patch below (starting with From:)
without this additional comment? I had to manually edit the patch :(
Anyway:
Acked-By: Jan Safranek <jsafr...@redhat.com>
> [...]
>>> What about this?
>>
>> That's it! Apart from few nits (see below) it looks really nice.
>
> See updated
>
> [...]
>>> /**
>>> + * Stores given file permissions of the group's control and tasks files
>>> + * into the @c cgroup data structure. Use NO_PERMS if permissions shouldn't
>>> + * be changed.
>>> + * @param cgroup
>>> + * @param control_dperm Directory permission for the group.
>>> + * @param control_fperm File permission for the control files.
>>> + * @param task_fperm File permissions for task file.
>>
>> I'd add some info what values are expected, i.e. reference to sys/stat.h
>> or chmod(3). And also please note the umask behavior.
>
> Yes. Updated
>>> /* all variables set so create cgroup */
>>> + if (dirm_change + filem_change > 0)
>>> + cgroup_set_permissions(cgroup, dir_mode, file_mode,
>>> + fle_mode);
>>
>> file_mode? ^^^^^^^^^^^^
>
> hmmm, fat fingers after I have included the file, probably...
> ---
>>From 6a32d625464438362588afa7205b7d3271880c9a Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mho...@suse.cz>
> Date: Wed, 8 Jun 2011 17:27:52 +0200
> Subject: [PATCH] chmod_file: Introduce intelligent file permissions setting
>
> There is no general rule on which permissions make sense for files in
> different subsystems. Nevertheless the kernel creates those files with
> the maximum allowed permissions for owner so we should use its part as
> an umask for group and others permissions as well.
> This means that if we specify 777 for file_mode we will end up having
> same permissions as owner what ever they are.
>
> The primary intelligence is implemented in cg_chmod_path function which
> takes an argument which says whether user permissions should be used
> as a template.
>
> This patch adds and exports cgroup_set_permissions which sets
> permissions for control and task files into the cgroup descriptor
> and cgroup_create_cgroup does the rest.
>
> Example:
> /etc/cgconfig.conf:
> mount {
> cpu = /cgroup/cpuctl/;
> }
>
> group devel {
> perm {
> task {
> uid = root;
> gid = cgroup;
> fperm = 770;
> }
> admin {
> uid = root;
> gid = cgroup;
> dperm = 775;
> fperm = 770;
> }
> }
> cpu {
> cpu.shares = 5120;
> }
> }
>
> cd /cgroup/cpuctl/devel/
> ls -la
> drwxrwxr-x 2 root cgroup 0 May 19 16:42 .
> drwxr-xr-x 4 root root 0 May 19 16:14 ..
> -rw-rw---- 1 root cgroup 0 May 19 16:42 cgroup.clone_children
> --w--w---- 1 root cgroup 0 May 19 16:42 cgroup.event_control
> -r--r----- 1 root cgroup 0 May 19 16:42 cgroup.procs
> -rw-rw---- 1 root cgroup 0 May 19 16:42 cpu.rt_period_us
> -rw-rw---- 1 root cgroup 0 May 19 16:42 cpu.rt_runtime_us
> -rw-rw---- 1 root cgroup 0 May 19 16:42 cpu.shares
> -rw-rw---- 1 root cgroup 0 May 19 16:42 notify_on_release
> -rw-rw---- 1 root cgroup 0 May 19 16:42 tasks
>
> Signed-off-by: Michal Hocko <mho...@suse.cz>
> ---
> include/libcgroup/groups.h | 19 +++++++++++-
> src/api.c | 71 +++++++++++++++++++++++++++++++++++--------
> src/libcgroup-internal.h | 11 +++++++
> src/libcgroup.map | 2 +-
> src/tools/cgcreate.c | 14 ++-------
> 5 files changed, 90 insertions(+), 27 deletions(-)
>
> diff --git a/include/libcgroup/groups.h b/include/libcgroup/groups.h
> index 61f18a0..86c5b76 100644
> --- a/include/libcgroup/groups.h
> +++ b/include/libcgroup/groups.h
> @@ -176,7 +176,8 @@ void cgroup_free_controllers(struct cgroup *cgroup);
> * Physically create a control group in kernel. The group is created in all
> * hierarchies, which cover controllers added by cgroup_add_controller().
> * All parameters set by cgroup_add_value_* functions are written.
> - * The created groups has owner which was set by cgroup_set_uid_gid().
> + * The created groups has owner which was set by cgroup_set_uid_gid() and
> + * permissions set by cgroup_set_permissions.
> * @param cgroup
> * @param ignore_ownership When nozero, all errors are ignored when setting
> * owner of the group and/or its tasks file.
> @@ -341,6 +342,22 @@ int cgroup_get_uid_gid(struct cgroup *cgroup, uid_t
> *tasks_uid,
> gid_t *tasks_gid, uid_t *control_uid, gid_t *control_gid);
>
> /**
> + * Stores given file permissions of the group's control and tasks files
> + * into the @c cgroup data structure. Use NO_PERMS if permissions shouldn't
> + * be changed or a value which applicable to chmod(2). Please note that
> + * the given permissions are masked with the file owner's permissions.
> + * For example if a control file has permissions 640 and control_fperm is
> + * 471 the result will be 460.
> + * @param cgroup
> + * @param control_dperm Directory permission for the group.
> + * @param control_fperm File permission for the control files.
> + * @param task_fperm File permissions for task file.
> + */
> +void cgroup_set_permissions(struct cgroup *cgroup,
> + mode_t control_dperm, mode_t control_fperm,
> + mode_t task_fperm);
> +
> +/**
> * @}
> * @name Group parameters
> * These are functions can read or modify parameter of a group.
> diff --git a/src/api.c b/src/api.c
> index 0f308af..66cc2b3 100644
> --- a/src/api.c
> +++ b/src/api.c
> @@ -175,12 +175,47 @@ static int cg_chown_recursive(char **path, uid_t owner,
> gid_t group)
> return ret;
> }
>
> +int cg_chmod_path(const char *path, mode_t mode, int owner_is_umask)
> +{
> + struct stat buf;
> + mode_t mask = -1U;
> +
> + if (owner_is_umask) {
> + mode_t umask, gmask, omask;
> +
> + /*
> + * Use owner permissions as an umask for group and others
> + * permissions because we trust kernel to initialize owner
> + * permissions to something useful.
> + */
> + if (stat(path, &buf) == -1)
> + goto fail;
> + umask = S_IRWXU & buf.st_mode;
> + gmask = umask >> 3;
> + omask = gmask >> 3;
> +
> + mask = umask|gmask|omask;
> + }
> +
> + if (chmod(path, mode & mask))
> + goto fail;
> +
> + return 0;
> +
> +fail:
> + last_errno = errno;
> + return ECGOTHER;
> +}
> +
> int cg_chmod_file(FTS *fts, FTSENT *ent, mode_t dir_mode,
> - int dirm_change, mode_t file_mode, int filem_change)
> + int dirm_change, mode_t file_mode, int filem_change,
> + int owner_is_umask)
> {
> int ret = 0;
> const char *filename = fts->fts_path;
> +
> cgroup_dbg("chmod: seeing file %s\n", filename);
> +
> switch (ent->fts_info) {
> case FTS_ERR:
> errno = ent->fts_errno;
> @@ -190,20 +225,17 @@ int cg_chmod_file(FTS *fts, FTSENT *ent, mode_t
> dir_mode,
> case FTS_DNR:
> case FTS_DP:
> if (dirm_change)
> - ret = chmod(filename, dir_mode);
> + ret = cg_chmod_path(filename, dir_mode, owner_is_umask);
> break;
> case FTS_F:
> case FTS_NSOK:
> case FTS_NS:
> case FTS_DEFAULT:
> if (filem_change)
> - ret = chmod(filename, file_mode);
> + ret = cg_chmod_path(filename, file_mode,
> + owner_is_umask);
> break;
> }
> - if (ret < 0) {
> - last_errno = errno;
> - ret = ECGOTHER;
> - }
> return ret;
> }
>
> @@ -212,7 +244,8 @@ int cg_chmod_file(FTS *fts, FTSENT *ent, mode_t dir_mode,
> * TODO: Need to decide a better place to put this function.
> */
> static int cg_chmod_recursive_controller(char *path, mode_t dir_mode,
> - int dirm_change, mode_t file_mode, int filem_change)
> + int dirm_change, mode_t file_mode, int filem_change,
> + int owner_is_umask)
> {
> int ret = 0;
> int final_ret =0;
> @@ -239,7 +272,7 @@ static int cg_chmod_recursive_controller(char *path,
> mode_t dir_mode,
> break;
> }
> ret = cg_chmod_file(fts, ent, dir_mode, dirm_change,
> - file_mode, filem_change);
> + file_mode, filem_change, owner_is_umask);
> if (ret) {
> last_errno = errno;
> final_ret = ECGOTHER;
> @@ -269,7 +302,7 @@ int cg_chmod_recursive(struct cgroup *cgroup, mode_t
> dir_mode,
> break;
> }
> ret = cg_chmod_recursive_controller(path, dir_mode, dirm_change,
> - file_mode, filem_change);
> + file_mode, filem_change, 0);
> if (ret)
> final_ret = ret;
> }
> @@ -277,6 +310,15 @@ int cg_chmod_recursive(struct cgroup *cgroup, mode_t
> dir_mode,
> return final_ret;
> }
>
> +void cgroup_set_permissions(struct cgroup *cgroup,
> + mode_t control_dperm, mode_t control_fperm,
> + mode_t task_fperm)
> +{
> + cgroup->control_dperm = control_dperm;
> + cgroup->control_fperm = control_fperm;
> + cgroup->task_fperm = task_fperm;
> +}
> +
> static char *cgroup_basename(const char *path)
> {
> char *base;
> @@ -1475,13 +1517,13 @@ int cgroup_create_cgroup(struct cgroup *cgroup, int
> ignore_ownership)
> cgroup_dbg("Changing ownership of %s\n", fts_path[0]);
> error = cg_chown_recursive(fts_path,
> cgroup->control_uid, cgroup->control_gid);
> - if (!error) {
> + if (!error)
> error =
> cg_chmod_recursive_controller(fts_path[0],
> cgroup->control_dperm,
> cgroup->control_dperm !=
> NO_PERMS,
> cgroup->control_fperm,
> - cgroup->control_fperm !=
> NO_PERMS);
> - }
> + cgroup->control_fperm !=
> NO_PERMS,
> + 1);
> }
>
> if (error)
> @@ -1529,7 +1571,8 @@ int cgroup_create_cgroup(struct cgroup *cgroup, int
> ignore_ownership)
> error = chown(path, cgroup->tasks_uid,
> cgroup->tasks_gid);
> if (!error && cgroup->task_fperm != NO_PERMS)
> - error = chmod(path, cgroup->task_fperm);
> + error = cg_chmod_path(path, cgroup->task_fperm,
> + 1);
>
> if (error) {
> last_errno = errno;
> diff --git a/src/libcgroup-internal.h b/src/libcgroup-internal.h
> index 9845cad..de437e1 100644
> --- a/src/libcgroup-internal.h
> +++ b/src/libcgroup-internal.h
> @@ -251,6 +251,17 @@ extern int cgroup_dictionary_iterator_next(void **handle,
> */
> extern void cgroup_dictionary_iterator_end(void **handle);
>
> +/**
> + * Changes permissions for given path. If owner_is_umask is specified
> + * then it uses owner permissions as a mask for group and others permissions.
> + *
> + * @param path Patch to chmod.
> + * @param mode File permissions to set.
> + * @param owner_is_umask Flag whether path owner permissions should be used
> + * as a mask for group and others permissions.
> + */
> +int cg_chmod_path(const char *path, mode_t mode, int owner_is_umask);
> +
> __END_DECLS
>
> #endif
> diff --git a/src/libcgroup.map b/src/libcgroup.map
> index fb30801..f1afaf6 100644
> --- a/src/libcgroup.map
> +++ b/src/libcgroup.map
> @@ -101,5 +101,5 @@ CGROUP_0.38 {
> cgroup_get_subsys_mount_point_begin;
> cgroup_get_subsys_mount_point_next;
> cgroup_get_subsys_mount_point_end;
> + cgroup_set_permissions;
> } CGROUP_0.37;
> -
> diff --git a/src/tools/cgcreate.c b/src/tools/cgcreate.c
> index 7312b32..d470b94 100644
> --- a/src/tools/cgcreate.c
> +++ b/src/tools/cgcreate.c
> @@ -302,6 +302,9 @@ int main(int argc, char *argv[])
> }
>
> /* all variables set so create cgroup */
> + if (dirm_change + filem_change > 0)
> + cgroup_set_permissions(cgroup, dir_mode, file_mode,
> + file_mode);
> ret = cgroup_create_cgroup(cgroup, 0);
> if (ret) {
> fprintf(stderr, "%s: "
> @@ -310,17 +313,6 @@ int main(int argc, char *argv[])
> cgroup_free(&cgroup);
> goto err;
> }
> - if (dirm_change + filem_change > 0) {
> - ret = cg_chmod_recursive(cgroup, dir_mode, dirm_change,
> - file_mode, filem_change);
> - if (ret) {
> - fprintf(stderr, "%s: can't change permission " \
> - "of cgroup %s: %s\n", argv[0],
> - cgroup->name, cgroup_strerror(ret));
> - cgroup_free(&cgroup);
> - goto err;
> - }
> - }
> cgroup_free(&cgroup);
> }
> err:
------------------------------------------------------------------------------
EditLive Enterprise is the world's most technically advanced content
authoring tool. Experience the power of Track Changes, Inline Image
Editing and ensure content is compliant with Accessibility Checking.
http://p.sf.net/sfu/ephox-dev2dev
_______________________________________________
Libcg-devel mailing list
Libcg-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libcg-devel
