Re: [Qemu-devel] [PATCH 7/8] slirp: VMStatify socket level

["Dr. David Alan Gilbert" <dgilbert@redhat.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