From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52760) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5cyY-00084y-94 for qemu-devel@nongnu.org; Wed, 25 May 2016 13:52:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b5cyX-0008Hp-8E for qemu-devel@nongnu.org; Wed, 25 May 2016 13:52:10 -0400 Date: Wed, 25 May 2016 13:51:55 -0400 (EDT) From: Paolo Bonzini Message-ID: <1171824705.17235966.1464198715349.JavaMail.zimbra@redhat.com> In-Reply-To: <5745D4E7.2000003@linux.vnet.ibm.com> References: <1464112509-21806-1-git-send-email-duanj@linux.vnet.ibm.com> <1464112509-21806-5-git-send-email-duanj@linux.vnet.ibm.com> <5745D4E7.2000003@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [QEMU RFC PATCH v2 4/6] Migration: migrate QTAILQ List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jianjun Duan Cc: qemu-devel@nongnu.org, quintela@redhat.com, mdroth@linux.vnet.ibm.com, qemu-ppc@nongnu.org, amit shah , david@gibson.dropbear.id.au > >> + /* > >> + * Following 3 fields are for VMStateField which needs customized > >> handling, > >> + * such as QTAILQ in qemu/queue.h, lists, and tree. > >> + */ > >> + const void *meta_data; > >> + int (*extend_get)(QEMUFile *f, const void *metadata, void *opaque); > >> + void (*extend_put)(QEMUFile *f, const void *metadata, void *opaque, > >> + QJSON *vmdesc); > >> } VMStateField; > > > > Do not add these two function pointers to VMStateField, instead add > > QJSON* and VMStateField* arguments as needed to VMStateInfo's get and put. > > That is definitely the ideal way. However, VMStateInfo's get/put are > already used extensively. If I change them, I need change all the > calling sites of them. Not very sure about whether it will be welcomed. Sure, don't be worried. :) True, there are quite a few VMStateInfos (about 35) but the changes are simple. > >> +#define VMSTATE_QTAILQ_METADATA(_field, _state, _type, _next, _vmsd) { \ > >> + .first = QTAILQ_FIRST_OFFSET(typeof_field(_state, _field)), \ > >> + .last = QTAILQ_LAST_OFFSET(typeof_field(_state, _field)), \ > >> + .next = QTAILQ_NEXT_OFFSET(_type, _next), \ > >> + .prev = QTAILQ_PREV_OFFSET(_type, _next), \ > >> + .entry = offsetof(_type, _next), \ > >> + .size = sizeof(_type), \ > >> + .vmsd = &(_vmsd), \ > >> +} > > > > .last and .prev are unnecessary, since they come just after .first and > > .next (and their use is hidden behind QTAILQ_RAW_*). .first and .next ^^^^^^^^^^^^^^^^ Actually, .first and .next are always 0. I misread your changes to qemu/queue.h. See below. > > can be placed in .offset and .num_offset respectively. So you don't > > really need an extra metadata struct. > > > > If you prefer you could have something like > > > > union { > > size_t num_offset; /* VMS_VARRAY */ > > size_t size_offset; /* VMS_VBUFFER */ > > size_t next_offset; /* VMS_TAILQ */ > > } offsets; > > Actually I explored the approach in which I use a VMSD to encode all the > information. But a VMSD describes actual memory layout. Interpreting it > another way could be confusing. > > One of the assumption about QTAILQ is that we can only use the > interfaces defined in queue.h to access it. I intend not to depend on > its actually layout. From this perspective, these 6 parameters are > needed. You are already adding QTAILQ_RAW_*, aren't you? Those interfaces would need to know about the layout, and you are passing redundant information: - .next_offset should always be 0 - .prev_offset should always be sizeof(void *) - .first_offset should always be 0 - .last_offset should always be sizeof(void *) so you only need head and entry, which you can store in .offset and .num_offset. The .vmsd field you have in ->metadata can be stored in VMStateField's .vmsd, and likewise for .size (which can be stored in VMStateField's .vmsd). Thanks, Paolo