| Message ID | 1602508140-11372-4-git-send-email-yubihong@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix some style problems in migration | expand |
* 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
"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 *[]) { [...]
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 *[]) { > > [...] > > . >
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. [...]
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. > > [...] > > . >
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; }