From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58564) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUe7R-00027e-JU for qemu-devel@nongnu.org; Mon, 31 Mar 2014 11:27:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WUe7L-0000zQ-7K for qemu-devel@nongnu.org; Mon, 31 Mar 2014 11:27:25 -0400 Date: Mon, 31 Mar 2014 18:27:42 +0300 From: "Michael S. Tsirkin" Message-ID: <20140331152742.GA10939@redhat.com> References: <1396275242-10810-1-git-send-email-mst@redhat.com> <1396275242-10810-2-git-send-email-mst@redhat.com> <20140331150134.GB11125@work-vm> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140331150134.GB11125@work-vm> Subject: Re: [Qemu-devel] [PATCH v4 01/30] vmstate: reduce code duplication List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: mdroth@linux.vnet.ibm.com, Juan Quintela , Alexey Kardashevskiy , qemu-stable@nongnu.org, qemu-devel@nongnu.org, =?us-ascii?B?PT9VVEYtOD9xP0FuZHJlYXM9MjBGPUMzPUE0cmJlcj89?= On Mon, Mar 31, 2014 at 04:01:34PM +0100, Dr. David Alan Gilbert wrote: > * Michael S. Tsirkin (mst@redhat.com) wrote: > > move size offset and number of elements math out > > to functions, to reduce code duplication. > > > > Signed-off-by: Michael S. Tsirkin > > --- > > vmstate.c | 97 ++++++++++++++++++++++++++++++++++----------------------------- > > 1 file changed, 52 insertions(+), 45 deletions(-) > > > > diff --git a/vmstate.c b/vmstate.c > > index b689f2f..dc99e1a 100644 > > --- a/vmstate.c > > +++ b/vmstate.c > > @@ -10,6 +10,50 @@ static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, > > static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd, > > void *opaque); > > > > +static int vmstate_n_elems(void *opaque, VMStateField *field) > > +{ > > + int n_elems = 1; > > + > > + if (field->flags & VMS_ARRAY) { > > + n_elems = field->num; > > + } else if (field->flags & VMS_VARRAY_INT32) { > > + n_elems = *(int32_t *)(opaque+field->num_offset); > > + } else if (field->flags & VMS_VARRAY_UINT32) { > > + n_elems = *(uint32_t *)(opaque+field->num_offset); > > + } else if (field->flags & VMS_VARRAY_UINT16) { > > + n_elems = *(uint16_t *)(opaque+field->num_offset); > > + } else if (field->flags & VMS_VARRAY_UINT8) { > > + n_elems = *(uint8_t *)(opaque+field->num_offset); > > + } > > + > > + return n_elems; > > +} > > + > > +static int vmstate_size(void *opaque, VMStateField *field) > > +{ > > + int size = field->size; > > + > > + if (field->flags & VMS_VBUFFER) { > > + size = *(int32_t *)(opaque+field->size_offset); > > + if (field->flags & VMS_MULTIPLY) { > > + size *= field->size; > > + } > > + } > > + > > + return size; > > +} > > + > > +static void *vmstate_base_addr(void *opaque, VMStateField *field) > > +{ > > + void *base_addr = opaque + field->offset; > > + > > + if (field->flags & VMS_POINTER) { > > + base_addr = *(void **)base_addr + field->start; > > + } > > + > > + return base_addr; > > +} > > + > > int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > > void *opaque, int version_id) > > { > > @@ -36,30 +80,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > > field->field_exists(opaque, version_id)) || > > (!field->field_exists && > > field->version_id <= version_id)) { > > - void *base_addr = opaque + field->offset; > > - int i, n_elems = 1; > > - int size = field->size; > > - > > - if (field->flags & VMS_VBUFFER) { > > - size = *(int32_t *)(opaque+field->size_offset); > > - if (field->flags & VMS_MULTIPLY) { > > - size *= field->size; > > - } > > - } > > - if (field->flags & VMS_ARRAY) { > > - n_elems = field->num; > > - } else if (field->flags & VMS_VARRAY_INT32) { > > - n_elems = *(int32_t *)(opaque+field->num_offset); > > - } else if (field->flags & VMS_VARRAY_UINT32) { > > - n_elems = *(uint32_t *)(opaque+field->num_offset); > > - } else if (field->flags & VMS_VARRAY_UINT16) { > > - n_elems = *(uint16_t *)(opaque+field->num_offset); > > - } else if (field->flags & VMS_VARRAY_UINT8) { > > - n_elems = *(uint8_t *)(opaque+field->num_offset); > > - } > > - if (field->flags & VMS_POINTER) { > > - base_addr = *(void **)base_addr + field->start; > > - } > > + void *base_addr = vmstate_base_addr(opaque, field); > > + int i, n_elems = vmstate_n_elems(opaque, field); > > + int size = vmstate_size(opaque, field); > > + > > for (i = 0; i < n_elems; i++) { > > void *addr = base_addr + size * i; > > > > @@ -102,27 +126,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, > > while (field->name) { > > if (!field->field_exists || > > field->field_exists(opaque, vmsd->version_id)) { > > - void *base_addr = opaque + field->offset; > > - int i, n_elems = 1; > > - int size = field->size; > > - > > - if (field->flags & VMS_VBUFFER) { > > - size = *(int32_t *)(opaque+field->size_offset); > > - if (field->flags & VMS_MULTIPLY) { > > - size *= field->size; > > - } > > - } > > - if (field->flags & VMS_ARRAY) { > > - n_elems = field->num; > > - } else if (field->flags & VMS_VARRAY_INT32) { > > - n_elems = *(int32_t *)(opaque+field->num_offset); > > - } else if (field->flags & VMS_VARRAY_UINT32) { > > - n_elems = *(uint32_t *)(opaque+field->num_offset); > > - } else if (field->flags & VMS_VARRAY_UINT16) { > > - n_elems = *(uint16_t *)(opaque+field->num_offset); > > - } else if (field->flags & VMS_VARRAY_UINT8) { > > - n_elems = *(uint8_t *)(opaque+field->num_offset); > > - } > > + void *base_addr = vmstate_base_addr(opaque, field); > > + int i, n_elems = vmstate_n_elems(opaque, field); > > + int size = vmstate_size(opaque, field); > > + > > if (field->flags & VMS_POINTER) { > > base_addr = *(void **)base_addr + field->start; > > } > > Hmm, shouldn't those last 3 lines be deleted as well - the logic is > now in vmstate_base_addr? > > > Dave So it should, thanks. > > -- > > MST > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK