[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/5] migration/vmstate: split up vmstate_base_ad
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH 2/5] migration/vmstate: split up vmstate_base_addr |
Date: |
Tue, 21 Feb 2017 12:07:00 +0000 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
* Halil Pasic (address@hidden) wrote:
> Currently vmstate_base_addr does several things: it pinpoints the field
> within the struct, possibly allocates memory and possibly does the first
> pointer dereference. Obviously allocation is needed only for load.
>
> Let us split up the functionality in vmstate_base_addr and move the
> address manipulations (that is everything but the allocation logic) to
> load and save so it becomes more obvious what is actually going on. Like
> this all the address calculations (and the handling of the flags
> controlling these) is in one place and the sequence is more obvious.
>
> The newly introduced function vmstate_handle_alloc also fixes the
> allocation for the unused VMS_VBUFFER| VMS_MULTIPLY scenario and is
> substantially simpler than the original vmstate_base_addr.
Note that VMSTATE_VBUFFER_MULTIPLY does use VMS_VBUFFER|VMS_MULTIPLY - but
not with alloc (and I started using VMSTATE_VBUFFER_MULTIPLY in a recent
patch that went in). I'm not sure I see what the error is that you fixed.
However, I'm ok with it:
Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> In load and save some asserts are added so it's easier to debug
> situations where we would end up with a null pointer dereference.
>
> Signed-off-by: Halil Pasic <address@hidden>
> ---
> migration/vmstate.c | 39 +++++++++++++++++----------------------
> 1 file changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 36efa80..836a7a4 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -52,29 +52,15 @@ static int vmstate_size(void *opaque, VMStateField *field)
> return size;
> }
>
> -static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
> +static void vmstate_handle_alloc(void *ptr, VMStateField *field, void
> *opaque)
> {
> - void *base_addr = opaque + field->offset;
> -
> - if (field->flags & VMS_POINTER) {
> - if (alloc && (field->flags & VMS_ALLOC)) {
> - gsize size = 0;
> - if (field->flags & VMS_VBUFFER) {
> - size = vmstate_size(opaque, field);
> - } else {
> - int n_elems = vmstate_n_elems(opaque, field);
> - if (n_elems) {
> - size = n_elems * field->size;
> - }
> - }
> - if (size) {
> - *(void **)base_addr = g_malloc(size);
> - }
> + if (field->flags & VMS_POINTER && field->flags & VMS_ALLOC) {
> + gsize size = vmstate_size(opaque, field);
> + size *= vmstate_n_elems(opaque, field);
> + if (size) {
> + *(void **)ptr = g_malloc(size);
> }
> - base_addr = *(void **)base_addr;
> }
> -
> - return base_addr;
> }
>
> int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> @@ -116,10 +102,15 @@ int vmstate_load_state(QEMUFile *f, const
> VMStateDescription *vmsd,
> field->field_exists(opaque, version_id)) ||
> (!field->field_exists &&
> field->version_id <= version_id)) {
> - void *first_elem = vmstate_base_addr(opaque, field, true);
> + void *first_elem = opaque + field->offset;
> int i, n_elems = vmstate_n_elems(opaque, field);
> int size = vmstate_size(opaque, field);
>
> + vmstate_handle_alloc(first_elem, field, opaque);
> + if (field->flags & VMS_POINTER) {
> + first_elem = *(void **)first_elem;
> + assert(first_elem || !n_elems);
> + }
> for (i = 0; i < n_elems; i++) {
> void *curr_elem = first_elem + size * i;
>
> @@ -321,13 +312,17 @@ void vmstate_save_state(QEMUFile *f, const
> VMStateDescription *vmsd,
> while (field->name) {
> if (!field->field_exists ||
> field->field_exists(opaque, vmsd->version_id)) {
> - void *first_elem = vmstate_base_addr(opaque, field, false);
> + void *first_elem = opaque + field->offset;
> int i, n_elems = vmstate_n_elems(opaque, field);
> int size = vmstate_size(opaque, field);
> int64_t old_offset, written_bytes;
> QJSON *vmdesc_loop = vmdesc;
>
> trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
> + if (field->flags & VMS_POINTER) {
> + first_elem = *(void **)first_elem;
> + assert(first_elem || !n_elems);
> + }
> for (i = 0; i < n_elems; i++) {
> void *curr_elem = first_elem + size * i;
>
> --
> 2.8.4
>
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK
[Qemu-devel] [PATCH 2/5] migration/vmstate: split up vmstate_base_addr, Halil Pasic, 2017/02/16
- Re: [Qemu-devel] [PATCH 2/5] migration/vmstate: split up vmstate_base_addr,
Dr. David Alan Gilbert <=
[Qemu-devel] [PATCH 4/5] tests/test-vmstate.c: test array of ptr with null, Halil Pasic, 2017/02/16
[Qemu-devel] [PATCH 5/5] tests/test-vmstate.c: test array of ptr to primitive, Halil Pasic, 2017/02/16