[RISU,v5,07/13] risu: paramterise send/receive functions

Message ID 20170619104655.31104-8-alex.bennee@linaro.org
State Superseded
Headers show
Series
  • RISU record/replay patches
Related show

Commit Message

Alex Bennée June 19, 2017, 10:46 a.m.
This is a precursor to record/playback support. Instead of passing the
socket fd we now pass helper functions for reading/writing and
responding. This will allow us to do the rest of the record/playback
cleanly outside of the main worker function.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>


---
v5
  - re-base without tab/format cleanps
v4
  - split header code
  - fix formatting foo-bar's
v3
  - new for v3
  - arm, aarch64, ppc64
---
 reginfo.c | 39 +++++++++++++++++++--------------------
 risu.c    | 23 +++++++++++++++++++++--
 risu.h    | 11 +++++++++--
 3 files changed, 49 insertions(+), 24 deletions(-)

-- 
2.13.0

Comments

Peter Maydell June 20, 2017, 1:40 p.m. | #1
On 19 June 2017 at 11:46, Alex Bennée <alex.bennee@linaro.org> wrote:
> This is a precursor to record/playback support. Instead of passing the

> socket fd we now pass helper functions for reading/writing and

> responding. This will allow us to do the rest of the record/playback

> cleanly outside of the main worker function.

>

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>

> ---

> v5

>   - re-base without tab/format cleanps

> v4

>   - split header code

>   - fix formatting foo-bar's

> v3

>   - new for v3

>   - arm, aarch64, ppc64

> ---

>  reginfo.c | 39 +++++++++++++++++++--------------------

>  risu.c    | 23 +++++++++++++++++++++--

>  risu.h    | 11 +++++++++--

>  3 files changed, 49 insertions(+), 24 deletions(-)

>

> diff --git a/reginfo.c b/reginfo.c

> index 31bb99f..90cea8f 100644

> --- a/reginfo.c

> +++ b/reginfo.c

> @@ -21,7 +21,7 @@ uint8_t apprentice_memblock[MEMBLOCKLEN];

>  static int mem_used;

>  static int packet_mismatch;

>

> -int send_register_info(int sock, void *uc)

> +int send_register_info(write_fn write_fn, void *uc)

>  {

>      struct reginfo ri;

>      int op;

> @@ -29,24 +29,25 @@ int send_register_info(int sock, void *uc)

>      op = get_risuop(&ri);

>

>      switch (op) {

> -    case OP_COMPARE:

>      case OP_TESTEND:

> -    default:

> -        /* Do a simple register compare on (a) explicit request

> -         * (b) end of test (c) a non-risuop UNDEF

> -         */

> -        return send_data_pkt(sock, &ri, sizeof(ri));

> +        write_fn(&ri, sizeof(ri));

> +        return 1;


Why the change from "return write function's return value"
to "ignore return value and always return 1" for OP_TESTEND ?

>      case OP_COMPAREMEM:

> -        return send_data_pkt(sock, memblock, MEMBLOCKLEN);

> +        return write_fn(memblock, MEMBLOCKLEN);

>          break;


...here we still just pass the return value through, for instance.

> +    case OP_COMPARE:

> +    default:

> +        /* Do a simple register compare on (a) explicit request

> +         * (b) end of test (c) a non-risuop UNDEF

> +         */

> +        return write_fn(&ri, sizeof(ri));

>      }

>      return 0;

>  }

> @@ -59,7 +60,7 @@ int send_register_info(int sock, void *uc)

>   * that says whether it is register or memory data, so if the two

>   * sides get out of sync then we will fail obscurely.

>   */

> -int recv_and_compare_register_info(int sock, void *uc)

> +int recv_and_compare_register_info(read_fn read_fn, respond_fn resp_fn, void *uc)

>  {

>      int resp = 0, op;

>

> @@ -73,36 +74,34 @@ int recv_and_compare_register_info(int sock, void *uc)

>          /* Do a simple register compare on (a) explicit request

>           * (b) end of test (c) a non-risuop UNDEF

>           */

> -        if (recv_data_pkt(sock, &apprentice_ri, sizeof(apprentice_ri))) {

> +        if (read_fn(&apprentice_ri, sizeof(apprentice_ri))) {

>              packet_mismatch = 1;

>              resp = 2;

> -

>          } else if (!reginfo_is_eq(&master_ri, &apprentice_ri)) {

>              /* register mismatch */

>              resp = 2;

> -

>          } else if (op == OP_TESTEND) {

>              resp = 1;

>          }

> -        send_response_byte(sock, resp);

> +        resp_fn(resp);

>          break;

>      case OP_SETMEMBLOCK:

> -        memblock = (void *) (uintptr_t) get_reginfo_paramreg(&master_ri);

> +        memblock = (void *)(uintptr_t)get_reginfo_paramreg(&master_ri);

>          break;

>      case OP_GETMEMBLOCK:

>          set_ucontext_paramreg(uc, get_reginfo_paramreg(&master_ri) +

> -                              (uintptr_t) memblock);

> +                              (uintptr_t)memblock);

>          break;

>      case OP_COMPAREMEM:

>          mem_used = 1;

> -        if (recv_data_pkt(sock, apprentice_memblock, MEMBLOCKLEN)) {

> +        if (read_fn(apprentice_memblock, MEMBLOCKLEN)) {

>              packet_mismatch = 1;

>              resp = 2;

>          } else if (memcmp(memblock, apprentice_memblock, MEMBLOCKLEN) != 0) {

>              /* memory mismatch */

>              resp = 2;

>          }

> -        send_response_byte(sock, resp);

> +        resp_fn(resp);

>          break;

>      }

>

> diff --git a/risu.c b/risu.c

> index a10422a..88e586c 100644

> --- a/risu.c

> +++ b/risu.c

> @@ -37,9 +37,28 @@ sigjmp_buf jmpbuf;

>  /* Should we test for FP exception status bits? */

>  int test_fp_exc;

>

> +/* Master functions */

> +

> +int read_sock(void *ptr, size_t bytes)

> +{

> +    return recv_data_pkt(master_socket, ptr, bytes);

> +}

> +

> +void respond_sock(int r)

> +{

> +    send_response_byte(master_socket, r);

> +}

> +

> +/* Apprentice function */

> +

> +int write_sock(void *ptr, size_t bytes)

> +{

> +    return send_data_pkt(apprentice_socket, ptr, bytes);

> +}

> +

>  void master_sigill(int sig, siginfo_t *si, void *uc)

>  {

> -    switch (recv_and_compare_register_info(master_socket, uc)) {

> +    switch (recv_and_compare_register_info(read_sock, respond_sock, uc)) {

>      case 0:

>          /* match OK */

>          advance_pc(uc);

> @@ -52,7 +71,7 @@ void master_sigill(int sig, siginfo_t *si, void *uc)

>

>  void apprentice_sigill(int sig, siginfo_t *si, void *uc)

>  {

> -    switch (send_register_info(apprentice_socket, uc)) {

> +    switch (send_register_info(write_sock, uc)) {

>      case 0:

>          /* match OK */

>          advance_pc(uc);

> diff --git a/risu.h b/risu.h

> index 78a7313..32241bc 100644

> --- a/risu.h

> +++ b/risu.h

> @@ -53,17 +53,24 @@ struct reginfo;

>

>  /* Functions operating on reginfo */

>

> +/* To keep the read/write logic from multiplying across all arches

> + * we wrap up the function here to keep all the changes in one place

> + */


Stale comment? The calling code is not per-arch any more.

> +typedef int (*write_fn) (void *ptr, size_t bytes);

> +typedef int (*read_fn) (void *ptr, size_t bytes);

> +typedef void (*respond_fn) (int response);

> +

>  /* Send the register information from the struct ucontext down the socket.

>   * Return the response code from the master.

>   * NB: called from a signal handler.

>   */

> -int send_register_info(int sock, void *uc);

> +int send_register_info(write_fn write_fn, void *uc);

>

>  /* Read register info from the socket and compare it with that from the

>   * ucontext. Return 0 for match, 1 for end-of-test, 2 for mismatch.

>   * NB: called from a signal handler.

>   */

> -int recv_and_compare_register_info(int sock, void *uc);

> +int recv_and_compare_register_info(read_fn read_fn, respond_fn respond, void *uc);

>

>  /* Print a useful report on the status of the last comparison

>   * done in recv_and_compare_register_info(). This is called on

> --

> 2.13.0

>


thanks
-- PMM
Alex Bennée June 20, 2017, 1:54 p.m. | #2
Peter Maydell <peter.maydell@linaro.org> writes:

> On 19 June 2017 at 11:46, Alex Bennée <alex.bennee@linaro.org> wrote:

>> This is a precursor to record/playback support. Instead of passing the

>> socket fd we now pass helper functions for reading/writing and

>> responding. This will allow us to do the rest of the record/playback

>> cleanly outside of the main worker function.

>>

>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>>

>> ---

>> v5

>>   - re-base without tab/format cleanps

>> v4

>>   - split header code

>>   - fix formatting foo-bar's

>> v3

>>   - new for v3

>>   - arm, aarch64, ppc64

>> ---

>>  reginfo.c | 39 +++++++++++++++++++--------------------

>>  risu.c    | 23 +++++++++++++++++++++--

>>  risu.h    | 11 +++++++++--

>>  3 files changed, 49 insertions(+), 24 deletions(-)

>>

>> diff --git a/reginfo.c b/reginfo.c

>> index 31bb99f..90cea8f 100644

>> --- a/reginfo.c

>> +++ b/reginfo.c

>> @@ -21,7 +21,7 @@ uint8_t apprentice_memblock[MEMBLOCKLEN];

>>  static int mem_used;

>>  static int packet_mismatch;

>>

>> -int send_register_info(int sock, void *uc)

>> +int send_register_info(write_fn write_fn, void *uc)

>>  {

>>      struct reginfo ri;

>>      int op;

>> @@ -29,24 +29,25 @@ int send_register_info(int sock, void *uc)

>>      op = get_risuop(&ri);

>>

>>      switch (op) {

>> -    case OP_COMPARE:

>>      case OP_TESTEND:

>> -    default:

>> -        /* Do a simple register compare on (a) explicit request

>> -         * (b) end of test (c) a non-risuop UNDEF

>> -         */

>> -        return send_data_pkt(sock, &ri, sizeof(ri));

>> +        write_fn(&ri, sizeof(ri));

>> +        return 1;

>

> Why the change from "return write function's return value"

> to "ignore return value and always return 1" for OP_TESTEND ?


Hmm possibly a re-base snaggle. I'll fix it up.

>

>>      case OP_COMPAREMEM:

>> -        return send_data_pkt(sock, memblock, MEMBLOCKLEN);

>> +        return write_fn(memblock, MEMBLOCKLEN);

>>          break;

>

> ...here we still just pass the return value through, for instance.

>

>> +    case OP_COMPARE:

>> +    default:

>> +        /* Do a simple register compare on (a) explicit request

>> +         * (b) end of test (c) a non-risuop UNDEF

>> +         */

>> +        return write_fn(&ri, sizeof(ri));

>>      }

>>      return 0;

>>  }

>> @@ -59,7 +60,7 @@ int send_register_info(int sock, void *uc)

>>   * that says whether it is register or memory data, so if the two

>>   * sides get out of sync then we will fail obscurely.

>>   */

>> -int recv_and_compare_register_info(int sock, void *uc)

>> +int recv_and_compare_register_info(read_fn read_fn, respond_fn resp_fn, void *uc)

>>  {

>>      int resp = 0, op;

>>

>> @@ -73,36 +74,34 @@ int recv_and_compare_register_info(int sock, void *uc)

>>          /* Do a simple register compare on (a) explicit request

>>           * (b) end of test (c) a non-risuop UNDEF

>>           */

>> -        if (recv_data_pkt(sock, &apprentice_ri, sizeof(apprentice_ri))) {

>> +        if (read_fn(&apprentice_ri, sizeof(apprentice_ri))) {

>>              packet_mismatch = 1;

>>              resp = 2;

>> -

>>          } else if (!reginfo_is_eq(&master_ri, &apprentice_ri)) {

>>              /* register mismatch */

>>              resp = 2;

>> -

>>          } else if (op == OP_TESTEND) {

>>              resp = 1;

>>          }

>> -        send_response_byte(sock, resp);

>> +        resp_fn(resp);

>>          break;

>>      case OP_SETMEMBLOCK:

>> -        memblock = (void *) (uintptr_t) get_reginfo_paramreg(&master_ri);

>> +        memblock = (void *)(uintptr_t)get_reginfo_paramreg(&master_ri);

>>          break;

>>      case OP_GETMEMBLOCK:

>>          set_ucontext_paramreg(uc, get_reginfo_paramreg(&master_ri) +

>> -                              (uintptr_t) memblock);

>> +                              (uintptr_t)memblock);

>>          break;

>>      case OP_COMPAREMEM:

>>          mem_used = 1;

>> -        if (recv_data_pkt(sock, apprentice_memblock, MEMBLOCKLEN)) {

>> +        if (read_fn(apprentice_memblock, MEMBLOCKLEN)) {

>>              packet_mismatch = 1;

>>              resp = 2;

>>          } else if (memcmp(memblock, apprentice_memblock, MEMBLOCKLEN) != 0) {

>>              /* memory mismatch */

>>              resp = 2;

>>          }

>> -        send_response_byte(sock, resp);

>> +        resp_fn(resp);

>>          break;

>>      }

>>

>> diff --git a/risu.c b/risu.c

>> index a10422a..88e586c 100644

>> --- a/risu.c

>> +++ b/risu.c

>> @@ -37,9 +37,28 @@ sigjmp_buf jmpbuf;

>>  /* Should we test for FP exception status bits? */

>>  int test_fp_exc;

>>

>> +/* Master functions */

>> +

>> +int read_sock(void *ptr, size_t bytes)

>> +{

>> +    return recv_data_pkt(master_socket, ptr, bytes);

>> +}

>> +

>> +void respond_sock(int r)

>> +{

>> +    send_response_byte(master_socket, r);

>> +}

>> +

>> +/* Apprentice function */

>> +

>> +int write_sock(void *ptr, size_t bytes)

>> +{

>> +    return send_data_pkt(apprentice_socket, ptr, bytes);

>> +}

>> +

>>  void master_sigill(int sig, siginfo_t *si, void *uc)

>>  {

>> -    switch (recv_and_compare_register_info(master_socket, uc)) {

>> +    switch (recv_and_compare_register_info(read_sock, respond_sock, uc)) {

>>      case 0:

>>          /* match OK */

>>          advance_pc(uc);

>> @@ -52,7 +71,7 @@ void master_sigill(int sig, siginfo_t *si, void *uc)

>>

>>  void apprentice_sigill(int sig, siginfo_t *si, void *uc)

>>  {

>> -    switch (send_register_info(apprentice_socket, uc)) {

>> +    switch (send_register_info(write_sock, uc)) {

>>      case 0:

>>          /* match OK */

>>          advance_pc(uc);

>> diff --git a/risu.h b/risu.h

>> index 78a7313..32241bc 100644

>> --- a/risu.h

>> +++ b/risu.h

>> @@ -53,17 +53,24 @@ struct reginfo;

>>

>>  /* Functions operating on reginfo */

>>

>> +/* To keep the read/write logic from multiplying across all arches

>> + * we wrap up the function here to keep all the changes in one place

>> + */

>

> Stale comment? The calling code is not per-arch any more.


Yes, I'll remove

>

>> +typedef int (*write_fn) (void *ptr, size_t bytes);

>> +typedef int (*read_fn) (void *ptr, size_t bytes);

>> +typedef void (*respond_fn) (int response);

>> +

>>  /* Send the register information from the struct ucontext down the socket.

>>   * Return the response code from the master.

>>   * NB: called from a signal handler.

>>   */

>> -int send_register_info(int sock, void *uc);

>> +int send_register_info(write_fn write_fn, void *uc);

>>

>>  /* Read register info from the socket and compare it with that from the

>>   * ucontext. Return 0 for match, 1 for end-of-test, 2 for mismatch.

>>   * NB: called from a signal handler.

>>   */

>> -int recv_and_compare_register_info(int sock, void *uc);

>> +int recv_and_compare_register_info(read_fn read_fn, respond_fn respond, void *uc);

>>

>>  /* Print a useful report on the status of the last comparison

>>   * done in recv_and_compare_register_info(). This is called on

>> --

>> 2.13.0

>>

>

> thanks

> -- PMM



--
Alex Bennée

Patch hide | download patch | download mbox

diff --git a/reginfo.c b/reginfo.c
index 31bb99f..90cea8f 100644
--- a/reginfo.c
+++ b/reginfo.c
@@ -21,7 +21,7 @@  uint8_t apprentice_memblock[MEMBLOCKLEN];
 static int mem_used;
 static int packet_mismatch;
 
-int send_register_info(int sock, void *uc)
+int send_register_info(write_fn write_fn, void *uc)
 {
     struct reginfo ri;
     int op;
@@ -29,24 +29,25 @@  int send_register_info(int sock, void *uc)
     op = get_risuop(&ri);
 
     switch (op) {
-    case OP_COMPARE:
     case OP_TESTEND:
-    default:
-        /* Do a simple register compare on (a) explicit request
-         * (b) end of test (c) a non-risuop UNDEF
-         */
-        return send_data_pkt(sock, &ri, sizeof(ri));
+        write_fn(&ri, sizeof(ri));
+        return 1;
     case OP_SETMEMBLOCK:
-        memblock = (void *) (uintptr_t) get_reginfo_paramreg(&ri);
+        memblock = (void *)(uintptr_t)get_reginfo_paramreg(&ri);
         break;
     case OP_GETMEMBLOCK:
         set_ucontext_paramreg(uc,
-                              get_reginfo_paramreg(&ri) +
-                              (uintptr_t) memblock);
+                              get_reginfo_paramreg(&ri) + (uintptr_t)memblock);
         break;
     case OP_COMPAREMEM:
-        return send_data_pkt(sock, memblock, MEMBLOCKLEN);
+        return write_fn(memblock, MEMBLOCKLEN);
         break;
+    case OP_COMPARE:
+    default:
+        /* Do a simple register compare on (a) explicit request
+         * (b) end of test (c) a non-risuop UNDEF
+         */
+        return write_fn(&ri, sizeof(ri));
     }
     return 0;
 }
@@ -59,7 +60,7 @@  int send_register_info(int sock, void *uc)
  * that says whether it is register or memory data, so if the two
  * sides get out of sync then we will fail obscurely.
  */
-int recv_and_compare_register_info(int sock, void *uc)
+int recv_and_compare_register_info(read_fn read_fn, respond_fn resp_fn, void *uc)
 {
     int resp = 0, op;
 
@@ -73,36 +74,34 @@  int recv_and_compare_register_info(int sock, void *uc)
         /* Do a simple register compare on (a) explicit request
          * (b) end of test (c) a non-risuop UNDEF
          */
-        if (recv_data_pkt(sock, &apprentice_ri, sizeof(apprentice_ri))) {
+        if (read_fn(&apprentice_ri, sizeof(apprentice_ri))) {
             packet_mismatch = 1;
             resp = 2;
-
         } else if (!reginfo_is_eq(&master_ri, &apprentice_ri)) {
             /* register mismatch */
             resp = 2;
-
         } else if (op == OP_TESTEND) {
             resp = 1;
         }
-        send_response_byte(sock, resp);
+        resp_fn(resp);
         break;
     case OP_SETMEMBLOCK:
-        memblock = (void *) (uintptr_t) get_reginfo_paramreg(&master_ri);
+        memblock = (void *)(uintptr_t)get_reginfo_paramreg(&master_ri);
         break;
     case OP_GETMEMBLOCK:
         set_ucontext_paramreg(uc, get_reginfo_paramreg(&master_ri) +
-                              (uintptr_t) memblock);
+                              (uintptr_t)memblock);
         break;
     case OP_COMPAREMEM:
         mem_used = 1;
-        if (recv_data_pkt(sock, apprentice_memblock, MEMBLOCKLEN)) {
+        if (read_fn(apprentice_memblock, MEMBLOCKLEN)) {
             packet_mismatch = 1;
             resp = 2;
         } else if (memcmp(memblock, apprentice_memblock, MEMBLOCKLEN) != 0) {
             /* memory mismatch */
             resp = 2;
         }
-        send_response_byte(sock, resp);
+        resp_fn(resp);
         break;
     }
 
diff --git a/risu.c b/risu.c
index a10422a..88e586c 100644
--- a/risu.c
+++ b/risu.c
@@ -37,9 +37,28 @@  sigjmp_buf jmpbuf;
 /* Should we test for FP exception status bits? */
 int test_fp_exc;
 
+/* Master functions */
+
+int read_sock(void *ptr, size_t bytes)
+{
+    return recv_data_pkt(master_socket, ptr, bytes);
+}
+
+void respond_sock(int r)
+{
+    send_response_byte(master_socket, r);
+}
+
+/* Apprentice function */
+
+int write_sock(void *ptr, size_t bytes)
+{
+    return send_data_pkt(apprentice_socket, ptr, bytes);
+}
+
 void master_sigill(int sig, siginfo_t *si, void *uc)
 {
-    switch (recv_and_compare_register_info(master_socket, uc)) {
+    switch (recv_and_compare_register_info(read_sock, respond_sock, uc)) {
     case 0:
         /* match OK */
         advance_pc(uc);
@@ -52,7 +71,7 @@  void master_sigill(int sig, siginfo_t *si, void *uc)
 
 void apprentice_sigill(int sig, siginfo_t *si, void *uc)
 {
-    switch (send_register_info(apprentice_socket, uc)) {
+    switch (send_register_info(write_sock, uc)) {
     case 0:
         /* match OK */
         advance_pc(uc);
diff --git a/risu.h b/risu.h
index 78a7313..32241bc 100644
--- a/risu.h
+++ b/risu.h
@@ -53,17 +53,24 @@  struct reginfo;
 
 /* Functions operating on reginfo */
 
+/* To keep the read/write logic from multiplying across all arches
+ * we wrap up the function here to keep all the changes in one place
+ */
+typedef int (*write_fn) (void *ptr, size_t bytes);
+typedef int (*read_fn) (void *ptr, size_t bytes);
+typedef void (*respond_fn) (int response);
+
 /* Send the register information from the struct ucontext down the socket.
  * Return the response code from the master.
  * NB: called from a signal handler.
  */
-int send_register_info(int sock, void *uc);
+int send_register_info(write_fn write_fn, void *uc);
 
 /* Read register info from the socket and compare it with that from the
  * ucontext. Return 0 for match, 1 for end-of-test, 2 for mismatch.
  * NB: called from a signal handler.
  */
-int recv_and_compare_register_info(int sock, void *uc);
+int recv_and_compare_register_info(read_fn read_fn, respond_fn respond, void *uc);
 
 /* Print a useful report on the status of the last comparison
  * done in recv_and_compare_register_info(). This is called on