Re: [Fuego] [PATCH] samba: Add test case to access the target from host using samba.

Hi,
Tim

>> +sed -i 's/test_target/'"$test_target"'/g' data/smb05.conf
>Is there some reason this can't use 'localhost' or '127.0.0.1'
>instead of $test_target here?

The host_allow in samba.conf is used to set the ip address that can access the samba server. 
In the test, the target accesses the samba server established on itself, still needs to specify the ip address of the target. 
Specified as localhost cannot access samba server.

The interfaces can be specified to localhost.

Other items have been fixed, and I will resubmit the patch later.

Thanks,
 -- Wangmy

-----Original Message-----
From: [email protected] [mailto:[email protected]] 
Sent: Tuesday, November 19, 2019 11:37 PM
To: Wang, Mingyu/王 鸣瑜 <[email protected]>; [email protected]
Subject: RE: [Fuego] [PATCH] samba: Add test case to access the target from host using samba.

This looks like a nice upgrade, but I have a couple of comments.

> -----Original Message-----
> From: Wang Mingyu
> 
> Signed-off-by: Wang Mingyu <[email protected]>
> ---
>  tests/Functional.samba/data/smb04.conf        |   4 +-
>  tests/Functional.samba/data/smb05.conf        |  19 ++++
>  tests/Functional.samba/data/smb06.conf        |  18 +++
>  tests/Functional.samba/fuego_test.sh          |  20 +++-
>  tests/Functional.samba/samba_test.sh          |   8 +-
>  tests/Functional.samba/test.yaml              |  10 +-
>  .../Functional.samba/tests/restore_target.sh  |  24 ++++  
> tests/Functional.samba/tests/test_samba04.sh  |  16 +--  
> tests/Functional.samba/tests/test_samba05.sh  | 103
> ++++++++++++++++++
>  tests/Functional.samba/tests/test_samba06.sh  |  58 ++++++++++
>  .../tests/test_samba06_host.sh                |  23 ++++
>  11 files changed, 278 insertions(+), 25 deletions(-)  create mode 
> 100644 tests/Functional.samba/data/smb05.conf
>  create mode 100644 tests/Functional.samba/data/smb06.conf
>  mode change 100644 => 100755 tests/Functional.samba/samba_test.sh
>  create mode 100644 tests/Functional.samba/tests/restore_target.sh
>  create mode 100644 tests/Functional.samba/tests/test_samba05.sh
>  create mode 100644 tests/Functional.samba/tests/test_samba06.sh
>  create mode 100644 tests/Functional.samba/tests/test_samba06_host.sh
> 
> diff --git a/tests/Functional.samba/data/smb04.conf
> b/tests/Functional.samba/data/smb04.conf
> index 94acbbc..c836021 100644
> --- a/tests/Functional.samba/data/smb04.conf
> +++ b/tests/Functional.samba/data/smb04.conf
> @@ -6,14 +6,12 @@
>     load printers = yes
>     log file = /var/log/samba/log.%m
>     max log size = 50
> -   max protocol = SMB3_10
> -   min protocol = SMB2
>     interfaces = 127.0.0.1 test_target
>     dns proxy = no
> 
>  [test]
>     comment = samba server
> -   path = /home/test_for_fuego/samba
> +   path = /home/test/samba
>     writable = yes
>     browseable = yes
>     guest ok = yes
> diff --git a/tests/Functional.samba/data/smb05.conf
> b/tests/Functional.samba/data/smb05.conf
> new file mode 100644
> index 0000000..63d3d02
> --- /dev/null
> +++ b/tests/Functional.samba/data/smb05.conf
> @@ -0,0 +1,19 @@
> +[global]
> +   workgroup = MYGROUP
> +   server string = Samba Server
> +   security = user
> +   passdb backend = tdbsam
> +   hosts allow = 127.0.0.1 test_target
> +   load printers = no
> +   log file = /var/log/samba/log.%m
> +   max log size = 50
> +   max protocol = SMB3_10
> +   min protocol = SMB2
> +   interfaces = 127.0.0.1 test_target
> +   dns proxy = no
> +
> +[test]
> +   comment = samba server
> +   path = /home/test/samba
> +   writable = yes
> +   browseable = yes
> diff --git a/tests/Functional.samba/data/smb06.conf
> b/tests/Functional.samba/data/smb06.conf
> new file mode 100644
> index 0000000..1bbacd4
> --- /dev/null
> +++ b/tests/Functional.samba/data/smb06.conf
> @@ -0,0 +1,18 @@
> +[global]
> +   workgroup = MYGROUP
> +   server string = Samba Server
> +   security = user
> +   hosts allow = 127.0.0.1 test_host
> +   load printers = yes
> +   log file = /var/log/samba/log.%m
> +   max log size = 50
> +   interfaces = 127.0.0.1 test_target
> +   dns proxy = no
> +
> +[test]
> +   comment = samba server
> +   path = /home/test/samba
> +   writable = yes
> +   browseable = yes
> +   guest ok = yes
> +   public = yes
> diff --git a/tests/Functional.samba/fuego_test.sh
> b/tests/Functional.samba/fuego_test.sh
> index 1f694ef..63e0cef 100644
> --- a/tests/Functional.samba/fuego_test.sh
> +++ b/tests/Functional.samba/fuego_test.sh
> @@ -1,7 +1,9 @@
> +NEED_ROOT=1
> +
>  function test_pre_check {
> +    assert_has_program samba
>      assert_has_program expect
>      assert_has_program smbclient
> -    assert_has_program sed
You use 'sed' below.  I'm not sure why you are removing this assertion.
'sed' is not one of the programs required by Fuego, so you should check for it.

>  }
> 
>  function test_deploy {
> @@ -12,9 +14,19 @@ function test_deploy {  }
> 
>  function test_run {
> -    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; \
> -        export test_target=$IPADDR; \
> -        ./samba_test.sh"
> +    report "cd $BOARD_TESTDIR/fuego.$TESTDIR;\
> +    export test_target=$IPADDR;\
> +    export test_host=$SRV_IP;\
> +    ./samba_test.sh"
> +
> +    service_file=/tmp/test_for_fuego
> +    report_append "cd $BOARD_TESTDIR/fuego.$TESTDIR;\
> +    sh tests/test_samba06.sh $SRV_IP $IPADDR $service_file"
> +
> +    log_this "cd $TEST_HOME; sh tests/test_samba06_host.sh $IPADDR"
> +
> +    report_append "cd $BOARD_TESTDIR/fuego.$TESTDIR;\
> +    sh tests/restore_target.sh $service_file"
Does this need to be a 'report_append'?  Is there something from restore_target.sh that needs to go into the test log?  I can't see anything.
Please convert this to just 'cmd'.

>  }
> 
>  function test_processing {
> diff --git a/tests/Functional.samba/samba_test.sh
> b/tests/Functional.samba/samba_test.sh
> old mode 100644
> new mode 100755
> index 95c34db..93b0740
> --- a/tests/Functional.samba/samba_test.sh
> +++ b/tests/Functional.samba/samba_test.sh
> @@ -2,6 +2,8 @@
>  . ./fuego_board_function_lib.sh
>  set_init_manager
> 
> -for i in tests/*.sh; do
> -    source $i
> -done
> +source tests/test_samba01.sh
> +source tests/test_samba02.sh
> +source tests/test_samba03.sh
> +source tests/test_samba04.sh
> +source tests/test_samba05.sh
> diff --git a/tests/Functional.samba/test.yaml
> b/tests/Functional.samba/test.yaml
> index bd61de6..f38dcf9 100644
> --- a/tests/Functional.samba/test.yaml
> +++ b/tests/Functional.samba/test.yaml
> @@ -1,11 +1,9 @@
>  fuego_package_version: 1
>  name: Functional.samba
>  description: |
> -      The Samba software suite is a collection of programs that implements
> -      the Server Message Block protocol for UNIX systems.
> -      This is a simple test to check the ps/pid/syslog of samba and use
> -      smbclient to implement a simple ftp-like client.
> -license: BSD-3-Clause
> +      The Samba software suite is a collection of programs that 
> + implements
> the Server Message Block protocol for UNIX systems.
> +      This is a simple test to check the ps/pid/syslog of samba and 
> + use
> smbclient to implement a simple ftp-like client.
Please keep descriptions to 80 columns.

> +license: BSD-3-Clause
>  author: Wang Mingyu <[email protected]>
>  maintainer: Wang Mingyu <[email protected]>
>  version: 1.00
> @@ -15,6 +13,8 @@ tags: ['samba']
>  params:
>      IPADDR:
>          description: the IP address of your target board - Defined in 
> board file
> +    SRV_IP:
> +        description: the IP address of your host - Defined in board 
> + file
>  data_files:
>   - samba_test.sh
>   - fuego_test.sh
> diff --git a/tests/Functional.samba/tests/restore_target.sh
> b/tests/Functional.samba/tests/restore_target.sh
> new file mode 100644
> index 0000000..6610923
> --- /dev/null
> +++ b/tests/Functional.samba/tests/restore_target.sh
> @@ -0,0 +1,24 @@
> +#!/bin/sh
> +
> +#  In the target start samba, and confirm the process condition by 
> +command
> ps.
> +#  check the keyword "smbd".
This comment appears to be a copy-paste bug.  It does not describe this file.  Please update it.

> +
> +. ./fuego_board_function_lib.sh
> +set_init_manager
> +
> +if [ -f /etc/samba/smb.conf_bak ]
> +then
> +    mv /etc/samba/smb.conf_bak /etc/samba/smb.conf fi mv 
> +data/smb06.conf_bak data/smb06.conf
> +
> +if [ "$(tail -n 1 $1)" = "active" -o "$(tail -n 1 $1)" = "unknown" ] 
> +then
> +    exec_service_on_target iptables start fi if [ "$(head -n 1 $1)" = 
> +"inactive" ] then
> +    exec_service_on_target smb stop
> +fi
> +
> +rm -fr $1
> diff --git a/tests/Functional.samba/tests/test_samba04.sh
> b/tests/Functional.samba/tests/test_samba04.sh
> index 2ad6e59..c1dac10 100644
> --- a/tests/Functional.samba/tests/test_samba04.sh
> +++ b/tests/Functional.samba/tests/test_samba04.sh
> @@ -10,14 +10,12 @@ iptables_status=$(get_service_status iptables)  
> exec_service_on_target smb stop  exec_service_on_target iptables stop
> 
> -useradd test_for_fuego
> +mkdir -p /home/test/samba
> +mkdir -p /home/test/samba_test
> 
> -mkdir -p /home/test_for_fuego/samba
> -mkdir -p /home/test_for_fuego/samba_test
> +cp data/test1 /home/test/samba/
> 
> -cp data/test1 /home/test_for_fuego/samba/
> -
> -chown nobody:nobody -R /home/test_for_fuego/samba
> +chown nobody:nobody -R /home/test/samba
> 
>  if [ -f /etc/samba/smb.conf ]
>  then
> @@ -30,8 +28,6 @@ restore_target(){
>          mv /etc/samba/smb.conf_bak /etc/samba/smb.conf
>      fi
>      mv data/smb04.conf_bak data/smb04.conf
> -    rm -fr /home/test_for_fuego
> -    userdel test_for_fuego
>      if [ "$iptables_status" = "active" -o "$iptables_status" = "unknown" ]
>      then
>          exec_service_on_target iptables start @@ -62,7 +58,7 @@ 
> expect <<-EOF  spawn smbclient //$test_target/test -N  expect {
>   -re ".*smb: .*" {
> -           send "get test1 /home/test_for_fuego/samba_test/test1\n"
> +           send "get test1 /home/test/samba_test/test1\n"
>            }
>   default { send_user "Can not log into the board.\n"}  }  expect { @@ 
> -74,7 +70,7 @@ send "exit\n"
>  expect eof
>  EOF
> 
> -if ls /home/test_for_fuego/samba_test/test1
> +if ls /home/test/samba_test/test1
>  then
>      echo " -> $test: TEST-PASS"
>  else
> diff --git a/tests/Functional.samba/tests/test_samba05.sh
> b/tests/Functional.samba/tests/test_samba05.sh
> new file mode 100644
> index 0000000..f66a92f
> --- /dev/null
> +++ b/tests/Functional.samba/tests/test_samba05.sh
> @@ -0,0 +1,103 @@
> +#!/bin/sh
> +
> +#  In the target start samba,
> +#  Set the server to access the target having password from the 
> +server, and
> make sure the server can access the target.
Please keep comments (and code where possible) to 80 columns width.

Also - just to clarify - is this test (samba05) doing all operations on the board.  The description is a bit confusing.

> +
> +test="samba05"
> +
> +. ./fuego_board_function_lib.sh
> +
> +set_init_manager
> +
> +useradd test_for_fuego
> +
> +expect <<-EOF
> +spawn smbpasswd -a test_for_fuego
> +expect "SMB password:"
> +send "goodluck\r"
> +expect "Retype new SMB password:"
> +send "goodluck\r"
> +expect "Added user root."
> +EOF
> +
> +smb_status=$(get_service_status smb)
> +iptables_status=$(get_service_status iptables)
> +
> +exec_service_on_target smb stop
> +exec_service_on_target iptables stop
> +
> +mkdir -p /home/test/samba
> +
> +cp data/test1 /home/test/samba/
> +
> +chown nobody:nobody -R /home/test/samba
> +
> +if [ -f /etc/samba/smb.conf ]
> +then
> +    mv /etc/samba/smb.conf /etc/samba/smb.conf_bak fi
> +
> +restore_target(){
> +    if [ -f /etc/samba/smb.conf_bak ]
> +    then
> +        mv /etc/samba/smb.conf_bak /etc/samba/smb.conf
> +    fi
> +    smbpasswd -x test_for_fuego
> +    rm -fr /home/test_for_fuego
> +    mv data/smb05.conf_bak data/smb05.conf
Normally, the entire fuego.Functional.samba directory is removed after the test.  I don't think it's necessary to save off the smb05.bak and restore it here.  (Unless it's used more than once per test run, but I don't see that anywhere.)

> +    userdel test_for_fuego
> +    if [ "$iptables_status" = "active" -o "$iptables_status" = "unknown" ]
> +    then
> +        exec_service_on_target iptables start
> +    fi
> +    if [ "$smb_status" = "inactive" ]
> +    then
> +        exec_service_on_target smb stop
> +    fi
> +}
> +
> +cp data/smb05.conf data/smb05.conf_bak
See above.  I don't think you need to save a copy of this file.

> +sed -i 's/test_target/'"$test_target"'/g' data/smb05.conf

Is there some reason this can't use 'localhost' or '127.0.0.1'
instead of $test_target here?

> +cp data/smb05.conf /etc/samba/smb.conf
> +
> +if exec_service_on_target smb start
> +then
> +    echo " -> start of smb succeeded."
> +else
> +    echo " -> start of smb failed."
> +    echo " -> $test: TEST-FAIL"
> +    restore_target
> +    exit
> +fi
> +
> +sleep 5
> +
> +expect <<-EOF
> +spawn sh
> +expect ".*"
> +send "smbclient //$test_target/test --user=test_for_fuego\r"

Same thing here.  Is $test_target needed, or could it be 'localhost'
or '127.0.0.1'?  (Maybe it's needed.  I'm not an smb expert.  But this could be simplified you could use a generic address for the local machine.)

> +expect {
> + -re ".*Enter.*password:.*" {
> +           send "goodluck\r"
> +          }
> + default { send_user "Can not access the board.\n"}  } expect {  -re 
> +".*Password:.*" {
> +           send "goodluck\r"
> +          }
> + default { send_user "Password is wrong.\n"}  } expect {  -re 
> +".*smb:.*" {
> +           send "ls\r"
> +          }
> + default { send_user "Can not log into the board.\n"}  } expect {  
> +-re ".*test1.*" {
> +           send_user " -> $test: TEST-PASS\n"
> +          }
> + default { send_user " -> $test: TEST-FAIL\n"}  } send "exit\n"
> +expect eof
> +EOF
> +
> +restore_target
> diff --git a/tests/Functional.samba/tests/test_samba06.sh
> b/tests/Functional.samba/tests/test_samba06.sh
> new file mode 100644
> index 0000000..29c1dd1
> --- /dev/null
> +++ b/tests/Functional.samba/tests/test_samba06.sh
> @@ -0,0 +1,58 @@
> +#!/bin/sh
> +
> +#  In the target start samba, and confirm the process condition by 
> +command
> ps.
> +#  check the keyword "smbd".
> +
> +
> +. ./fuego_board_function_lib.sh
> +set_init_manager
> +
> +echo $(get_service_status smb) > $3
> +echo $(get_service_status iptables) >> $3
> +
> +exec_service_on_target smb stop
> +exec_service_on_target iptables stop
> +
> +mkdir -p /home/test/samba
> +
> +cp data/test1 /home/test/samba/
> +
> +chown nobody:nobody -R /home/test/samba
> +
> +if [ -f /etc/samba/smb.conf ]
> +then
> +    mv /etc/samba/smb.conf /etc/samba/smb.conf_bak fi
> +
> +restore_target(){
> +    if [ -f /etc/samba/smb.conf_bak ]
> +    then
> +        mv /etc/samba/smb.conf_bak /etc/samba/smb.conf
> +    fi
> +    mv data/smb06.conf_bak data/smb06.conf
I don't think smb06.conf needs to be backed up or restored.

> +    if [ "$iptables_status" = "active" -o "$iptables_status" = "unknown" ]
> +    then
> +        exec_service_on_target iptables start
> +    fi
> +    if [ "$smb_status" = "inactive" ]
> +    then
> +        exec_service_on_target smb stop
> +    fi
> +}
> +
> +
> +cp data/smb06.conf data/smb06.conf_bak
I don't think smb06.conf needs to be backed up or restored.
This could be remove, IMHO.

Or are you trying to make it so this test can be run manually multiple times (outside of Fuego's CI context?)

> +sed -i 's/test_target/'"$2"'/g' data/smb06.conf sed -i 
> +'s/test_host/'"$1"'/g' data/smb06.conf cp data/smb06.conf 
> +/etc/samba/smb.conf
> +
> +if exec_service_on_target smb start
> +then
> +    echo " -> start of smb succeeded."
> +else
> +    echo " -> start of smb failed."
> +    echo " -> $test: TEST-FAIL"
> +    exit
> +fi
> +
> +sleep 5
> diff --git a/tests/Functional.samba/tests/test_samba06_host.sh
> b/tests/Functional.samba/tests/test_samba06_host.sh
> new file mode 100644
> index 0000000..77ff7a5
> --- /dev/null
> +++ b/tests/Functional.samba/tests/test_samba06_host.sh
> @@ -0,0 +1,23 @@
> +#!/bin/sh
> +
> +#  In the target start samba, and confirm the process condition by 
> +command
> ps.
> +#  check the keyword "smbd".
This description doesn't seem right.  isn't this a test on the host against the server running on the target?

Please update this comment, or explain why it's accurate.
> +
> +test="samba06"
> +expect <<-EOF
> +spawn sh
> +expect ".*"
> +send "smbclient //$1/test -N\r"
> +expect {
> + -re ".*smb:.*" {
> +           send "ls\r"
> +          }
> + default { send_user "Can't connect to the board"\n"}  } expect {  
> +-re ".*test1.*" {
> +           send_user " -> $test: TEST-PASS\n"
> +          }
> + default { send_user " -> $test: TEST-FAIL\n"}  } send "exit\n"
> +expect eof
> +EOF
> --
> 2.17.1

This is pretty nice.  It's very nice to see a test that uses Fuego's inherent host/target capabilities to perform a test.  And it's nice to see a test of actual samba functionality.

Please fix the items mentioned, and/or respond to comments and resubmit.

Thanks,
 -- Tim