From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60929) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c3q17-0005qn-PN for qemu-devel@nongnu.org; Mon, 07 Nov 2016 14:55:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c3q13-0008Bt-Tl for qemu-devel@nongnu.org; Mon, 07 Nov 2016 14:55:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58420) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c3q13-0008AX-MT for qemu-devel@nongnu.org; Mon, 07 Nov 2016 14:55:37 -0500 Date: Mon, 7 Nov 2016 19:55:32 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20161107195532.GB1155@work-vm> References: <20161027153217.16984-1-dgilbert@redhat.com> <20161027153217.16984-8-dgilbert@redhat.com> <20161030144712.GE3671@var.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161030144712.GE3671@var.home> Subject: Re: [Qemu-devel] [PATCH 7/8] slirp: VMStatify socket level List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Samuel Thibault Cc: qemu-devel@nongnu.org, quintela@redhat.com, amit.shah@redhat.com, jan.kiszka@siemens.com, duanj@linux.vnet.ibm.com * Samuel Thibault (samuel.thibault@gnu.org) wrote: > Hello, > > Dr. David Alan Gilbert (git), on Thu 27 Oct 2016 16:32:16 +0100, wrote: > > - case AF_INET: > > - qemu_put_be32(f, so->so_faddr.s_addr); > > - qemu_put_be16(f, so->so_fport); > > - break; > > > + if (version_id >= 4 && !is_inet) { > > + error_report("%s: so_ffamily unknown, socket not preserved", __func__); > > } > > Well, no, we need to settle this another way, because we want to be able > to easily add inet6 support here. At least pave the way in a way that > makes it not unnecessarily hard. The code you are adding here looks to > me like very hard to rework to make it support the various socket > families. Well, I was just trying to match the current semantics/errors there. > > + VMSTATE_UINT16_V(so_ffamily, struct socket, 4), > > + VMSTATE_UINT32_TEST(so_faddr.s_addr, struct socket, > > + slirp_v4_or_newer_ffamily_inet), > > + VMSTATE_UINT16_TEST(so_fport, struct socket, > > + slirp_v4_or_newer_ffamily_inet), > > Does VMStat not provide a way to have differing content depending on a > field? (here, so_ffamily) It has two things: a) Versions (e.g. VMSTATE_UINT16_V) - that says only include this field if we're on version >= n b) Tests (.e.g. VMSTATE_UINT16_TEST) - that says only include the field if the test says so. The tests are a bit tedious since they're each a function, their is no generic test description scheme. (A lambda here would be lovely, but making one to work on both Gcc and Clang and anything else isn't trivial; I looked) I think the underlying macros would make it easy to do a VMSTATE_UINT16_TEST_V that only happens if both the test and the check are true which might make this easier. It's not that hard, but a bit tedious if we wanted to add another family here; we'd end up with: VMSTATE_UINT16_V(so_ffamily, struct socket, 4), VMSTATE_UINT32_TEST(so_faddr.s_addr, struct socket, slirp_v4_or_newer_ffamily_inet), VMSTATE_UINT32_TEST(so_faddr.s_addr6, struct socket, slirp_v4_or_newer_ffamily_inet6), VMSTATE_UINT16_TEST(so_fport, struct socket, slirp_v4_or_newer_ffamily_inet), and change the test to: static bool slirp_v4_or_newer_ffamily_inet(void *opaque, int version_id) { bool is_inet = ((struct socket *)opaque)->so_ffamily == AF_INET; bool is_inet6 = ((struct socket *)opaque)->so_ffamily == AF_INET6; if (version_id >= 4 && !is_inet && !is_inet6) { error_report("%s: so_ffamily unknown, socket not preserved", __func__); } return version_id >= 4 && is_inet; } static bool slirp_v4_or_newer_ffamily_inet6(void *opaque, int version_id) { bool is_inet6 = ((struct socket *)opaque)->so_ffamily == AF_INET6; return version_id >= 4 && is_inet6; } The asymmetry is purely to generate the error. Actually, I might be able to avoid some of the lfamily/ffamily duplication here - I hadn't spotted the were macros for lhost.ss.ss_family, so I think I can rework the v4 or newer by doing: a) Split the fhost and lhost union's out as a separate union and share them. b) For the v4 entries do: VMSTATE_STRUCT(fhost, struct socket, 4, vmstate_slirp_hosts); VMSTATE_STRUCT(lhost, struct socket, 4, vmstate_slirp_hosts); and define vmstate_slirp_hosts to have the logic to check the family; although I've never tried VMSTATE_STRUCT on a union I think it'll work (!). Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK