Skip to content

Comments

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

Open
ppiyakk2 wants to merge 1 commit intokosslab-kr:masterfrom
ppiyakk2:perf_config
Open

perf config: Improvement the check routine of mutual exclusivity using between system and user config#134
ppiyakk2 wants to merge 1 commit intokosslab-kr:masterfrom
ppiyakk2:perf_config

Conversation

@ppiyakk2
Copy link

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.

…g between system and user config

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
Copy link
Member

Taeung commented Sep 28, 2016

테스트도 하고 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
Copy link
Author

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

@Taeung
Copy link
Member

Taeung commented Sep 28, 2016

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

@ppiyakk2
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants