New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common/config: eliminate config_t::set_val unsafe option #13687

Merged
merged 10 commits into from Mar 28, 2017

Conversation

Projects
None yet
3 participants
@Liuchang0812
Contributor

Liuchang0812 commented Feb 28, 2017

@Liuchang0812

This comment has been minimized.

Show comment
Hide comment
@Liuchang0812

Liuchang0812 Feb 28, 2017

Contributor

jenkins test this please

Contributor

Liuchang0812 commented Feb 28, 2017

jenkins test this please

@Liuchang0812

This comment has been minimized.

Show comment
Hide comment
@Liuchang0812

Liuchang0812 Feb 28, 2017

Contributor

Working on fail test cases

Contributor

Liuchang0812 commented Feb 28, 2017

Working on fail test cases

@Liuchang0812

This comment has been minimized.

Show comment
Hide comment
@Liuchang0812

Liuchang0812 Mar 1, 2017

Contributor

🤣 I need help. It seems that ctest could not find some *.so files, but it is ok when I try to run unittest binary directly. What should I do to fix this problem? @tchaikov

more logs as following:

➜  build git:(wip-19106-eliminate-unsafe-option) ✗ ctest -VV -R unittest_compression
UpdateCTestConfiguration  from :/home/liuchang/WorkSpace/ceph/build/DartConfiguration.tcl
UpdateCTestConfiguration  from :/home/liuchang/WorkSpace/ceph/build/DartConfiguration.tcl
Test project /home/liuchang/WorkSpace/ceph/build
Constructing a list of tests
Done constructing a list of tests
Checking test dependency graph...
Checking test dependency graph end
test 85
    Start 85: unittest_compression

85: Test command: /home/liuchang/WorkSpace/ceph/build/bin/unittest_compression
85: Environment variables:
85:  CEPH_ROOT=/home/liuchang/WorkSpace/ceph
85:  CEPH_BIN=/home/liuchang/WorkSpace/ceph/build/bin
85:  CEPH_LIB=/home/liuchang/WorkSpace/ceph/build/lib
85:  CEPH_BUILD_DIR=/home/liuchang/WorkSpace/ceph/build
85:  LD_LIBRARY_PATH=/home/liuchang/WorkSpace/ceph/build/lib
85:  PATH=/home/liuchang/WorkSpace/ceph/build/bin:/home/liuchang/WorkSpace/ceph/src:/home/liuchang/bin:/home/liuchang/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin
85:  PYTHONPATH=/home/liuchang/WorkSpace/ceph/build/lib/cython_modules/lib.2:/home/liuchang/WorkSpace/ceph/src/pybind
85:  CEPH_BUILD_VIRTUALENV=/tmp
85: Test timeout computed to be: 3600
85: 2017-03-01 14:27:35.522809 7fc7e144af00 -1 did not load config file, using default settings.
85: [==========] Running 68 tests from 3 test cases.
85: [----------] Global test environment set-up.
85: [----------] 3 tests from ZlibCompressor
85: [ RUN      ] ZlibCompressor.zlib_isal_compatibility
85: *** Caught signal (Segmentation fault) **
85:  in thread 7fc7e144af00 thread_name:unittest_compre
85: 2017-03-01 14:27:35.525576 7fc7e144af00 -1 load failed dlopen(): "/usr/local/lib/ceph/compressor/libceph_zlib.so: cannot open shared object file: No such file or directory" or "/usr/local/lib/ceph/libceph_zlib.so: cannot open shared object file: No such file or directory"
85: 2017-03-01 14:27:35.525591 7fc7e144af00 -1 create cannot load compressor of type zlib


➜  build git:(wip-19106-eliminate-unsafe-option) ✗ ./bin/unittest_compression
2017-03-01 14:29:10.801193 7fca8f4b2f00 -1 WARNING: all dangerous and experimental features are enabled.
2017-03-01 14:29:10.801358 7fca8f4b2f00 -1 WARNING: all dangerous and experimental features are enabled.
2017-03-01 14:29:10.802573 7fca8f4b2f00 -1 WARNING: all dangerous and experimental features are enabled.
[==========] Running 68 tests from 3 test cases.
[----------] Global test environment set-up.
[----------] 3 tests from ZlibCompressor
[ RUN      ] ZlibCompressor.zlib_isal_compatibility
[       OK ] ZlibCompressor.zlib_isal_compatibility (0 ms)
[ RUN      ] ZlibCompressor.isal_compress_zlib_decompress_random
[       OK ] ZlibCompressor.isal_compress_zlib_decompress_random (556 ms)
[ RUN      ] ZlibCompressor.isal_compress_zlib_decompress_walk
[       OK ] ZlibCompressor.isal_compress_zlib_decompress_walk (451 ms)

Contributor

Liuchang0812 commented Mar 1, 2017

🤣 I need help. It seems that ctest could not find some *.so files, but it is ok when I try to run unittest binary directly. What should I do to fix this problem? @tchaikov

more logs as following:

➜  build git:(wip-19106-eliminate-unsafe-option) ✗ ctest -VV -R unittest_compression
UpdateCTestConfiguration  from :/home/liuchang/WorkSpace/ceph/build/DartConfiguration.tcl
UpdateCTestConfiguration  from :/home/liuchang/WorkSpace/ceph/build/DartConfiguration.tcl
Test project /home/liuchang/WorkSpace/ceph/build
Constructing a list of tests
Done constructing a list of tests
Checking test dependency graph...
Checking test dependency graph end
test 85
    Start 85: unittest_compression

85: Test command: /home/liuchang/WorkSpace/ceph/build/bin/unittest_compression
85: Environment variables:
85:  CEPH_ROOT=/home/liuchang/WorkSpace/ceph
85:  CEPH_BIN=/home/liuchang/WorkSpace/ceph/build/bin
85:  CEPH_LIB=/home/liuchang/WorkSpace/ceph/build/lib
85:  CEPH_BUILD_DIR=/home/liuchang/WorkSpace/ceph/build
85:  LD_LIBRARY_PATH=/home/liuchang/WorkSpace/ceph/build/lib
85:  PATH=/home/liuchang/WorkSpace/ceph/build/bin:/home/liuchang/WorkSpace/ceph/src:/home/liuchang/bin:/home/liuchang/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin
85:  PYTHONPATH=/home/liuchang/WorkSpace/ceph/build/lib/cython_modules/lib.2:/home/liuchang/WorkSpace/ceph/src/pybind
85:  CEPH_BUILD_VIRTUALENV=/tmp
85: Test timeout computed to be: 3600
85: 2017-03-01 14:27:35.522809 7fc7e144af00 -1 did not load config file, using default settings.
85: [==========] Running 68 tests from 3 test cases.
85: [----------] Global test environment set-up.
85: [----------] 3 tests from ZlibCompressor
85: [ RUN      ] ZlibCompressor.zlib_isal_compatibility
85: *** Caught signal (Segmentation fault) **
85:  in thread 7fc7e144af00 thread_name:unittest_compre
85: 2017-03-01 14:27:35.525576 7fc7e144af00 -1 load failed dlopen(): "/usr/local/lib/ceph/compressor/libceph_zlib.so: cannot open shared object file: No such file or directory" or "/usr/local/lib/ceph/libceph_zlib.so: cannot open shared object file: No such file or directory"
85: 2017-03-01 14:27:35.525591 7fc7e144af00 -1 create cannot load compressor of type zlib


➜  build git:(wip-19106-eliminate-unsafe-option) ✗ ./bin/unittest_compression
2017-03-01 14:29:10.801193 7fca8f4b2f00 -1 WARNING: all dangerous and experimental features are enabled.
2017-03-01 14:29:10.801358 7fca8f4b2f00 -1 WARNING: all dangerous and experimental features are enabled.
2017-03-01 14:29:10.802573 7fca8f4b2f00 -1 WARNING: all dangerous and experimental features are enabled.
[==========] Running 68 tests from 3 test cases.
[----------] Global test environment set-up.
[----------] 3 tests from ZlibCompressor
[ RUN      ] ZlibCompressor.zlib_isal_compatibility
[       OK ] ZlibCompressor.zlib_isal_compatibility (0 ms)
[ RUN      ] ZlibCompressor.isal_compress_zlib_decompress_random
[       OK ] ZlibCompressor.isal_compress_zlib_decompress_random (556 ms)
[ RUN      ] ZlibCompressor.isal_compress_zlib_decompress_walk
[       OK ] ZlibCompressor.isal_compress_zlib_decompress_walk (451 ms)

@Liuchang0812

This comment has been minimized.

Show comment
Hide comment
@Liuchang0812

Liuchang0812 Mar 1, 2017

Contributor

@tchaikov Do you have time to take a look ?

Contributor

Liuchang0812 commented Mar 1, 2017

@tchaikov Do you have time to take a look ?

@Liuchang0812

This comment has been minimized.

Show comment
Hide comment
@Liuchang0812

Liuchang0812 Mar 1, 2017

Contributor

ok, I found that PluginRegistry::load depends on cct->_conf->plugin_dir.

I print cc->_conf->plugin_dir in log, I found that:

# in ctest
➜  build git:(wip-19106-eliminate-unsafe-option) ✗ ctest -VV -R unittest_compression
UpdateCTestConfiguration  from :/home/liuchang/WorkSpace/ceph/build/DartConfiguration.tcl
UpdateCTestConfiguration  from :/home/liuchang/WorkSpace/ceph/build/DartConfiguration.tcl
Test project /home/liuchang/WorkSpace/ceph/build
Constructing a list of tests
Done constructing a list of tests
Checking test dependency graph...
Checking test dependency graph end
test 85
    Start 85: unittest_compression

85: Test command: /home/liuchang/WorkSpace/ceph/build/bin/unittest_compression
85: Environment variables:
85:  CEPH_ROOT=/home/liuchang/WorkSpace/ceph
85:  CEPH_BIN=/home/liuchang/WorkSpace/ceph/build/bin
85:  CEPH_LIB=/home/liuchang/WorkSpace/ceph/build/lib
85:  CEPH_BUILD_DIR=/home/liuchang/WorkSpace/ceph/build
85:  LD_LIBRARY_PATH=/home/liuchang/WorkSpace/ceph/build/lib
85:  PATH=/home/liuchang/WorkSpace/ceph/build/bin:/home/liuchang/WorkSpace/ceph/src:/home/liuchang/bin:/home/liuchang/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin
85:  PYTHONPATH=/home/liuchang/WorkSpace/ceph/build/lib/cython_modules/lib.2:/home/liuchang/WorkSpace/ceph/src/pybind
85:  CEPH_BUILD_VIRTUALENV=/tmp
85: Test timeout computed to be: 3600
85: 2017-03-01 20:55:09.056527 7f412168af00 -1 did not load config file, using default settings.
85: [==========] Running 68 tests from 3 test cases.
85: [----------] Global test environment set-up.   
85: [----------] 3 tests from ZlibCompressor
85: [ RUN      ] ZlibCompressor.zlib_isal_compatibility
85: 2017-03-01 20:55:09.058956 7f412168af00 -1 DEBUG plugin_dir is: /usr/local/lib/ceph


# run testcase 

➜  build git:(wip-19106-eliminate-unsafe-option) ✗ ./bin/unittest_compression
2017-03-01 20:59:47.847400 7f49e7b28f00 -1 WARNING: all dangerous and experimental features are enabled.
2017-03-01 20:59:47.847768 7f49e7b28f00 -1 WARNING: all dangerous and experimental features are enabled.
2017-03-01 20:59:47.857325 7f49e7b28f00 -1 WARNING: all dangerous and experimental features are enabled.
[==========] Running 68 tests from 3 test cases.
[----------] Global test environment set-up.
[----------] 3 tests from ZlibCompressor
[ RUN      ] ZlibCompressor.zlib_isal_compatibility
2017-03-01 20:59:47.858560 7f49e7b28f00 -1 DEBUG plugin_dir is: /home/liuchang/WorkSpace/ceph/build/lib
[       OK ] ZlibCompressor.zlib_isal_compatibility (15 ms)
[ RUN      ] ZlibCompressor.isal_compress_zlib_decompress_random
[       OK ] ZlibCompressor.isal_compress_zlib_decompress_random (544 ms)
[ RUN      ] ZlibCompressor.isal_compress_zlib_decompress_walk
[       OK ] ZlibCompressor.isal_compress_zlib_decompress_walk (441 ms)

@tchaikov I'm not familiar with cmake. is there any magic in ctest ?

Contributor

Liuchang0812 commented Mar 1, 2017

ok, I found that PluginRegistry::load depends on cct->_conf->plugin_dir.

I print cc->_conf->plugin_dir in log, I found that:

# in ctest
➜  build git:(wip-19106-eliminate-unsafe-option) ✗ ctest -VV -R unittest_compression
UpdateCTestConfiguration  from :/home/liuchang/WorkSpace/ceph/build/DartConfiguration.tcl
UpdateCTestConfiguration  from :/home/liuchang/WorkSpace/ceph/build/DartConfiguration.tcl
Test project /home/liuchang/WorkSpace/ceph/build
Constructing a list of tests
Done constructing a list of tests
Checking test dependency graph...
Checking test dependency graph end
test 85
    Start 85: unittest_compression

85: Test command: /home/liuchang/WorkSpace/ceph/build/bin/unittest_compression
85: Environment variables:
85:  CEPH_ROOT=/home/liuchang/WorkSpace/ceph
85:  CEPH_BIN=/home/liuchang/WorkSpace/ceph/build/bin
85:  CEPH_LIB=/home/liuchang/WorkSpace/ceph/build/lib
85:  CEPH_BUILD_DIR=/home/liuchang/WorkSpace/ceph/build
85:  LD_LIBRARY_PATH=/home/liuchang/WorkSpace/ceph/build/lib
85:  PATH=/home/liuchang/WorkSpace/ceph/build/bin:/home/liuchang/WorkSpace/ceph/src:/home/liuchang/bin:/home/liuchang/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin
85:  PYTHONPATH=/home/liuchang/WorkSpace/ceph/build/lib/cython_modules/lib.2:/home/liuchang/WorkSpace/ceph/src/pybind
85:  CEPH_BUILD_VIRTUALENV=/tmp
85: Test timeout computed to be: 3600
85: 2017-03-01 20:55:09.056527 7f412168af00 -1 did not load config file, using default settings.
85: [==========] Running 68 tests from 3 test cases.
85: [----------] Global test environment set-up.   
85: [----------] 3 tests from ZlibCompressor
85: [ RUN      ] ZlibCompressor.zlib_isal_compatibility
85: 2017-03-01 20:55:09.058956 7f412168af00 -1 DEBUG plugin_dir is: /usr/local/lib/ceph


# run testcase 

➜  build git:(wip-19106-eliminate-unsafe-option) ✗ ./bin/unittest_compression
2017-03-01 20:59:47.847400 7f49e7b28f00 -1 WARNING: all dangerous and experimental features are enabled.
2017-03-01 20:59:47.847768 7f49e7b28f00 -1 WARNING: all dangerous and experimental features are enabled.
2017-03-01 20:59:47.857325 7f49e7b28f00 -1 WARNING: all dangerous and experimental features are enabled.
[==========] Running 68 tests from 3 test cases.
[----------] Global test environment set-up.
[----------] 3 tests from ZlibCompressor
[ RUN      ] ZlibCompressor.zlib_isal_compatibility
2017-03-01 20:59:47.858560 7f49e7b28f00 -1 DEBUG plugin_dir is: /home/liuchang/WorkSpace/ceph/build/lib
[       OK ] ZlibCompressor.zlib_isal_compatibility (15 ms)
[ RUN      ] ZlibCompressor.isal_compress_zlib_decompress_random
[       OK ] ZlibCompressor.isal_compress_zlib_decompress_random (544 ms)
[ RUN      ] ZlibCompressor.isal_compress_zlib_decompress_walk
[       OK ] ZlibCompressor.isal_compress_zlib_decompress_walk (441 ms)

@tchaikov I'm not familiar with cmake. is there any magic in ctest ?

@gregsfortytwo

This comment has been minimized.

Show comment
Hide comment
@gregsfortytwo

gregsfortytwo Mar 1, 2017

Member

It looks like this just eliminates the safe/unsafe tracking. I think the point of that card is to make all config sets have safe behavior?

Member

gregsfortytwo commented Mar 1, 2017

It looks like this just eliminates the safe/unsafe tracking. I think the point of that card is to make all config sets have safe behavior?

@Liuchang0812

This comment has been minimized.

Show comment
Hide comment
@Liuchang0812

Liuchang0812 Mar 2, 2017

Contributor

@gregsfortytwo thanks for your comment.

I think that i don't eliminates the safe/unsafe tracking. md_config_t has is_safe() 1 function which makes a type check. integer and floating point options considered thread safe by default. we could use SAFE_OPTION to declare a safe config as 2 . OPTION with string type is unsafe default. So we do not need the safe parameter in md_config_t::set_val 3 .

I am not sure whether i get correct point of the trello card. @liewegas Would you mind taking a look at this question?

Contributor

Liuchang0812 commented Mar 2, 2017

@gregsfortytwo thanks for your comment.

I think that i don't eliminates the safe/unsafe tracking. md_config_t has is_safe() 1 function which makes a type check. integer and floating point options considered thread safe by default. we could use SAFE_OPTION to declare a safe config as 2 . OPTION with string type is unsafe default. So we do not need the safe parameter in md_config_t::set_val 3 .

I am not sure whether i get correct point of the trello card. @liewegas Would you mind taking a look at this question?

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Mar 2, 2017

Member

The point of hte card is to fix all of the callers that pass safe=false to set_val. They currently have to do that because the system is past the init phsae when we're free to update these at will and the options don't have observers. Each caller needs to be looked at individually to figure out if/how that's possible. Only after all the callers are fixed can we drop the option!

For example, in global_init.cc, instead of changing the setuser value and then checking it 15 lines down, use getuid() to decide whether whether to check the fields at all.

Member

liewegas commented Mar 2, 2017

The point of hte card is to fix all of the callers that pass safe=false to set_val. They currently have to do that because the system is past the init phsae when we're free to update these at will and the options don't have observers. Each caller needs to be looked at individually to figure out if/how that's possible. Only after all the callers are fixed can we drop the option!

For example, in global_init.cc, instead of changing the setuser value and then checking it 15 lines down, use getuid() to decide whether whether to check the fields at all.

@Liuchang0812

This comment has been minimized.

Show comment
Hide comment
@Liuchang0812

Liuchang0812 Mar 7, 2017

Contributor

@liewegas 👍 thanks your explaining. I get its point now.

in global_init.cc, We could configure conf->setuid forced because of that getuid function guards the function atomic, other threads can't visit those values. is it right?

Contributor

Liuchang0812 commented Mar 7, 2017

@liewegas 👍 thanks your explaining. I get its point now.

in global_init.cc, We could configure conf->setuid forced because of that getuid function guards the function atomic, other threads can't visit those values. is it right?

@Liuchang0812

This comment has been minimized.

Show comment
Hide comment
@Liuchang0812

Liuchang0812 Mar 7, 2017

Contributor

In other words, We could remove safe param if we have used SAFE_OPTION to declare all options that modified in this patch.

Contributor

Liuchang0812 commented Mar 7, 2017

In other words, We could remove safe param if we have used SAFE_OPTION to declare all options that modified in this patch.

@Liuchang0812 Liuchang0812 changed the title from common/config: eliminate config_t::set_val unsafe option to 【DNM】common/config: eliminate config_t::set_val unsafe option Mar 7, 2017

@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Mar 7, 2017

Member

We could, but that doesn't improve the situation. The problem is that we occasionally see valgrind errors showing a corrupted std::string in the config structure. The point of removing the 'unsafe' set_val is to structurally make it impossible for the config to be modified once it is loaded without using the observer framework (which is allows config options to be read under a lock).

The first step is to fix up any users that simply don't need to use it. The setuid one is an easy example.. there's no reason to change the string at all; we can just restructure the calling code.

Member

liewegas commented Mar 7, 2017

We could, but that doesn't improve the situation. The problem is that we occasionally see valgrind errors showing a corrupted std::string in the config structure. The point of removing the 'unsafe' set_val is to structurally make it impossible for the config to be modified once it is loaded without using the observer framework (which is allows config options to be read under a lock).

The first step is to fix up any users that simply don't need to use it. The setuid one is an easy example.. there's no reason to change the string at all; we can just restructure the calling code.

@liewegas liewegas self-assigned this Mar 8, 2017

common/config: eliminate config_t::set_val unsafe option
Fixes: http://tracker.ceph.com/issues/19106

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
addr = "127.0.0.1:15000";
port_addr = "127.0.0.1:15001";
} else {
g_ceph_context->_conf->set_val("ms_type", "async+dpdk", false, false);
g_ceph_context->_conf->set_val("ms_dpdk_debug_allow_loopback", "true", false, false);
g_ceph_context->_conf->set_val("ms_async_op_threads", "2", false, false);

This comment has been minimized.

@Liuchang0812

Liuchang0812 Mar 21, 2017

Contributor

ms_async_op_threads is a int config. it's safe. no need to pass safe=false

@Liuchang0812

Liuchang0812 Mar 21, 2017

Contributor

ms_async_op_threads is a int config. it's safe. no need to pass safe=false

addr = "127.0.0.1:15000";
port_addr = "127.0.0.1:15001";
} else {
g_ceph_context->_conf->set_val("ms_type", "async+dpdk", false, false);
g_ceph_context->_conf->set_val("ms_dpdk_debug_allow_loopback", "true", false, false);

This comment has been minimized.

@Liuchang0812

Liuchang0812 Mar 21, 2017

Contributor

OPTION(ms_dpdk_debug_allow_loopback, OPT_BOOL, false)

It's OPT_BOOL, no need to pass safe=false

@Liuchang0812

Liuchang0812 Mar 21, 2017

Contributor

OPTION(ms_dpdk_debug_allow_loopback, OPT_BOOL, false)

It's OPT_BOOL, no need to pass safe=false

common: convert plugin_dir and erasure_plugin_dir to SAFE_OPTION
Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
@@ -209,11 +209,13 @@ void Pipe::start_reader()
void Pipe::maybe_start_delay_thread()
{
if (!delay_thread &&

This comment has been minimized.

@Liuchang0812

Liuchang0812 Mar 21, 2017

Contributor

this line is too long. split to 3 lines.

@Liuchang0812

Liuchang0812 Mar 21, 2017

Contributor

this line is too long. split to 3 lines.

@Liuchang0812

This comment has been minimized.

Show comment
Hide comment
@Liuchang0812

Liuchang0812 Mar 22, 2017

Contributor

@liewegas

  1. setuser/setgroup: they are no need to modify. We could just use getuid to decide whether to do the work.
  2. ms_async_op_threads/ms_dpdk_debug_allow_loopback: they are POD types. We could eliminate safe=false option in set_val.
  3. other options are modified in run-time. They are converted to SAFE_OPTION.

Now, there are no functions that need safe=false option.

Please review this commit carefully: bb72c76

There are some works we could do in future:

  1. Give template function template <typename T> T md_config_t::get_val default type std::string. Now there are almost g_conf->get_val<std::string>("") code. We could make code more readable.
  2. For OPTION_STR, We should give explicit tips if user try to set_val them, rather then that do not apply changes now. It would be better if we could check it in compile time(use static_assert?).
  3. We should disable all g_conf->foo calling mode of OPTION_STR. We couldn't do it now, because there are too much code need to be changed. Maybe we could introduce macro FLAGS_foo like gflags?
Contributor

Liuchang0812 commented Mar 22, 2017

@liewegas

  1. setuser/setgroup: they are no need to modify. We could just use getuid to decide whether to do the work.
  2. ms_async_op_threads/ms_dpdk_debug_allow_loopback: they are POD types. We could eliminate safe=false option in set_val.
  3. other options are modified in run-time. They are converted to SAFE_OPTION.

Now, there are no functions that need safe=false option.

Please review this commit carefully: bb72c76

There are some works we could do in future:

  1. Give template function template <typename T> T md_config_t::get_val default type std::string. Now there are almost g_conf->get_val<std::string>("") code. We could make code more readable.
  2. For OPTION_STR, We should give explicit tips if user try to set_val them, rather then that do not apply changes now. It would be better if we could check it in compile time(use static_assert?).
  3. We should disable all g_conf->foo calling mode of OPTION_STR. We couldn't do it now, because there are too much code need to be changed. Maybe we could introduce macro FLAGS_foo like gflags?
Show outdated Hide outdated src/global/global_init.cc Outdated
@Liuchang0812

This comment has been minimized.

Show comment
Hide comment
@Liuchang0812

Liuchang0812 Mar 22, 2017

Contributor
Contributor

Liuchang0812 commented Mar 22, 2017

@Liuchang0812 Liuchang0812 changed the title from 【DNM】common/config: eliminate config_t::set_val unsafe option to common/config: eliminate config_t::set_val unsafe option Mar 22, 2017

Liuchang0812 added some commits Mar 21, 2017

os/bluestore: convert rocksdb_db_paths to SAFE_OPTION
We need to change `rocksdb_db_paths` in runtime. pass safe=false to set_val is not
a good behavior. We should use SAFE_OPTION.

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
common: convert ms_type option to SAFE_OPTION
We need to modify ms_type in unittest. That use SAFE_OPTION to declare ms_type
is safer than pass `safe=false` to set_val.

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
msg/async: convert ms_dpdk_coremask to SAFE_OPTION
ms_dpdk_coremask is modified in test/msgr/test_async_networkstack.cc. It's better
to declare SAFE_OPTION.

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
msg/async: convert ms_dpdk_host_ipv4_addr to SAFE_OPTION
ms_dpdk_host_ipv4_addr is modified in src/test/msgr/test_async_networkstack.cc,
It's better to use SAFE_OPTION.

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
msg/async: convert ms_dpdk_gateway_ipv4_addr to SAFE_OPTION
We would like to eliminate `safe=false` parameter in opt.set_val. ms_dpdk_gateway_ipv4_addr
is modified in runtime. We convert it to SAFE_OPTION to eliminate `safe=fales` and
guard thread safity.

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
msg/async: convert ms_dpdk_netmask_ipv4_addr to SAFE_OPTION
We would like to eliminate `safe=false` parameter in opt.set_val. ms_dpdk_netmask_ipv4_addr
is modified in runtime. We convert it to SAFE_OPTION to eliminate `safe=fales` and
guard thread safity.

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
msg: convert ms_inject_delay_type to SAFE_OPTION
We would like to eliminate `safe=false` parameter in opt.set_val. ms_inject_delay_type
is modified in runtime. We convert it to SAFE_OPTION to eliminate `safe=fales` and
guard thread safity.

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Mar 22, 2017

Member

not sure about 3; but an easier step would be to rename the member varaible for SAFE_OPTION to _safe##name## or something so that someone trying to do g_conf->some_safe_option will fail to compile.

Anyway, this series looks good to me!

Member

liewegas commented Mar 22, 2017

not sure about 3; but an easier step would be to rename the member varaible for SAFE_OPTION to _safe##name## or something so that someone trying to do g_conf->some_safe_option will fail to compile.

Anyway, this series looks good to me!

@liewegas liewegas added the needs-qa label Mar 22, 2017

common/global: refactor global_init to avoid setuser/setgroup modific…
…ation

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>

@liewegas liewegas merged commit eda8d28 into ceph:master Mar 28, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@Liuchang0812 Liuchang0812 deleted the Liuchang0812:wip-19106-eliminate-unsafe-option branch Mar 28, 2017

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