kosslab-kr / linux-perf

:rocket: perf contribution (mirrored from git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git)
Other
16 stars 8 forks source link

perf config: Improvement the check routine of mutual exclusivity using between system and user config #134

Open ppiyakk2 opened 8 years ago

ppiyakk2 commented 8 years ago

Change system and user config_options from OPT_BOOLEAN to OPT_BOOLEAN_FLAG and add option 'PARSE_OPT_EXCLUSIVE' on it

So, There are no need to check mutual exclusivity using between system and user config.

Taeung commented 8 years ago

테스트도 하고 code style도 보는중이 었는데.. 이제서야 생각이 났네요 제가 PARSE_OPT_EXCLUSIVE 를 걸어서 구현 했다가 다시 제외 했던 이유가 앞으로 추가할 config 명령의 옵션들이 --user 또는 --system과 함께 이용될수있기 때문이었는데요.

아직 구현된 내용은 아니지만 --list-all 이라는 옵션을 추가할 예정입니다. 이건 $HOME/.perfconfig 이나 $(sysconfdir)/perfconfig에 있는 내용뿐만 아니라 원래 지정할수도있는 다른 config 내용까지도 보여주는 기능 입니다. 예를 들면 아래 처럼 나오는데요

$ perf config -a
colors.top=red, default
colors.medium=green, default
colors.normal=lightgray, default
colors.selected=white, lightgray
colors.jump_arrows=blue, default
colors.addr=magenta, default
colors.root=white, blue
tui.report=on
tui.annotate=true
tui.top=true
buildid.dir=~/.debug
annotate.hide_src_code=false
annotate.use_offset=true
annotate.jump_arrows=true
annotate.show_nr_jumps=false
annotate.show_linenr=false
annotate.show_total_period=false
gtk.annotate=false
gtk.report=false
gtk.top=false
pager.cmd=true
pager.report=true
pager.annotate=true
pager.top=true
pager.diff=true
help.format=man
help.autocorrect=0
hist.percentage=absolute
ui.show-headers=true
call-graph.record-mode=fp
call-graph.dump-size=8192
call-graph.print-type=graph
call-graph.order=callee
call-graph.sort-key=function
call-graph.threshold=0.500000
call-graph.print-limit=0
report.group=true
report.children=true
report.percent-limit=0.000000
report.queue-size=0
top.children=true
man.viewer=man
kmem.default=slab

제가 나중에 --list 와 --list-all은 동시에 쓰지 못하게 하기위해서 아래 처럼 구현 했었는데요..

...
665 int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
666 {
...
672 
673         set_option_flag(config_options, 'l', "list", 665 int cmd_config(int argc, const char **argv, const char *prefix __maybe_unused)
666 {
667         int i, ret = 0;
668         struct list_head *sections;
669         struct list_head all_sections, user_sections, system_sections;
670         const char *system_config = perf_etc_perfconfig();
671         char *user_config = mkpath("%s/.perfconfig", getenv("HOME"));
672 
673         set_option_flag(config_options, 'l', "list", PARSE_OPT_EXCLUSIVE);
674         set_option_flag(config_options, 'a', "list-all", PARSE_OPT_EXCLUSIVE);
);
...

만약에 --user, --system, --list, --list-all 모두 PARSE_OPT_EXCLUSIVE를 걸어버리면 아래와같이 --user --list 를 동시에 못쓰는 문제가 발생해서 구현을 빼버렸었습니다..

$  perf config --user --list
 Error: option `list' cannot be used with user
 Usage: perf config [<file-option>] [options] [section.name[=value] ...]

    -l, --list            show current config variables
    -a, --list-all        show current and all possible config variables with default values
        --user            use user config file

원래는 user config 파일과 system config 파일이 아래와 같이 있을때

$ cat ~/.perfconfig
[annotate]
    hide_src_code = false
[tui]
    report = on
[colors]
        top = red, default

$ cat /usr/local/etc/perfconfig 
[annotate]
    hide_src_code = true
[report]
    queue-size = 0

이렇게 user config 내용만 나와야 정상이라서요

$ perf config --user --list
annotate.hide_src_code=false
tui.report=on
colors.top=red, default
``
물론 list와 list-all을 if 문으로 따로 예외처리 해도 되긴하는데.. 무튼 저는 이렇게 할생각이 있어서 제외했었습니다... ㅎㅎ 다른 좋은 방법이 있을까요 ?
ppiyakk2 commented 8 years ago

그렇다면 system 과 user 에 대해서만 EXCLUSIVE 옵션을 주면 되지 않을까요?? 그럼 --system, --user 에 대해서만 동시에 사용을 못하고 나머지 옵션들이랑은 조화롭게 사용할 수 있을 듯 합니다.

Taeung commented 8 years ago

그래도 되긴합니다만 사실 정책문제이긴 한데 --user 와 --system 서로만 같이 쓰이면 안되는거지 다른 옵션들과는 다양하게 조화하려는 목적의 옵션이자나요? 하지만 --list 나 --list-all --remove 기타등등 옵션들은 원래 독립적으로 쓰이는게 의미상으로 맞다고 생각해서 그렇게 생각했습니다 EXCLUSIVE옵션을 --user 와 --system서로 한번에 못쓰게 하는 용도만 쓰고 다른 옵션들은 못쓰게 하는것보다 둘은 if문으로 처리하고 (새롭게 추가될) 새로운 옵션들이 EXCLUSIVE가 필요 하다면 쭉 쓰는게 낫다고 생각해서 정책문제 이긴한데 어떻게 생각하시나요? 반대가 나으신가요?

ppiyakk2 commented 8 years ago

정책상의 문제이고, 미래에 추가될 옵션과의 조화를 생각하면 현재의 코드대로 진행하는 것이 베스트라고 생각하여 이 패치를 적용하지 않습니다.