Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!

Bug 541792

Summary: sys-apps/openrc: Clean up rc-cgroup.sh for clarity
Product: Gentoo Linux Reporter: tokiclover <tokiclover>
Component: [OLD] Core systemAssignee: OpenRC Team <openrc>
Status: RESOLVED UPSTREAM    
Severity: normal Keywords: PATCH
Priority: Normal    
Version: unspecified   
Hardware: All   
OS: Linux   
Whiteboard:
Package list:
Runtime testing required: ---
Attachments: rc-cgroup.sh.patch
rc-cgroup.sh.in.patch

Description tokiclover 2015-03-01 12:11:47 UTC
Please, consider this patch in an attempt to clean up rc-cgroup.sh. I've just took a look the other day because I needed to make quick similar file for Supervision-Scripts[1] and had a hard time going trough it because of poor coding style (somewhat confused at times) and bad variable naming scheme.

I suggest as well:
    * Renaming cgroup_cleanup() to cgroup_del_service() or cgroup_remove_service() to get more consistency on helpers naming (in par with cgroup_add_service()). This is not done in the current iteration because it would require patching runscripts.sh.
    * Use an environment variable--(e.g.) RC_CGROUP_NAME--for the use of 'openrc' group (name.)

Thanks.

(I am opening a bug request because I don't want to clone OpenRC and submit pull requests. Too much has to be done for that end. Moreover, upstream *can* or *could* completely ignore it--too much work and troubles for nothing. No thanks, Sir!)

[1]: https://github.com/tokiclover/supervision-scripts

Reproducible: Always
Comment 1 tokiclover 2015-03-01 12:13:55 UTC
Created attachment 397774 [details, diff]
rc-cgroup.sh.patch

The patch is big (almost twice the size of the file!)
Comment 2 William Hubbs gentoo-dev 2015-03-01 20:16:20 UTC
I will reply here, but please move further discussion to the github issue. I do not want to track a bug in two bug reporting systems.

(In reply to tokiclover from comment #0)
> Please, consider this patch in an attempt to clean up rc-cgroup.sh. I've
> just took a look the other day because I needed to make quick similar file
> for Supervision-Scripts[1] and had a hard time going trough it because of
> poor coding style (somewhat confused at times) and bad variable naming
> scheme.

"poor coding style" and "bad variable naming scheme" are definitely subjective, but I am open to suggestions for improvement. :-)

> I suggest as well:
>     * Renaming cgroup_cleanup() to cgroup_del_service() or
> cgroup_remove_service() to get more consistency on helpers naming (in par
> with cgroup_add_service()). This is not done in the current iteration
> because it would require patching runscripts.sh.

The function is different. cgroup_cleanup is used to clean up an entire cgroup and kill all processes in it. cgroup_del_service implies to me that we are just removing a process from the cgroup.

>     * Use an environment variable--(e.g.) RC_CGROUP_NAME--for the use of
> 'openrc' group (name.)

The "openrc" cgroup is not meant to be messed with directly, so I don't see why I would want to allow users to give it a different name. Again, if there is a reason, I will consider it.

> (I am opening a bug request because I don't want to clone OpenRC and submit
> pull requests. Too much has to be done for that end. Moreover, upstream
> *can* or *could* completely ignore it--too much work and troubles for
> nothing. No thanks, Sir!)

I use "git am" or "git apply" to apply patches from bugzilla bug reports to the master branch (with a strong preference for "git am" since it preserves authorship information). The patch you  attached was not generated with
"git format-patch", so that forces me to use "git apply", but that will not work with your patch because it is not against the master branch. That leaves me with doing all of the edits manually, which I would rather not with a patch this size.

Since you are on github already, I strongly recommend forking and submitting pull requests.
Comment 3 tokiclover 2015-03-02 09:07:20 UTC
(In reply to William Hubbs from comment #2)
> > I suggest as well:
> >     * Renaming cgroup_cleanup() to cgroup_del_service() or
> > cgroup_remove_service() to get more consistency on helpers naming (in par
> > with cgroup_add_service()). This is not done in the current iteration
> > because it would require patching runscripts.sh.
> 
> The function is different. cgroup_cleanup is used to clean up an entire
> cgroup and kill all processes in it. cgroup_del_service implies to me that
> we are just removing a process from the cgroup.

I was awre of that, it was just meant as helper naming consistency. However, Alexander has a valid, on github issue tracker, cgroup_cleanup() is a public API, so there is no simple way to change it now but keep it for the time being and warn users of a possible removale, encourage using cgroup_del_service() instead et al. Well, we will be better off leaving it as as.

> >     * Use an environment variable--(e.g.) RC_CGROUP_NAME--for the use of
> > 'openrc' group (name.)
> 
> The "openrc" cgroup is not meant to be messed with directly, so I don't see
> why I would want to allow users to give it a different name. Again, if there
> is a reason, I will consider it.

Thanks for making this issue apparent... I did not meant to make the variable name in public API. Moreover, the name used--with the RC_ prefix--implies it. And this was supposed to be a clean up _for_ clarity...

I've just realized that, a different name space should be used first (and maybe with prepending '_'); and use something like CG_ROOT=/sys/fs/cgroup instead or in conjunction with CG_NAME=openrc (and/or CG_PATH=/${CG_ROOT}/${CG_NAME}).

So, something like
---
:    ${CG_NAME=openrc}
:    ${CG_ROOT=/sys/fs/cgroup}
:    ${CG_PATH=${CG_ROOT}/${CG_NAME}
---
(with optional '_' to steress on the 'private' and maybe no caps at all) can be added in the begining of the file.

The idead was to use that simple point of programming by eliminating redundancy and adding clarity by using (explicit and not confusing) variable/function names.

So, I could update the patch by using _any_ or _all_ or _nothing_ of the above.
Comments?


> [..]
> Since you are on github already, I strongly recommend forking and submitting
> pull requests.

I am not decided yet... I may just pick up the current rc-cgroup.sh from master before submitting a new itteration of the patches if any after your & Alexander's comments.
Comment 4 tokiclover 2015-03-19 19:43:50 UTC
Created attachment 399266 [details, diff]
rc-cgroup.sh.in.patch
Comment 5 Andrew Savchenko gentoo-dev 2015-03-26 21:35:10 UTC
*** Bug 543862 has been marked as a duplicate of this bug. ***