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

[Hui Zhu <hui_zhu at mentor dot com>]

On 05/29/14 19:12, Pedro Alves wrote:
> 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?

It will use fileio if argument of Fsystem is not NULL.
But will not use fileio if its argument is NULL.

> > 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?

With current patch, we will get a new UNSUPPORTED because it just check 
if target support fileio system(NULL) without "set remote 
system-call-allowed 1".


> > @@ -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).  */

Updated.

> >      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.

Committed as 
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=9f5a4cef68413e211bc765e969bf6778150231db

Thanks,
Hui