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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Jan 30, 2019 at 10:32:31AM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> 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 <dgilbert@redhat.com>
> ---
>  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.


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 :|