Re: [Qemu-devel] [RISU PATCH v4 07/10] risu: add simple trace and replay support

[Alex Bennée]

Peter Maydell <address@hidden> writes:

> On 2 June 2017 at 17:08, Alex Bennée <address@hidden> wrote:
>> This adds a very dumb and easily breakable trace and replay support. In
>> --master mode the various risu ops trigger a write of register/memory
>> state into a binary file which can be played back to an apprentice.
>> Currently there is no validation of the image source so feeding the
>> wrong image will fail straight away.
>>
>> The trace files will get very big for any appreciable sized test file
>> and this will be addressed in later patches.
>>
>> Signed-off-by: Alex Bennée <address@hidden>
>>
>> ---
>> v4
>>   - fix formatting mess
>>   - abort() instead of reporting de-sync
>>   - don't fake return for compiler
>> v3
>>   - fix options parsing
>>   - re-factored so no need for copy & paste
>> v2
>>   - moved read/write functions into main risu.c
>>   - cleaned up formatting
>>   - report more in apprentice --trace mode
>> ---
>>  risu.c | 97 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 84 insertions(+), 13 deletions(-)
>>
>> diff --git a/risu.c b/risu.c
>> index 22571cd..137cbbf 100644
>> --- a/risu.c
>> +++ b/risu.c
>> @@ -31,6 +31,7 @@
>>  void *memblock = 0;
>>
>>  int apprentice_socket, master_socket;
>> +int trace_file = 0;
>>
>>  sigjmp_buf jmpbuf;
>>
>> @@ -40,10 +41,12 @@ int test_fp_exc = 0;
>>  long executed_tests = 0;
>>  void report_test_status(void *pc)
>>  {
>> -   executed_tests += 1;
>> -   if (executed_tests % 100 == 0) {
>> -      fprintf(stderr,"Executed %ld test instructions (pc=%p)\r",
>> -              executed_tests, pc);
>> +   if (ismaster || trace_file) {
>> +      executed_tests += 1;
>> +      if (executed_tests % 100 == 0) {
>> +         fprintf(stderr,"Executed %ld test instructions (pc=%p)\r",
>> +                 executed_tests, pc);
>> +      }
>>     }
>>  }
>>
>> @@ -54,6 +57,12 @@ int read_sock(void *ptr, size_t bytes)
>>     return recv_data_pkt(master_socket, ptr, bytes);
>>  }
>>
>> +int write_trace(void *ptr, size_t bytes)
>> +{
>> +   size_t res = write(trace_file, ptr, bytes);
>> +   return (res == bytes) ? 0 : 1;
>> +}
>> +
>>  void respond_sock(int r)
>>  {
>>     send_response_byte(master_socket, r);
>> @@ -66,9 +75,36 @@ int write_sock(void *ptr, size_t bytes)
>>     return send_data_pkt(apprentice_socket, ptr, bytes);
>>  }
>>
>> +int read_trace(void *ptr, size_t bytes)
>> +{
>> +   size_t res = read(trace_file, ptr, bytes);
>> +   return (res == bytes) ? 0 : 1;
>> +}
>> +
>> +void respond_trace(int r)
>> +{
>> +   switch (r) {
>> +      case 0: /* test ok */
>> +      case 1: /* end of test */
>> +         break;
>> +      default:
>> +         /* should not get here */
>> +         abort();
>> +         break;
>> +   }
>> +}
>> +
>>  void master_sigill(int sig, siginfo_t *si, void *uc)
>>  {
>> -   switch (recv_and_compare_register_info(read_sock, respond_sock, uc))
>> +   int r;
>> +
>> +   if (trace_file) {
>> +      r = send_register_info(write_trace, uc);
>> +   } else {
>> +      r = recv_and_compare_register_info(read_sock, respond_sock, uc);
>> +   }
>
> It's a bit weird that in the trace file case we send the data
> from the master, but in the normal case we send it from
> the apprentice end.

It seemed the natural way round (master generating the "gold" stream
that the apprentice checks). I could switch it or make the apprentice
responsible for checking it's own work?

>
>> +
>> +   switch (r)
>>     {
>>        case 0:
>>           /* match OK */
>> @@ -82,7 +118,15 @@ void master_sigill(int sig, siginfo_t *si, void *uc)
>>
>>  void apprentice_sigill(int sig, siginfo_t *si, void *uc)
>>  {
>> -   switch (send_register_info(write_sock, uc))
>> +   int r;
>> +
>> +   if (trace_file) {
>> +      r = recv_and_compare_register_info(read_trace, respond_trace, uc);
>> +   } else {
>> +      r = send_register_info(write_sock, uc);
>> +   }
>> +
>> +   switch (r)
>>     {
>>        case 0:
>>           /* match OK */
>> @@ -94,6 +138,9 @@ void apprentice_sigill(int sig, siginfo_t *si, void *uc)
>>           exit(0);
>>        default:
>>           /* mismatch */
>> +         if (trace_file) {
>> +            report_match_status();
>> +         }
>
> report_match_status() uses stdio, you can't call it from a signal
> handler.

Oops, I'll sigjmp.

>
>>           exit(1);
>>     }
>>  }
>> @@ -155,7 +202,13 @@ int master(int sock)
>>  {
>>     if (sigsetjmp(jmpbuf, 1))
>>     {
>> -      return report_match_status();
>> +      if (trace_file) {
>> +         close(trace_file);
>> +         fprintf(stderr,"\nDone...\n");
>> +         return 0;
>> +      } else {
>> +         return report_match_status();
>> +      }
>>     }
>>     master_socket = sock;
>>     set_sigill_handler(&master_sigill);
>> @@ -184,6 +237,7 @@ void usage (void)
>>     fprintf(stderr, "between master and apprentice risu processes.\n\n");
>>     fprintf(stderr, "Options:\n");
>>     fprintf(stderr, "  --master          Be the master (server)\n");
>> +   fprintf(stderr, "  -t, --trace=FILE  Record/playback trace file\n");
>>     fprintf(stderr, "  -h, --host=HOST   Specify master host machine 
>> (apprentice only)\n");
>>     fprintf(stderr, "  -p, --port=PORT   Specify the port to connect 
>> to/listen on (default 9191)\n");
>>  }
>> @@ -194,6 +248,7 @@ int main(int argc, char **argv)
>>     uint16_t port = 9191;
>>     char *hostname = "localhost";
>>     char *imgfile;
>> +   char *trace_fn = NULL;
>>     int sock;
>>
>>     // TODO clean this up later
>> @@ -204,13 +259,14 @@ int main(int argc, char **argv)
>>           {
>>              { "help", no_argument, 0, '?'},
>>              { "master", no_argument, &ismaster, 1 },
>> +            { "trace", required_argument, 0, 't' },
>>              { "host", required_argument, 0, 'h' },
>>              { "port", required_argument, 0, 'p' },
>>              { "test-fp-exc", no_argument, &test_fp_exc, 1 },
>>              { 0,0,0,0 }
>>           };
>>        int optidx = 0;
>> -      int c = getopt_long(argc, argv, "h:p:", longopts, &optidx);
>> +      int c = getopt_long(argc, argv, "h:p:t:", longopts, &optidx);
>>        if (c == -1)
>>        {
>>           break;
>> @@ -223,6 +279,11 @@ int main(int argc, char **argv)
>>              /* flag set by getopt_long, do nothing */
>>              break;
>>           }
>> +         case 't':
>> +         {
>> +           trace_fn = optarg;
>> +           break;
>> +         }
>>           case 'h':
>>           {
>>              hostname = optarg;
>> @@ -253,17 +314,27 @@ int main(int argc, char **argv)
>>     }
>>
>>     load_image(imgfile);
>> -
>> +
>>     if (ismaster)
>>     {
>> -      fprintf(stderr, "master port %d\n", port);
>> -      sock = master_connect(port);
>> +      if (trace_fn)
>> +      {
>> +         trace_file = open(trace_fn, O_WRONLY|O_CREAT, S_IRWXU);
>> +      } else {
>> +         fprintf(stderr, "master port %d\n", port);
>> +         sock = master_connect(port);
>> +      }
>>        return master(sock);
>
> Doesn't this call master() with an uninitialized sock if
> the trace file is in use ?

Hmm yes I can clean that up.

>
>>     }
>>     else
>>     {
>> -      fprintf(stderr, "apprentice host %s port %d\n", hostname, port);
>> -      sock = apprentice_connect(hostname, port);
>> +      if (trace_fn)
>> +      {
>> +         trace_file = open(trace_fn, O_RDONLY);
>> +      } else {
>> +         fprintf(stderr, "apprentice host %s port %d\n", hostname, port);
>> +         sock = apprentice_connect(hostname, port);
>> +      }
>>        return apprentice(sock);
>>     }
>>  }
>> --
>> 2.13.0
>>
>
> thanks
> -- PMM


--
Alex Bennée