From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:51381) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RfS8O-0007Qy-Ou for qemu-devel@nongnu.org; Tue, 27 Dec 2011 03:11:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RfS8N-0002C5-4V for qemu-devel@nongnu.org; Tue, 27 Dec 2011 03:11:44 -0500 Received: from mailout2.w1.samsung.com ([210.118.77.12]:10066) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RfS8N-0002Bv-0G for qemu-devel@nongnu.org; Tue, 27 Dec 2011 03:11:43 -0500 Received: from euspt2 (mailout2.w1.samsung.com [210.118.77.12]) by mailout2.w1.samsung.com (iPlanet Messaging Server 5.2 Patch 2 (built Jul 14 2004)) with ESMTP id <0LWU00LJES3F22@mailout2.w1.samsung.com> for qemu-devel@nongnu.org; Tue, 27 Dec 2011 08:11:39 +0000 (GMT) Received: from [106.109.8.52] by spt2.w1.samsung.com (iPlanet Messaging Server 5.2 Patch 2 (built Jul 14 2004)) with ESMTPA id <0LWU00HUPS3FLE@spt2.w1.samsung.com> for qemu-devel@nongnu.org; Tue, 27 Dec 2011 08:11:39 +0000 (GMT) Date: Tue, 27 Dec 2011 12:11:38 +0400 From: Mitsyanko Igor In-reply-to: Message-id: <4EF97DBA.9040604@samsung.com> MIME-version: 1.0 Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7BIT References: <1324893824-13558-1-git-send-email-i.mitsyanko@samsung.com> <1324893824-13558-2-git-send-email-i.mitsyanko@samsung.com> Subject: Re: [Qemu-devel] [PATCH 1/3] vmstate: introduce calc_size VMStateField Reply-To: i.mitsyanko@samsung.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Peter Maydell , Juan Quintela On 12/26/2011 07:20 PM, Peter Maydell wrote: > On 26 December 2011 10:03, Mitsyanko Igor wrote: >> New calc_size field in VMStateField is supposed to help us easily add save/restore >> support of dynamically allocated buffers in device's states. >> There are some cases when information on size of dynamically allocated buffer is >> already presented in specific device's state structure, but in such a form, that >> can not be used with existing VMStateField interface. Currently, we either can get size from >> another variable in device's state as it is with VMSTATE_VBUFFER_* macros, or we can >> also multiply value kept in a variable by a constant with VMSTATE_BUFFER_MULTIPLY >> macro. If we need to perform any other action, we're forced to add additional >> variable with size information to device state structure with the only intention >> to use it in VMStateDescription structure. This approach is not very pretty. Adding extra >> flags to VMStateFlags enum for every other possible operation with size field >> seems redundant, and still it would't cover cases when we need to perform a set of >> operations to get size value. >> With this new .calc_size field we can calculate size of dynamic array in whichever >> way we need. >> >> Signed-off-by: Mitsyanko Igor > > It seems a bit curious that this patch removes the existing (although admittedly > unused) VMSTATE_BUFFER_MULTIPLY but doesn't actually say so in the > commit message. > >> --- >> hw/hw.h | 14 +++++++------- >> savevm.c | 14 ++++++++------ >> 2 files changed, 15 insertions(+), 13 deletions(-) >> >> diff --git a/hw/hw.h b/hw/hw.h >> index efa04d1..8ce4475 100644 >> --- a/hw/hw.h >> +++ b/hw/hw.h >> @@ -303,9 +303,9 @@ enum VMStateFlags { >> VMS_ARRAY_OF_POINTER = 0x040, >> VMS_VARRAY_UINT16 = 0x080, /* Array with size in uint16_t field */ >> VMS_VBUFFER = 0x100, /* Buffer with size in int32_t field */ >> - VMS_MULTIPLY = 0x200, /* multiply "size" field by field_size */ >> - VMS_VARRAY_UINT8 = 0x400, /* Array with size in uint8_t field*/ >> - VMS_VARRAY_UINT32 = 0x800, /* Array with size in uint32_t field*/ >> + VMS_CALC_SIZE = 0x200, /* calculate size of dynamic buffer */ >> + VMS_VARRAY_UINT8 = 0x400, /* Array with size in uint8_t field */ >> + VMS_VARRAY_UINT32 = 0x800, /* Array with size in uint32_t field */ > > These unrelated whitespace fixes are confusing -- please drop them. QEMU wiki here http://wiki.qemu.org/Contribute/SubmitAPatch states that it's ok :) > It's OK to fix coding style issues in the immediate area (few lines) > of the lines you're changing.) Given some thoughts to it, I didn't like that this patch increases memory consumed by VMStateField. Maybe we should drop existing .size_offset field and replace it with generic get_buffsize callback, like we did in second version of this patch. It seems reasonable to me, even though existing .size_offset usage is pretty neat. -- Mitsyanko Igor ASWG, Moscow R&D center, Samsung Electronics email: i.mitsyanko@samsung.com