[PATCH v2 0/8] Fix some style problems in migration

[PATCH v2 0/8] Fix some style problems in migration

[Bihong Yu]

Recently I am reading migration related code, find some style problems in
migration directory while using checkpatch.pl to check migration code. Fix the
error style problems.

v2:
- fix Signed-off-by error
- fix printf format error: "%0x" -> "0x%x"

Bihong Yu (8):
  migration: Do not use C99 // comments
  migration: Don't use '#' flag of printf format
  migration: Add spaces around operator
  migration: Open brace '{' following struct go on the same line
  migration: Add braces {} for if statement
  migration: Do not initialise statics and globals to 0 or NULL
  migration: Open brace '{' following function declarations go on the
    next line
  migration: Delete redundant spaces

 migration/block.c        |  4 ++--
 migration/migration.c    |  4 ++--
 migration/migration.h    |  3 +--
 migration/postcopy-ram.c |  2 +-
 migration/ram.c          | 14 ++++++++------
 migration/rdma.c         |  7 ++++---
 migration/savevm.c       |  4 ++--
 migration/vmstate.c      | 10 +++++-----
 8 files changed, 25 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/8] migration: Do not use C99 // comments

[Bihong Yu]

Signed-off-by: Bihong Yu <yubihong@huawei.com>
Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
---
 migration/block.c | 2 +-
 migration/rdma.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index 737b649..4b8576b 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -40,7 +40,7 @@
 #define MAX_IO_BUFFERS 512
 #define MAX_PARALLEL_IO 16
 
-//#define DEBUG_BLK_MIGRATION
+/* #define DEBUG_BLK_MIGRATION */
 
 #ifdef DEBUG_BLK_MIGRATION
 #define DPRINTF(fmt, ...) \
diff --git a/migration/rdma.c b/migration/rdma.c
index 0340841..0eb42b7 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -1288,7 +1288,7 @@ const char *print_wrid(int wrid)
  * workload information or LRU information is available, do not attempt to use
  * this feature except for basic testing.
  */
-//#define RDMA_UNREGISTRATION_EXAMPLE
+/* #define RDMA_UNREGISTRATION_EXAMPLE */
 
 /*
  * Perform a non-optimized memory unregistration after every transfer
-- 
1.8.3.1

[PATCH v2 2/8] migration: Don't use '#' flag of printf format

[Bihong Yu]

Signed-off-by: Bihong Yu <yubihong@huawei.com>
Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
---
 migration/block.c | 2 +-
 migration/ram.c   | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index 4b8576b..273392b 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -998,7 +998,7 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
                    (addr == 100) ? '\n' : '\r');
             fflush(stdout);
         } else if (!(flags & BLK_MIG_FLAG_EOS)) {
-            fprintf(stderr, "Unknown block migration flags: %#x\n", flags);
+            fprintf(stderr, "Unknown block migration flags: 0x%x\n", flags);
             return -EINVAL;
         }
         ret = qemu_file_get_error(f);
diff --git a/migration/ram.c b/migration/ram.c
index 433489d..6ed4f9e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3298,7 +3298,7 @@ static int ram_load_postcopy(QEMUFile *f)
             multifd_recv_sync_main();
             break;
         default:
-            error_report("Unknown combination of migration flags: %#x"
+            error_report("Unknown combination of migration flags: 0x%x"
                          " (postcopy mode)", flags);
             ret = -EINVAL;
             break;
@@ -3576,7 +3576,7 @@ static int ram_load_precopy(QEMUFile *f)
             if (flags & RAM_SAVE_FLAG_HOOK) {
                 ram_control_load_hook(f, RAM_CONTROL_HOOK, NULL);
             } else {
-                error_report("Unknown combination of migration flags: %#x",
+                error_report("Unknown combination of migration flags: 0x%x",
                              flags);
                 ret = -EINVAL;
             }
-- 
1.8.3.1

[PATCH v2 3/8] migration: Add spaces around operator

[Bihong Yu]

Signed-off-by: Bihong Yu <yubihong@huawei.com>
Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
---
 migration/migration.c    |  4 ++--
 migration/postcopy-ram.c |  2 +-
 migration/ram.c          |  2 +-
 migration/savevm.c       |  2 +-
 migration/vmstate.c      | 10 +++++-----
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0575ecb..e050f57 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2478,8 +2478,8 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname,
      * Since we currently insist on matching page sizes, just sanity check
      * we're being asked for whole host pages.
      */
-    if (start & (our_host_ps-1) ||
-       (len & (our_host_ps-1))) {
+    if (start & (our_host_ps - 1) ||
+       (len & (our_host_ps - 1))) {
         error_report("%s: Misaligned page request, start: " RAM_ADDR_FMT
                      " len: %zd", __func__, start, len);
         mark_source_rp_bad(ms);
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 0a2f88a8..eea92bb 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -403,7 +403,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
                      strerror(errno));
         goto out;
     }
-    g_assert(((size_t)testarea & (pagesize-1)) == 0);
+    g_assert(((size_t)testarea & (pagesize - 1)) == 0);
 
     reg_struct.range.start = (uintptr_t)testarea;
     reg_struct.range.len = pagesize;
diff --git a/migration/ram.c b/migration/ram.c
index 6ed4f9e..0aea78f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1563,7 +1563,7 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len)
         rs->last_req_rb = ramblock;
     }
     trace_ram_save_queue_pages(ramblock->idstr, start, len);
-    if (start+len > ramblock->used_length) {
+    if (start + len > ramblock->used_length) {
         error_report("%s request overrun start=" RAM_ADDR_FMT " len="
                      RAM_ADDR_FMT " blocklen=" RAM_ADDR_FMT,
                      __func__, start, len, ramblock->used_length);
diff --git a/migration/savevm.c b/migration/savevm.c
index d2e141f..9e95df1 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -521,7 +521,7 @@ static const VMStateDescription vmstate_configuration = {
         VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, len),
         VMSTATE_END_OF_LIST()
     },
-    .subsections = (const VMStateDescription*[]) {
+    .subsections = (const VMStateDescription * []) {
         &vmstate_target_page_bits,
         &vmstate_capabilites,
         &vmstate_uuid,
diff --git a/migration/vmstate.c b/migration/vmstate.c
index bafa890..e9d2aef 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -32,13 +32,13 @@ static int vmstate_n_elems(void *opaque, const VMStateField *field)
     if (field->flags & VMS_ARRAY) {
         n_elems = field->num;
     } else if (field->flags & VMS_VARRAY_INT32) {
-        n_elems = *(int32_t *)(opaque+field->num_offset);
+        n_elems = *(int32_t *)(opaque + field->num_offset);
     } else if (field->flags & VMS_VARRAY_UINT32) {
-        n_elems = *(uint32_t *)(opaque+field->num_offset);
+        n_elems = *(uint32_t *)(opaque + field->num_offset);
     } else if (field->flags & VMS_VARRAY_UINT16) {
-        n_elems = *(uint16_t *)(opaque+field->num_offset);
+        n_elems = *(uint16_t *)(opaque + field->num_offset);
     } else if (field->flags & VMS_VARRAY_UINT8) {
-        n_elems = *(uint8_t *)(opaque+field->num_offset);
+        n_elems = *(uint8_t *)(opaque + field->num_offset);
     }
 
     if (field->flags & VMS_MULTIPLY_ELEMENTS) {
@@ -54,7 +54,7 @@ static int vmstate_size(void *opaque, const VMStateField *field)
     int size = field->size;
 
     if (field->flags & VMS_VBUFFER) {
-        size = *(int32_t *)(opaque+field->size_offset);
+        size = *(int32_t *)(opaque + field->size_offset);
         if (field->flags & VMS_MULTIPLY) {
             size *= field->size;
         }
-- 
1.8.3.1

[PATCH v2 4/8] migration: Open brace '{' following struct go on the same line

[Bihong Yu]

Signed-off-by: Bihong Yu <yubihong@huawei.com>
Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
---
 migration/migration.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index deb411a..99784b4 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -124,8 +124,7 @@ struct MigrationClass {
     DeviceClass parent_class;
 };
 
-struct MigrationState
-{
+struct MigrationState {
     /*< private >*/
     DeviceState parent_obj;
 
-- 
1.8.3.1

[PATCH v2 5/8] migration: Add braces {} for if statement

[Bihong Yu]

Signed-off-by: Bihong Yu <yubihong@huawei.com>
Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
---
 migration/ram.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 0aea78f..09178cc 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -101,14 +101,16 @@ static struct {
 
 static void XBZRLE_cache_lock(void)
 {
-    if (migrate_use_xbzrle())
+    if (migrate_use_xbzrle()) {
         qemu_mutex_lock(&XBZRLE.lock);
+    }
 }
 
 static void XBZRLE_cache_unlock(void)
 {
-    if (migrate_use_xbzrle())
+    if (migrate_use_xbzrle()) {
         qemu_mutex_unlock(&XBZRLE.lock);
+    }
 }
 
 /**
-- 
1.8.3.1

[PATCH v2 6/8] migration: Do not initialise statics and globals to 0 or NULL

[Bihong Yu]

Signed-off-by: Bihong Yu <yubihong@huawei.com>
Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
---
 migration/ram.c    | 2 +-
 migration/savevm.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 09178cc..2da2b62 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2743,7 +2743,7 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
  */
 static inline RAMBlock *ram_block_from_stream(QEMUFile *f, int flags)
 {
-    static RAMBlock *block = NULL;
+    static RAMBlock *block;
     char id[256];
     uint8_t len;
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 9e95df1..f808bc2 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -64,7 +64,7 @@
 #include "qemu/bitmap.h"
 #include "net/announce.h"
 
-const unsigned int postcopy_ram_discard_version = 0;
+const unsigned int postcopy_ram_discard_version;
 
 /* Subcommands for QEMU_VM_COMMAND */
 enum qemu_vm_cmd {
-- 
1.8.3.1

[PATCH v2 7/8] migration: Open brace '{' following function declarations go on the next line

[Bihong Yu]

Signed-off-by: Bihong Yu <yubihong@huawei.com>
Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
---
 migration/rdma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 0eb42b7..ca4d315 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -273,7 +273,8 @@ static uint64_t htonll(uint64_t v)
     return u.llv;
 }
 
-static uint64_t ntohll(uint64_t v) {
+static uint64_t ntohll(uint64_t v)
+{
     union { uint32_t lv[2]; uint64_t llv; } u;
     u.llv = v;
     return ((uint64_t)ntohl(u.lv[0]) << 32) | (uint64_t) ntohl(u.lv[1]);
-- 
1.8.3.1

[PATCH v2 8/8] migration: Delete redundant spaces

[Bihong Yu]

Signed-off-by: Bihong Yu <yubihong@huawei.com>
Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
---
 migration/rdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index ca4d315..00eac34 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -855,7 +855,7 @@ static int qemu_rdma_broken_ipv6_kernel(struct ibv_context *verbs, Error **errp)
      */
     if (!verbs) {
         int num_devices, x;
-        struct ibv_device ** dev_list = ibv_get_device_list(&num_devices);
+        struct ibv_device **dev_list = ibv_get_device_list(&num_devices);
         bool roce_found = false;
         bool ib_found = false;
 
-- 
1.8.3.1

Re: [PATCH v2 1/8] migration: Do not use C99 // comments

[Dr. David Alan Gilbert]

* Bihong Yu (yubihong@huawei.com) wrote:
> Signed-off-by: Bihong Yu <yubihong@huawei.com>
> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

another task at some point would be to rewrite the DPRINTF's in
migration/block.c to use trace_ instead.

> ---
>  migration/block.c | 2 +-
>  migration/rdma.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/block.c b/migration/block.c
> index 737b649..4b8576b 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -40,7 +40,7 @@
>  #define MAX_IO_BUFFERS 512
>  #define MAX_PARALLEL_IO 16
>  
> -//#define DEBUG_BLK_MIGRATION
> +/* #define DEBUG_BLK_MIGRATION */
>  
>  #ifdef DEBUG_BLK_MIGRATION
>  #define DPRINTF(fmt, ...) \
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 0340841..0eb42b7 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -1288,7 +1288,7 @@ const char *print_wrid(int wrid)
>   * workload information or LRU information is available, do not attempt to use
>   * this feature except for basic testing.
>   */
> -//#define RDMA_UNREGISTRATION_EXAMPLE
> +/* #define RDMA_UNREGISTRATION_EXAMPLE */
>  
>  /*
>   * Perform a non-optimized memory unregistration after every transfer
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 3/8] migration: Add spaces around operator

[Dr. David Alan Gilbert]

* Bihong Yu (yubihong@huawei.com) wrote:
> Signed-off-by: Bihong Yu <yubihong@huawei.com>
> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>

Yes that's OK, I'm a bit sturprised we need the space afte rthe * in the
VMStateDescription case, I wouldn't necessarily go and change them all.



Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c    |  4 ++--
>  migration/postcopy-ram.c |  2 +-
>  migration/ram.c          |  2 +-
>  migration/savevm.c       |  2 +-
>  migration/vmstate.c      | 10 +++++-----
>  5 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 0575ecb..e050f57 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2478,8 +2478,8 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname,
>       * Since we currently insist on matching page sizes, just sanity check
>       * we're being asked for whole host pages.
>       */
> -    if (start & (our_host_ps-1) ||
> -       (len & (our_host_ps-1))) {
> +    if (start & (our_host_ps - 1) ||
> +       (len & (our_host_ps - 1))) {
>          error_report("%s: Misaligned page request, start: " RAM_ADDR_FMT
>                       " len: %zd", __func__, start, len);
>          mark_source_rp_bad(ms);
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 0a2f88a8..eea92bb 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -403,7 +403,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
>                       strerror(errno));
>          goto out;
>      }
> -    g_assert(((size_t)testarea & (pagesize-1)) == 0);
> +    g_assert(((size_t)testarea & (pagesize - 1)) == 0);
>  
>      reg_struct.range.start = (uintptr_t)testarea;
>      reg_struct.range.len = pagesize;
> diff --git a/migration/ram.c b/migration/ram.c
> index 6ed4f9e..0aea78f 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1563,7 +1563,7 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len)
>          rs->last_req_rb = ramblock;
>      }
>      trace_ram_save_queue_pages(ramblock->idstr, start, len);
> -    if (start+len > ramblock->used_length) {
> +    if (start + len > ramblock->used_length) {
>          error_report("%s request overrun start=" RAM_ADDR_FMT " len="
>                       RAM_ADDR_FMT " blocklen=" RAM_ADDR_FMT,
>                       __func__, start, len, ramblock->used_length);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index d2e141f..9e95df1 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -521,7 +521,7 @@ static const VMStateDescription vmstate_configuration = {
>          VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, len),
>          VMSTATE_END_OF_LIST()
>      },
> -    .subsections = (const VMStateDescription*[]) {
> +    .subsections = (const VMStateDescription * []) {
>          &vmstate_target_page_bits,
>          &vmstate_capabilites,
>          &vmstate_uuid,
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index bafa890..e9d2aef 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -32,13 +32,13 @@ static int vmstate_n_elems(void *opaque, const VMStateField *field)
>      if (field->flags & VMS_ARRAY) {
>          n_elems = field->num;
>      } else if (field->flags & VMS_VARRAY_INT32) {
> -        n_elems = *(int32_t *)(opaque+field->num_offset);
> +        n_elems = *(int32_t *)(opaque + field->num_offset);
>      } else if (field->flags & VMS_VARRAY_UINT32) {
> -        n_elems = *(uint32_t *)(opaque+field->num_offset);
> +        n_elems = *(uint32_t *)(opaque + field->num_offset);
>      } else if (field->flags & VMS_VARRAY_UINT16) {
> -        n_elems = *(uint16_t *)(opaque+field->num_offset);
> +        n_elems = *(uint16_t *)(opaque + field->num_offset);
>      } else if (field->flags & VMS_VARRAY_UINT8) {
> -        n_elems = *(uint8_t *)(opaque+field->num_offset);
> +        n_elems = *(uint8_t *)(opaque + field->num_offset);
>      }
>  
>      if (field->flags & VMS_MULTIPLY_ELEMENTS) {
> @@ -54,7 +54,7 @@ static int vmstate_size(void *opaque, const VMStateField *field)
>      int size = field->size;
>  
>      if (field->flags & VMS_VBUFFER) {
> -        size = *(int32_t *)(opaque+field->size_offset);
> +        size = *(int32_t *)(opaque + field->size_offset);
>          if (field->flags & VMS_MULTIPLY) {
>              size *= field->size;
>          }
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 1/8] migration: Do not use C99 // comments

[Bihong Yu]

Thank you for your review. OK, I will try to rewrite the DPRINTF to use trace_ instead.

On 2020/10/13 17:39, Dr. David Alan Gilbert wrote:
> * Bihong Yu (yubihong@huawei.com) wrote:
>> Signed-off-by: Bihong Yu <yubihong@huawei.com>
>> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> another task at some point would be to rewrite the DPRINTF's in
> migration/block.c to use trace_ instead.
> 
>> ---
>>  migration/block.c | 2 +-
>>  migration/rdma.c  | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/block.c b/migration/block.c
>> index 737b649..4b8576b 100644
>> --- a/migration/block.c
>> +++ b/migration/block.c
>> @@ -40,7 +40,7 @@
>>  #define MAX_IO_BUFFERS 512
>>  #define MAX_PARALLEL_IO 16
>>  
>> -//#define DEBUG_BLK_MIGRATION
>> +/* #define DEBUG_BLK_MIGRATION */
>>  
>>  #ifdef DEBUG_BLK_MIGRATION
>>  #define DPRINTF(fmt, ...) \
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index 0340841..0eb42b7 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -1288,7 +1288,7 @@ const char *print_wrid(int wrid)
>>   * workload information or LRU information is available, do not attempt to use
>>   * this feature except for basic testing.
>>   */
>> -//#define RDMA_UNREGISTRATION_EXAMPLE
>> +/* #define RDMA_UNREGISTRATION_EXAMPLE */
>>  
>>  /*
>>   * Perform a non-optimized memory unregistration after every transfer
>> -- 
>> 1.8.3.1
>>

Re: [PATCH v2 4/8] migration: Open brace '{' following struct go on the same line

[Dr. David Alan Gilbert]

* Bihong Yu (yubihong@huawei.com) wrote:
> Signed-off-by: Bihong Yu <yubihong@huawei.com>
> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>

Yes, I think that line probably predates the coding standard :-)


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/migration/migration.h b/migration/migration.h
> index deb411a..99784b4 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -124,8 +124,7 @@ struct MigrationClass {
>      DeviceClass parent_class;
>  };
>  
> -struct MigrationState
> -{
> +struct MigrationState {
>      /*< private >*/
>      DeviceState parent_obj;
>  
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 1/8] migration: Do not use C99 // comments

[Zheng Chuan]

Also DEBUG_CACHE in migration/page_cache.c is need to rebase on trace_calls.

On 2020/10/13 21:20, Bihong Yu wrote:
> Thank you for your review. OK, I will try to rewrite the DPRINTF to use trace_ instead.
> 
> On 2020/10/13 17:39, Dr. David Alan Gilbert wrote:
>> * Bihong Yu (yubihong@huawei.com) wrote:
>>> Signed-off-by: Bihong Yu <yubihong@huawei.com>
>>> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
>>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>
>> another task at some point would be to rewrite the DPRINTF's in
>> migration/block.c to use trace_ instead.
>>
>>> ---
>>>  migration/block.c | 2 +-
>>>  migration/rdma.c  | 2 +-
>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/migration/block.c b/migration/block.c
>>> index 737b649..4b8576b 100644
>>> --- a/migration/block.c
>>> +++ b/migration/block.c
>>> @@ -40,7 +40,7 @@
>>>  #define MAX_IO_BUFFERS 512
>>>  #define MAX_PARALLEL_IO 16
>>>  
>>> -//#define DEBUG_BLK_MIGRATION
>>> +/* #define DEBUG_BLK_MIGRATION */
>>>  
>>>  #ifdef DEBUG_BLK_MIGRATION
>>>  #define DPRINTF(fmt, ...) \
>>> diff --git a/migration/rdma.c b/migration/rdma.c
>>> index 0340841..0eb42b7 100644
>>> --- a/migration/rdma.c
>>> +++ b/migration/rdma.c
>>> @@ -1288,7 +1288,7 @@ const char *print_wrid(int wrid)
>>>   * workload information or LRU information is available, do not attempt to use
>>>   * this feature except for basic testing.
>>>   */
>>> -//#define RDMA_UNREGISTRATION_EXAMPLE
>>> +/* #define RDMA_UNREGISTRATION_EXAMPLE */
>>>  
>>>  /*
>>>   * Perform a non-optimized memory unregistration after every transfer
>>> -- 
>>> 1.8.3.1
>>>
> .
> 

-- 
Thanks, Chuan

Re: [PATCH v2 1/8] migration: Do not use C99 // comments

[Bihong Yu]

OK, I will modify it later.

On 2020/10/14 9:29, Zheng Chuan wrote:
> Also DEBUG_CACHE in migration/page_cache.c is need to rebase on trace_calls.
> 
> On 2020/10/13 21:20, Bihong Yu wrote:
>> Thank you for your review. OK, I will try to rewrite the DPRINTF to use trace_ instead.
>>
>> On 2020/10/13 17:39, Dr. David Alan Gilbert wrote:
>>> * Bihong Yu (yubihong@huawei.com) wrote:
>>>> Signed-off-by: Bihong Yu <yubihong@huawei.com>
>>>> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
>>>
>>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>
>>> another task at some point would be to rewrite the DPRINTF's in
>>> migration/block.c to use trace_ instead.
>>>
>>>> ---
>>>>  migration/block.c | 2 +-
>>>>  migration/rdma.c  | 2 +-
>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/migration/block.c b/migration/block.c
>>>> index 737b649..4b8576b 100644
>>>> --- a/migration/block.c
>>>> +++ b/migration/block.c
>>>> @@ -40,7 +40,7 @@
>>>>  #define MAX_IO_BUFFERS 512
>>>>  #define MAX_PARALLEL_IO 16
>>>>  
>>>> -//#define DEBUG_BLK_MIGRATION
>>>> +/* #define DEBUG_BLK_MIGRATION */
>>>>  
>>>>  #ifdef DEBUG_BLK_MIGRATION
>>>>  #define DPRINTF(fmt, ...) \
>>>> diff --git a/migration/rdma.c b/migration/rdma.c
>>>> index 0340841..0eb42b7 100644
>>>> --- a/migration/rdma.c
>>>> +++ b/migration/rdma.c
>>>> @@ -1288,7 +1288,7 @@ const char *print_wrid(int wrid)
>>>>   * workload information or LRU information is available, do not attempt to use
>>>>   * this feature except for basic testing.
>>>>   */
>>>> -//#define RDMA_UNREGISTRATION_EXAMPLE
>>>> +/* #define RDMA_UNREGISTRATION_EXAMPLE */
>>>>  
>>>>  /*
>>>>   * Perform a non-optimized memory unregistration after every transfer
>>>> -- 
>>>> 1.8.3.1
>>>>
>> .
>>
> 

Re: [PATCH v2 8/8] migration: Delete redundant spaces

[Dr. David Alan Gilbert]

* Bihong Yu (yubihong@huawei.com) wrote:
> Signed-off-by: Bihong Yu <yubihong@huawei.com>
> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/rdma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index ca4d315..00eac34 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -855,7 +855,7 @@ static int qemu_rdma_broken_ipv6_kernel(struct ibv_context *verbs, Error **errp)
>       */
>      if (!verbs) {
>          int num_devices, x;
> -        struct ibv_device ** dev_list = ibv_get_device_list(&num_devices);
> +        struct ibv_device **dev_list = ibv_get_device_list(&num_devices);
>          bool roce_found = false;
>          bool ib_found = false;
>  
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 5/8] migration: Add braces {} for if statement

[Dr. David Alan Gilbert]

* Bihong Yu (yubihong@huawei.com) wrote:
> Signed-off-by: Bihong Yu <yubihong@huawei.com>
> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/ram.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 0aea78f..09178cc 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -101,14 +101,16 @@ static struct {
>  
>  static void XBZRLE_cache_lock(void)
>  {
> -    if (migrate_use_xbzrle())
> +    if (migrate_use_xbzrle()) {
>          qemu_mutex_lock(&XBZRLE.lock);
> +    }
>  }
>  
>  static void XBZRLE_cache_unlock(void)
>  {
> -    if (migrate_use_xbzrle())
> +    if (migrate_use_xbzrle()) {
>          qemu_mutex_unlock(&XBZRLE.lock);
> +    }
>  }
>  
>  /**
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 6/8] migration: Do not initialise statics and globals to 0 or NULL

[Dr. David Alan Gilbert]

* Bihong Yu (yubihong@huawei.com) wrote:
> Signed-off-by: Bihong Yu <yubihong@huawei.com>
> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>

OK, sometimes  I find initialisation clearer


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/ram.c    | 2 +-
>  migration/savevm.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 09178cc..2da2b62 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2743,7 +2743,7 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
>   */
>  static inline RAMBlock *ram_block_from_stream(QEMUFile *f, int flags)
>  {
> -    static RAMBlock *block = NULL;
> +    static RAMBlock *block;
>      char id[256];
>      uint8_t len;
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 9e95df1..f808bc2 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -64,7 +64,7 @@
>  #include "qemu/bitmap.h"
>  #include "net/announce.h"
>  
> -const unsigned int postcopy_ram_discard_version = 0;
> +const unsigned int postcopy_ram_discard_version;
>  
>  /* Subcommands for QEMU_VM_COMMAND */
>  enum qemu_vm_cmd {
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 7/8] migration: Open brace '{' following function declarations go on the next line

[Dr. David Alan Gilbert]

* Bihong Yu (yubihong@huawei.com) wrote:
> Signed-off-by: Bihong Yu <yubihong@huawei.com>
> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/rdma.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 0eb42b7..ca4d315 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -273,7 +273,8 @@ static uint64_t htonll(uint64_t v)
>      return u.llv;
>  }
>  
> -static uint64_t ntohll(uint64_t v) {
> +static uint64_t ntohll(uint64_t v)
> +{
>      union { uint32_t lv[2]; uint64_t llv; } u;
>      u.llv = v;
>      return ((uint64_t)ntohl(u.lv[0]) << 32) | (uint64_t) ntohl(u.lv[1]);
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 2/8] migration: Don't use '#' flag of printf format

[Dr. David Alan Gilbert]

* Bihong Yu (yubihong@huawei.com) wrote:
> Signed-off-by: Bihong Yu <yubihong@huawei.com>
> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/block.c | 2 +-
>  migration/ram.c   | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/block.c b/migration/block.c
> index 4b8576b..273392b 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -998,7 +998,7 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
>                     (addr == 100) ? '\n' : '\r');
>              fflush(stdout);
>          } else if (!(flags & BLK_MIG_FLAG_EOS)) {
> -            fprintf(stderr, "Unknown block migration flags: %#x\n", flags);
> +            fprintf(stderr, "Unknown block migration flags: 0x%x\n", flags);
>              return -EINVAL;
>          }
>          ret = qemu_file_get_error(f);
> diff --git a/migration/ram.c b/migration/ram.c
> index 433489d..6ed4f9e 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3298,7 +3298,7 @@ static int ram_load_postcopy(QEMUFile *f)
>              multifd_recv_sync_main();
>              break;
>          default:
> -            error_report("Unknown combination of migration flags: %#x"
> +            error_report("Unknown combination of migration flags: 0x%x"
>                           " (postcopy mode)", flags);
>              ret = -EINVAL;
>              break;
> @@ -3576,7 +3576,7 @@ static int ram_load_precopy(QEMUFile *f)
>              if (flags & RAM_SAVE_FLAG_HOOK) {
>                  ram_control_load_hook(f, RAM_CONTROL_HOOK, NULL);
>              } else {
> -                error_report("Unknown combination of migration flags: %#x",
> +                error_report("Unknown combination of migration flags: 0x%x",
>                               flags);
>                  ret = -EINVAL;
>              }
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 3/8] migration: Add spaces around operator

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Bihong Yu (yubihong@huawei.com) wrote:
>> Signed-off-by: Bihong Yu <yubihong@huawei.com>
>> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
>
> Yes that's OK, I'm a bit sturprised we need the space afte rthe * in the
> VMStateDescription case, I wouldn't necessarily go and change them all.

We don't: it's not the binary multiplication operator *, where we want a
space on both sides, it's a pointer declarator, where we want a space on
the left only.

Example:

    int *pa, *pb, *pc;
    *pa = *pb * *pc;

Note the space on both side of binary operator * (multiplication), but
only on the left side of the pointer declarator's * and the unary
operator * (indirection).

> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
[...]
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index d2e141f..9e95df1 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -521,7 +521,7 @@ static const VMStateDescription vmstate_configuration = {
>>          VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, len),
>>          VMSTATE_END_OF_LIST()
>>      },
>> -    .subsections = (const VMStateDescription*[]) {
>> +    .subsections = (const VMStateDescription * []) {
>>          &vmstate_target_page_bits,
>>          &vmstate_capabilites,
>>          &vmstate_uuid,

Should be

   .subsections = (const VMStateDescription *[]) {

[...]

Re: [PATCH v2 3/8] migration: Add spaces around operator

[Bihong Yu]

Yes, I used to think "const VMStateDescription *[]" was right, but when I search
similar expressions, most of all are "xxx * []". Such as:
fsdev/qemu-fsdev.c:54:        .opts = (const char * [])
hw/intc/s390_flic_kvm.c:567:    .subsections = (const VMStateDescription * [])
...

So, I keep the same style. Should I change it to "const VMStateDescription *[]"?

On 2020/10/19 16:24, Markus Armbruster wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
>> * Bihong Yu (yubihong@huawei.com) wrote:
>>> Signed-off-by: Bihong Yu <yubihong@huawei.com>
>>> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
>>
>> Yes that's OK, I'm a bit sturprised we need the space afte rthe * in the
>> VMStateDescription case, I wouldn't necessarily go and change them all.
> 
> We don't: it's not the binary multiplication operator *, where we want a
> space on both sides, it's a pointer declarator, where we want a space on
> the left only.
> 
> Example:
> 
>     int *pa, *pb, *pc;
>     *pa = *pb * *pc;
> 
> Note the space on both side of binary operator * (multiplication), but
> only on the left side of the pointer declarator's * and the unary
> operator * (indirection).
> 
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> [...]
>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>> index d2e141f..9e95df1 100644
>>> --- a/migration/savevm.c
>>> +++ b/migration/savevm.c
>>> @@ -521,7 +521,7 @@ static const VMStateDescription vmstate_configuration = {
>>>          VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, len),
>>>          VMSTATE_END_OF_LIST()
>>>      },
>>> -    .subsections = (const VMStateDescription*[]) {
>>> +    .subsections = (const VMStateDescription * []) {
>>>          &vmstate_target_page_bits,
>>>          &vmstate_capabilites,
>>>          &vmstate_uuid,
> 
> Should be
> 
>    .subsections = (const VMStateDescription *[]) {
> 
> [...]
> 
> .
> 

Re: [PATCH v2 3/8] migration: Add spaces around operator

[Markus Armbruster]

Bihong Yu <yubihong@huawei.com> writes:

> Yes, I used to think "const VMStateDescription *[]" was right, but when I search
> similar expressions, most of all are "xxx * []". Such as:
> fsdev/qemu-fsdev.c:54:        .opts = (const char * [])
> hw/intc/s390_flic_kvm.c:567:    .subsections = (const VMStateDescription * [])
> ...

All three variations occur in the code: no space, space on both sides,
space only on the left.

> So, I keep the same style. Should I change it to "const VMStateDescription *[]"?

Dropping the change to savevm.c should be fine.

Changing it to "VMStateDescription *[]" should be also fine.

I figure you can keep David's R-by in both cases.

[...]

Re: [PATCH v2 3/8] migration: Add spaces around operator

[Bihong Yu]

OK, I will change it to "VMStateDescription *[]". Thank you for your review.

On 2020/10/19 19:59, Markus Armbruster wrote:
> Bihong Yu <yubihong@huawei.com> writes:
> 
>> Yes, I used to think "const VMStateDescription *[]" was right, but when I search
>> similar expressions, most of all are "xxx * []". Such as:
>> fsdev/qemu-fsdev.c:54:        .opts = (const char * [])
>> hw/intc/s390_flic_kvm.c:567:    .subsections = (const VMStateDescription * [])
>> ...
> 
> All three variations occur in the code: no space, space on both sides,
> space only on the left.
> 
>> So, I keep the same style. Should I change it to "const VMStateDescription *[]"?
> 
> Dropping the change to savevm.c should be fine.
> 
> Changing it to "VMStateDescription *[]" should be also fine.
> 
> I figure you can keep David's R-by in both cases.
> 
> [...]
> 
> .
> 

Re: [PATCH v2 2/8] migration: Don't use '#' flag of printf format

[Philippe Mathieu-Daudé]

On 10/12/20 3:08 PM, Bihong Yu wrote:
> Signed-off-by: Bihong Yu <yubihong@huawei.com>
> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
> ---
>   migration/block.c | 2 +-
>   migration/ram.c   | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 5/8] migration: Add braces {} for if statement

[Philippe Mathieu-Daudé]

On 10/12/20 3:08 PM, Bihong Yu wrote:
> Signed-off-by: Bihong Yu <yubihong@huawei.com>
> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
> ---
>   migration/ram.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 6/8] migration: Do not initialise statics and globals to 0 or NULL

[Philippe Mathieu-Daudé]

On 10/14/20 9:21 PM, Dr. David Alan Gilbert wrote:
> * Bihong Yu (yubihong@huawei.com) wrote:
>> Signed-off-by: Bihong Yu <yubihong@huawei.com>
>> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
> 
> OK, sometimes  I find initialisation clearer
> 
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
>> ---
>>   migration/ram.c    | 2 +-
>>   migration/savevm.c | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 09178cc..2da2b62 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2743,7 +2743,7 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
>>    */
>>   static inline RAMBlock *ram_block_from_stream(QEMUFile *f, int flags)
>>   {
>> -    static RAMBlock *block = NULL;
>> +    static RAMBlock *block;

OK.

>>       char id[256];
>>       uint8_t len;
>>   
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 9e95df1..f808bc2 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -64,7 +64,7 @@
>>   #include "qemu/bitmap.h"
>>   #include "net/announce.h"
>>   
>> -const unsigned int postcopy_ram_discard_version = 0;
>> +const unsigned int postcopy_ram_discard_version;

I agree with Dave, this one is clearer initialized.

>>   
>>   /* Subcommands for QEMU_VM_COMMAND */
>>   enum qemu_vm_cmd {
>> -- 
>> 1.8.3.1
>>

Re: [PATCH v2 8/8] migration: Delete redundant spaces

[Philippe Mathieu-Daudé]

On 10/12/20 3:09 PM, Bihong Yu wrote:
> Signed-off-by: Bihong Yu <yubihong@huawei.com>
> Reviewed-by: Chuan Zheng <zhengchuan@huawei.com>
> ---
>   migration/rdma.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>