Re: [Qemu-devel] [PATCH 7/7] net: fix qemu_announce_self()

[Markus Armbruster]

Mark McLoughlin <address@hidden> writes:

> Now that we have a sane way of iterating over NICs.
>
> Signed-off-by: Mark McLoughlin <address@hidden>
> ---
>  savevm.c |   43 ++++++++++++++++++++++++++-----------------
>  1 files changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index 039740c..3736588 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -75,6 +75,7 @@
>  
>  #include "qemu-common.h"
>  #include "hw/hw.h"
> +#include "hw/qdev.h"
>  #include "net.h"
>  #include "monitor.h"
>  #include "sysemu.h"
> @@ -123,28 +124,36 @@ static int announce_self_create(uint8_t *buf,
>      return 60; /* len (FCS will be added by hardware) */
>  }
>  
> -static void qemu_announce_self_once(void *opaque)
> +static void qemu_announce_self_iter(DeviceState *dev, void *opaque)
>  {
> -    int i, len;
> -#ifdef FIXME
> -    VLANState *vlan;
> -    VLANClientState *vc;
> -#endif
> +    VLANClientState *client;
> +    uint8_t *mac;
>      uint8_t buf[60];
> +    int len;
> +
> +    if (!qdev_prop_exists(dev, "net-client")) {
> +        return;
> +    }

This is an ad-hoc "is this a NIC" test.  It assumes all NICs have a
"net-client" property (trivial), and that no other kind of device ever
has a "net-client" property (reasonable).  But, as we accumulate more of
this type of tests, we'll accumulate more of these assumptions, and
having them spread over the code could be confusing.  What about
collecting the "is this a DEVICE_KIND" tests in one place?

For the record, I still believe that identifying device kind by testing
presence of properties is suboptimal[*], but we got more important fish
to fry.

> +
> +    client = qdev_prop_get_net_client(dev, "net-client");
> +    mac = qdev_prop_get_macaddr(dev, "mac");
> +
> +    printf("qemu_announce_self_iter() mac = %p\n", mac);
> +
> +    len = announce_self_create(buf, mac);
> +
> +    printf("sending packet from %s\n", client->name);
> +
> +    qemu_send_packet_raw(client, buf, len);
> +}
[...]

[*] There's no way to test "is this a watchdog" by testing presence of
properties.  Ability to identify watchdog devices would let us kill
watchdog_list.