Re: [PATCH 17/52] migration/rdma: Replace dangerous macro CHECK_ERROR_STATE()

[Zhijian Li (Fujitsu)]

On 18/09/2023 22:41, Markus Armbruster wrote:
> Hiding return statements in macros is a bad idea.  Use a function
> instead, and open code the return part.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>


> ---
>   migration/rdma.c | 43 +++++++++++++++++++++++++++----------------
>   1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 30e6dff875..be66f53489 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -85,18 +85,6 @@
>    */
>   static uint32_t known_capabilities = RDMA_CAPABILITY_PIN_ALL;
>   
> -#define CHECK_ERROR_STATE() \
> -    do { \
> -        if (rdma->error_state) { \
> -            if (!rdma->error_reported) { \
> -                error_report("RDMA is in an error state waiting migration" \
> -                                " to abort!"); \
> -                rdma->error_reported = true; \
> -            } \
> -            return rdma->error_state; \
> -        } \
> -    } while (0)
> -
>   /*
>    * A work request ID is 64-bits and we split up these bits
>    * into 3 parts:
> @@ -451,6 +439,16 @@ typedef struct QEMU_PACKED {
>       uint64_t chunks;            /* how many sequential chunks to register */
>   } RDMARegister;
>   
> +static int check_error_state(RDMAContext *rdma)
> +{
> +    if (rdma->error_state && !rdma->error_reported) {
> +        error_report("RDMA is in an error state waiting migration"
> +                     " to abort!");
> +        rdma->error_reported = true;
> +    }
> +    return rdma->error_state;
> +}
> +
>   static void register_to_network(RDMAContext *rdma, RDMARegister *reg)
>   {
>       RDMALocalBlock *local_block;
> @@ -3219,7 +3217,10 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
>           return -EIO;
>       }
>   
> -    CHECK_ERROR_STATE();
> +    ret = check_error_state(rdma);
> +    if (ret) {
> +        return ret;
> +    }
>   
>       qemu_fflush(f);
>   
> @@ -3535,7 +3536,10 @@ static int qemu_rdma_registration_handle(QEMUFile *f)
>           return -EIO;
>       }
>   
> -    CHECK_ERROR_STATE();
> +    ret = check_error_state(rdma);
> +    if (ret) {
> +        return ret;
> +    }
>   
>       local = &rdma->local_ram_blocks;
>       do {
> @@ -3839,6 +3843,7 @@ static int qemu_rdma_registration_start(QEMUFile *f,
>   {
>       QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(qemu_file_get_ioc(f));
>       RDMAContext *rdma;
> +    int ret;
>   
>       if (migration_in_postcopy()) {
>           return 0;
> @@ -3850,7 +3855,10 @@ static int qemu_rdma_registration_start(QEMUFile *f,
>           return -EIO;
>       }
>   
> -    CHECK_ERROR_STATE();
> +    ret = check_error_state(rdma);
> +    if (ret) {
> +        return ret;
> +    }
>   
>       trace_qemu_rdma_registration_start(flags);
>       qemu_put_be64(f, RAM_SAVE_FLAG_HOOK);
> @@ -3881,7 +3889,10 @@ static int qemu_rdma_registration_stop(QEMUFile *f,
>           return -EIO;
>       }
>   
> -    CHECK_ERROR_STATE();
> +    ret = check_error_state(rdma);
> +    if (ret) {
> +        return ret;
> +    }
>   
>       qemu_fflush(f);
>       ret = qemu_rdma_drain_cq(f, rdma);