| Message ID | 20220428190831.15251-1-msuchanek@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 12dc1b568b1ff60e379126ff236e64b4fa6064b9 |
| Headers | show |
| Series | [ndctl] test: monitor: Use in-tree configuration file | expand |
On Thu, Apr 28, 2022 at 12:09 PM Michal Suchanek <msuchanek@suse.de> wrote: > > When ndctl is not installed /etc/ndctl.conf.d does not exist and the > monitor fails to start. Use in-tree configuration for testing. > Looks reasonable to me: Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > --- > test/monitor.sh | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/test/monitor.sh b/test/monitor.sh > index e58c908..c5beb2c 100755 > --- a/test/monitor.sh > +++ b/test/monitor.sh > @@ -13,6 +13,8 @@ smart_supported_bus="" > > . $(dirname $0)/common > > +monitor_conf="$TEST_PATH/../ndctl" > + > check_prereq "jq" > > trap 'err $LINENO' ERR > @@ -22,7 +24,7 @@ check_min_kver "4.15" || do_skip "kernel $KVER may not support monitor service" > start_monitor() > { > logfile=$(mktemp) > - $NDCTL monitor -l $logfile $1 & > + $NDCTL monitor -c "$monitor_conf" -l $logfile $1 & > monitor_pid=$! > sync; sleep 3 > truncate --size 0 $logfile #remove startup log > -- > 2.36.0 > >
On 4/29/22 00:38, Michal Suchanek wrote: > When ndctl is not installed /etc/ndctl.conf.d does not exist and the > monitor fails to start. Use in-tree configuration for testing. > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > --- > test/monitor.sh | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/test/monitor.sh b/test/monitor.sh > index e58c908..c5beb2c 100755 > --- a/test/monitor.sh > +++ b/test/monitor.sh > @@ -13,6 +13,8 @@ smart_supported_bus="" > > . $(dirname $0)/common > > +monitor_conf="$TEST_PATH/../ndctl" Though this patch gets the monitor to "listening" mode, its not really parsing anything from the $TEST_PATH/../ndctl There are two issues here. 1) Using the iniparser for parsing the monitor config file when the parser is set to parse_monitor_config() for monitor. I have posted a patch for this at https://patchwork.kernel.org/project/linux-nvdimm/patch/164750955519.2000193.16903542741359443926.stgit@LAPTOP-TBQTPII8/ 2) The directory passed in -c would silently be ignored in parse_monitor_config() during fseek() failure. The command proceeds to monitor everything. Should the -c option be made to accept the directory as argument? Thanks, Shivaprasad
On Mon, May 02, 2022 at 03:03:41PM +0530, Shivaprasad G Bhat wrote: > On 4/29/22 00:38, Michal Suchanek wrote: > > When ndctl is not installed /etc/ndctl.conf.d does not exist and the > > monitor fails to start. Use in-tree configuration for testing. > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > --- > > test/monitor.sh | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/test/monitor.sh b/test/monitor.sh > > index e58c908..c5beb2c 100755 > > --- a/test/monitor.sh > > +++ b/test/monitor.sh > > @@ -13,6 +13,8 @@ smart_supported_bus="" > > . $(dirname $0)/common > > +monitor_conf="$TEST_PATH/../ndctl" > > Though this patch gets the monitor to "listening" mode, > its not really parsing anything from the $TEST_PATH/../ndctl > > There are two issues here. > 1) Using the iniparser for parsing the monitor config file > when the parser is set to parse_monitor_config() for monitor. > I have posted a patch for this at https://patchwork.kernel.org/project/linux-nvdimm/patch/164750955519.2000193.16903542741359443926.stgit@LAPTOP-TBQTPII8/ > > 2) The directory passed in -c would silently be ignored > in parse_monitor_config() during fseek() failure. The command proceeds to > monitor everything. > > Should the -c option be made to accept the directory as argument? The documentation of -c states: -c, --config-file= Provide the config file(s) to use. This overrides the default config typically found in /etc/ndctl.conf.d It is not clear from this description if file or directory is expected. Given that it suggests multiple files can be provided it implies a directory is accepted. Either the description should be clarified or directory accepted. Thanks Michal
On Mon, May 02, 2022 at 03:03:41PM +0530, Shivaprasad G Bhat wrote: > On 4/29/22 00:38, Michal Suchanek wrote: > > When ndctl is not installed /etc/ndctl.conf.d does not exist and the > > monitor fails to start. Use in-tree configuration for testing. > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > --- > > test/monitor.sh | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/test/monitor.sh b/test/monitor.sh > > index e58c908..c5beb2c 100755 > > --- a/test/monitor.sh > > +++ b/test/monitor.sh > > @@ -13,6 +13,8 @@ smart_supported_bus="" > > . $(dirname $0)/common > > +monitor_conf="$TEST_PATH/../ndctl" > > Though this patch gets the monitor to "listening" mode, > its not really parsing anything from the $TEST_PATH/../ndctl Yes, very likely. The tests do pass so not failing to not parse the file is good enough at this point. > > There are two issues here. > 1) Using the iniparser for parsing the monitor config file > when the parser is set to parse_monitor_config() for monitor. > I have posted a patch for this at https://patchwork.kernel.org/project/linux-nvdimm/patch/164750955519.2000193.16903542741359443926.stgit@LAPTOP-TBQTPII8/ Thanks for fixing this. I am not really using the monitor for anything so I did not notice. > > 2) The directory passed in -c would silently be ignored > in parse_monitor_config() during fseek() failure. The command proceeds to > monitor everything. > > Should the -c option be made to accept the directory as argument? This has been already asked, and as far as I am concerned the answere remains the same: The documentation of the -c option talks about files which implies a directory should be accepted. This can be resolved either by clarifying the documentation to make it clear only single file is accepted for -c, or accepting a directory. Thanks Michal
diff --git a/test/monitor.sh b/test/monitor.sh index e58c908..c5beb2c 100755 --- a/test/monitor.sh +++ b/test/monitor.sh @@ -13,6 +13,8 @@ smart_supported_bus="" . $(dirname $0)/common +monitor_conf="$TEST_PATH/../ndctl" + check_prereq "jq" trap 'err $LINENO' ERR @@ -22,7 +24,7 @@ check_min_kver "4.15" || do_skip "kernel $KVER may not support monitor service" start_monitor() { logfile=$(mktemp) - $NDCTL monitor -l $logfile $1 & + $NDCTL monitor -c "$monitor_conf" -l $logfile $1 & monitor_pid=$! sync; sleep 3 truncate --size 0 $logfile #remove startup log
When ndctl is not installed /etc/ndctl.conf.d does not exist and the monitor fails to start. Use in-tree configuration for testing. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- test/monitor.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)