Re: [PATCH] Add system test before "set remote system-call-allowed 1"

[Pedro Alves <palves at redhat dot com>]

On 05/13/2014 04:27 AM, Hui Zhu wrote:
> This patch is update version according to the discussion in
> https://www.sourceware.org/ml/gdb-patches/2009-11/msg00090.html.
> If test get the target doesn't support fileio system according to the
> remote log.   It will set this test as "unsupported".
> 
> Before I made this patch, I want add a check before all of tests in this
> file.  But I found that the target maybe support one call but not others.
> For example: my target support Fwrite, Fopen and so on.  But not 
> Fgettimeofday.
> And it doesn't support Fsystem NULL but it support Fsystem not NULL.

So IIUC, the test will still have system (NULL) FAIL on your
target, right?

> So I think if we want to check target support fileio, we need check them
> one by one.

Against native, we'll get a new UNSUPPORTED, right?

> @@ -375,21 +379,26 @@ test_system ()
>      */
>     int ret;
> 
> -  /* Test for shell */
> +  /* Test for shell (testsuite should have it disabled).  */

This comment confused me a bit, as I didn't recall that
this is disabled by default.  So, I'd prefer:

  /* Test for shell ('set remote system-call-allowed' is disabled
     by default).  */

>     ret = system (NULL);
> -  printf ("system 1: ret = %d %s\n", ret, ret != 0 ? "OK" : "");
> +  printf ("system 1: ret = %d %s\n", ret, ret == 0 ? "OK" : "");
> +  stop ();
> +  /* Test for shell again (the testsuite will have enabled it now).  */
> +  ret = system (NULL);
> +  printf ("system 2: ret = %d %s\n", ret, ret != 0 ? "OK" : "");
>     stop ();
>     /* This test prepares the directory for test_rename() */
>     sprintf (sys, "mkdir -p %s/%s %s/%s", OUTDIR, TESTSUBDIR, OUTDIR, 
> TESTDIR2);
>     ret = system (sys);
>     if (ret == 127)
> -    printf ("system 2: ret = %d /bin/sh unavailable???\n", ret);
> +    printf ("system 3: ret = %d /bin/sh unavailable???\n", ret);
>     else
> -    printf ("system 2: ret = %d %s\n", ret, ret == 0 ? "OK" : "");
> +    printf ("system 3: ret = %d %s\n", ret, ret == 0 ? "OK" : "");
>     stop ();
>     /* Invalid command (just guessing ;-) ) */
>     ret = system ("wrtzlpfrmpft");
> -  printf ("system 3: ret = %d %s\n", ret, WEXITSTATUS (ret) == 127 ? 
> "OK" : "");
> +  printf ("system 4: ret = %d %s\n", ret,
> +	  WEXITSTATUS (ret) == 127 ? "OK" : "");
>     stop ();
>   }
> 
> --- a/gdb/testsuite/gdb.base/fileio.exp
> +++ b/gdb/testsuite/gdb.base/fileio.exp
> @@ -180,19 +180,34 @@ gdb_test continue \
>   "Continuing\\..*isatty 5:.*OK$stop_msg" \
>   "Isatty (open file)"
> 
> -gdb_test continue \
> -"Continuing\\..*system 1:.*OK$stop_msg" \
> -"System says shell is available"
> +gdb_test_no_output "set debug remote 1"
> +set msg "System says shell is not available"
> +gdb_test_multiple "continue" $msg {
> +    -re "Continuing\\..*Fsystem.*system 1:.*OK$stop_msg\r\n$gdb_prompt $" {
> +	pass $msg
> +    }
> +    -re ".*Fsystem.*$gdb_prompt $" {
> +	fail $msg
> +    }
> +    -re ".*$gdb_prompt $" {
> +	unsupported $msg
> +    }

No need for leading '.*' :

    -re "Fsystem.*$gdb_prompt $" {
	fail $msg
    }
    -re "$gdb_prompt $" {
	unsupported $msg
    }

OK with those changes.

-- 
Pedro Alves