Re: [Qemu-devel] [PATCH v2 4/9] migration: Switch to using announce timer

[Michael S. Tsirkin]

On Wed, Jan 30, 2019 at 10:47:17AM +0000, Daniel P. Berrangé wrote:
> On Wed, Jan 30, 2019 at 10:32:31AM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > Switch the announcements to using the new announce timer.
> > Move the code that does it to announce.c rather than savevm
> > because it really has nothing to do with the actual migration.
> > 
> > Migration starts the announce from bh's and so they're all
> > in the main thread/bql, and so there's never any racing with
> > the timers themselves.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  include/migration/misc.h | 10 ------
> >  include/net/announce.h   |  2 ++
> >  include/sysemu/sysemu.h  |  2 --
> >  migration/migration.c    |  2 +-
> >  migration/migration.h    |  4 +++
> >  migration/savevm.c       | 72 ++--------------------------------------
> >  migration/trace-events   |  1 -
> >  net/announce.c           | 67 +++++++++++++++++++++++++++++++++++++
> >  net/trace-events         |  4 +++
> >  9 files changed, 81 insertions(+), 83 deletions(-)
> 
> 
> > +#ifndef ETH_P_RARP
> > +#define ETH_P_RARP 0x8035
> > +#endif
> > +#define ARP_HTYPE_ETH 0x0001
> > +#define ARP_PTYPE_IP 0x0800
> > +#define ARP_OP_REQUEST_REV 0x3
> > +
> > +static int announce_self_create(uint8_t *buf,
> > +                                uint8_t *mac_addr)
> > +{
> > +    /* Ethernet header. */
> > +    memset(buf, 0xff, 6);         /* destination MAC addr */
> > +    memcpy(buf + 6, mac_addr, 6); /* source MAC addr */
> > +    *(uint16_t *)(buf + 12) = htons(ETH_P_RARP); /* ethertype */
> > +
> > +    /* RARP header. */
> > +    *(uint16_t *)(buf + 14) = htons(ARP_HTYPE_ETH); /* hardware addr space 
> > */
> > +    *(uint16_t *)(buf + 16) = htons(ARP_PTYPE_IP); /* protocol addr space 
> > */
> > +    *(buf + 18) = 6; /* hardware addr length (ethernet) */
> > +    *(buf + 19) = 4; /* protocol addr length (IPv4) */
> > +    *(uint16_t *)(buf + 20) = htons(ARP_OP_REQUEST_REV); /* opcode */
> > +    memcpy(buf + 22, mac_addr, 6); /* source hw addr */
> > +    memset(buf + 28, 0x00, 4);     /* source protocol addr */
> > +    memcpy(buf + 32, mac_addr, 6); /* target hw addr */
> > +    memset(buf + 38, 0x00, 4);     /* target protocol addr */
> > +
> > +    /* Padding to get up to 60 bytes (ethernet min packet size, minus 
> > FCS). */
> > +    memset(buf + 42, 0x00, 18);
> > +
> > +    return 60; /* len (FCS will be added by hardware) */
> > +}
> 
> I think I would suggest defining a packed struct for the ethernet
> packet we're sending, so we can just reference named fields instead
> of magic offsets. Then I realized this patch is just moving some
> pre-existing code, so it is probably better to leave it as it is
> in this patch, as mixing code movement with refactoring is bad
> practice. If anyone fancies doing a latter patch to use a struct
> though, it might be nice.

Won't work - this need to be network endian and endian-ness is all
messed up with packed structs.

The place to fix this would be in the C standard :)

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|