Re: [Qemu-devel] [RISU PATCH v5 07/13] risu: paramterise send/receive functions

[Peter Maydell]

On 19 June 2017 at 11:46, Alex Bennée <address@hidden> 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 <address@hidden>
>
> ---
> 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