Patchwork [RfC,1/3] net: macaddr tweaks.

Add new type for mac addresses.

Add function which sets the qemu default mac address if it finds the mac
address uninitialized (i.e. all zeros).
---
 net.c |   14 ++++++++++++++
 net.h |    2 ++
 2 files changed, 16 insertions(+), 0 deletions(-)

Gerd Hoffmann <kraxel@redhat.com> writes:

> Add new type for mac addresses.
>
> Add function which sets the qemu default mac address if it finds the mac
> address uninitialized (i.e. all zeros).
> ---
>  net.c |   14 ++++++++++++++
>  net.h |    2 ++
>  2 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/net.c b/net.c
> index 3fdf1e6..c6eb93c 100644
> --- a/net.c
> +++ b/net.c
> @@ -280,6 +280,20 @@ void qemu_format_nic_info_str(VLANClientState *vc, uint8_t macaddr[6])
>               macaddr[3], macaddr[4], macaddr[5]);
>  }
>  
> +void qemu_macaddr_default_if_unset(macaddr_t macaddr)
> +{
> +    macaddr_t zero = { 0,0,0,0,0,0 };
> +
> +    if (memcmp(macaddr, zero, sizeof(zero)) != 0)
> +        return;
> +    macaddr[0] = 0x52;
> +    macaddr[1] = 0x54;
> +    macaddr[2] = 0x00;
> +    macaddr[3] = 0x12;
> +    macaddr[4] = 0x34;
> +    macaddr[5] = 0x56;
> +}
> +

This will get us the same default MAC address for all NICs, won't it?
The old code provides a different default for each NIC.

Simply increment the default whenever it is used?

>  static char *assign_name(VLANClientState *vc1, const char *model)
>  {
>      VLANState *vlan;
> diff --git a/net.h b/net.h
> index 1479826..50630a1 100644
> --- a/net.h
> +++ b/net.h
> @@ -7,6 +7,7 @@
>  
>  /* VLANs support */
>  
> +typedef uint8_t macaddr_t[6];

Reserved identifier (any POSIX header).  Do we care?

>  typedef struct VLANClientState VLANClientState;
>  
>  typedef int (NetCanReceive)(VLANClientState *);
> @@ -75,6 +76,7 @@ ssize_t qemu_send_packet_async(VLANClientState *vc, const uint8_t *buf,
>  void qemu_purge_queued_packets(VLANClientState *vc);
>  void qemu_flush_queued_packets(VLANClientState *vc);
>  void qemu_format_nic_info_str(VLANClientState *vc, uint8_t macaddr[6]);
> +void qemu_macaddr_default_if_unset(macaddr_t macaddr);
>  void qemu_check_nic_model(NICInfo *nd, const char *model);
>  void qemu_check_nic_model_list(NICInfo *nd, const char * const *models,
>                                 const char *default_model);

>> +void qemu_macaddr_default_if_unset(macaddr_t macaddr)
>> +{
>> +    macaddr_t zero = { 0,0,0,0,0,0 };
>> +
>> +    if (memcmp(macaddr, zero, sizeof(zero)) != 0)
>> +        return;
>> +    macaddr[0] = 0x52;
>> +    macaddr[1] = 0x54;
>> +    macaddr[2] = 0x00;
>> +    macaddr[3] = 0x12;
>> +    macaddr[4] = 0x34;
>> +    macaddr[5] = 0x56;
>> +}
>> +
>
> This will get us the same default MAC address for all NICs, won't it?
> The old code provides a different default for each NIC.
>
> Simply increment the default whenever it is used?

Would be an option.  Doesn't fully match the old behavior though.

Old behavior is to use 0x56 + nic index (i.e. nd_table index) for the 
last byte, so nic #1 allways has 0x57, no matter whenever for nic #0 a 
default was specified or not.

The problem we have here is that the nics created via -device don't have 
a nd_table entry and thus no nic index.  We have a related issue with 
pxe boot (-boot n,o,p,q == boot first,second,third,fourth nic).

Didn't have a good idea (yet) how to address that.

>> +typedef uint8_t macaddr_t[6];
>
> Reserved identifier (any POSIX header).  Do we care?

We should prrobably qemu-ify it to something like MACAddr anyway.

cheers,
   Gerd

> +void qemu_macaddr_default_if_unset(macaddr_t macaddr)
> +{
> +    macaddr_t zero = { 0,0,0,0,0,0 };

Very minor since this is not at all a hot path, but please make it just

   static macaddr_t zero;

since otherwise it would be initialized on the stack at run time.

Paolo

On Wed, Sep 30, 2009 at 2:57 PM, Paolo Bonzini <bonzini@gnu.org> wrote:
>
>> +void qemu_macaddr_default_if_unset(macaddr_t macaddr)
>> +{
>> +    macaddr_t zero = { 0,0,0,0,0,0 };
>
> Very minor since this is not at all a hot path, but please make it just
>
>  static macaddr_t zero;
>
> since otherwise it would be initialized on the stack at run time.

Even better, static const.