Re: [Qemu-devel] [PATCH] slirp: Use lduw_be_p in slirp_input

[Marc-André Lureau]

Hi

On Thu, Jan 17, 2019 at 5:56 PM Peter Maydell <address@hidden> wrote:
>
> On Thu, 17 Jan 2019 at 13:41, Philippe Mathieu-Daudé <address@hidden> wrote:
> > On 1/17/19 12:50 AM, Samuel Thibault wrote:
> > > --- a/slirp/slirp.c
> > > +++ b/slirp/slirp.c
> > > @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, 
> > > int pkt_len)
> > >      if (pkt_len < ETH_HLEN)
> > >          return;
> > >
> > > -    proto = ntohs(*(uint16_t *)(pkt + 12));
> > > +    proto = (((uint16_t) pkt[12]) << 8) + pkt[13];
> > >      switch(proto) {
> > >      case ETH_P_ARP:
> > >          arp_input(slirp, pkt, pkt_len);
> >
> > What about using memcpy?
>
> We should use whatever the new libslirp wants to consistently
> use as its mechanism for loading unaligned data. I don't
> suppose this is the only place where it ever needs to do this.
>
> Personally I would vote for having libslirp have versions of
> the ld*_p functions, because they solve the problem in a
> clear and correct way. But that's up to Marc-André really.

I think I would go with a copy of qemu bswap.h, unless there is an
equivalent in glib (I don't think so) or gnulib? Or other standard
compiler solution.

It's somehow surprising me that there is no goto solution :).

> (As you can see from clang build logs:
> https://travis-ci.org/qemu/qemu/jobs/480813811
> slirp/ has a lot of as-yet-unfixed "takes address of packed
> member" bugs, which suggest it's a bit slapdash with
> alignment. Running it under the clang sanitizer to look
> for runtime alignment errors might also be interesting.)
>
> thanks
> -- PMM



-- 
Marc-André Lureau