Re: [Qemu-devel] [PATCH V2] migration: expose qemu_announce_self() via qmp

[Markus Armbruster]

"Dr. David Alan Gilbert" <address@hidden> writes:

> * Germano Veit Michel (address@hidden) wrote:
>> qemu_announce_self() is triggered by qemu at the end of migrations
>> to update the network regarding the path to the guest l2addr.
>> 
>> however it is also useful when there is a network change such as
>> an active bond slave swap. Essentially, it's the same as a migration
>> from a network perspective - the guest moves to a different point
>> in the network topology.
>> 
>> this exposes the function via qmp.
>
> Markus: Since you're asking for tests for qmp commands; how would you
> test this?

Good question, as tests/ isn't exactly full of examples you could crib.

Let me look at the patch...

> Jason: Does this look OK from the networking side of things?
>
>> Signed-off-by: Germano Veit Michel <address@hidden>
>> ---
>>  include/migration/vmstate.h |  5 +++++
>>  migration/savevm.c          | 30 +++++++++++++++++++-----------
>>  qapi-schema.json            | 18 ++++++++++++++++++
>>  3 files changed, 42 insertions(+), 11 deletions(-)
>> 
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 63e7b02..a08715c 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -1042,6 +1042,11 @@ int64_t self_announce_delay(int round)
>>      return 50 + (SELF_ANNOUNCE_ROUNDS - round - 1) * 100;
>>  }
>> 
>> +struct AnnounceRound {
>> +    QEMUTimer *timer;
>> +    int count;
>> +};
>> +
>>  void dump_vmstate_json_to_file(FILE *out_fp);
>> 
>>  #endif
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 5ecd264..44e196b 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -118,29 +118,37 @@ static void qemu_announce_self_iter(NICState
>> *nic, void *opaque)
>>      qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
>>  }
>> 
>> -
>>  static void qemu_announce_self_once(void *opaque)
>>  {
>> -    static int count = SELF_ANNOUNCE_ROUNDS;
>> -    QEMUTimer *timer = *(QEMUTimer **)opaque;
>> +    struct AnnounceRound *round = opaque;
>> 
>>      qemu_foreach_nic(qemu_announce_self_iter, NULL);
>> 
>> -    if (--count) {
>> +    round->count--;
>> +    if (round->count) {
>>          /* delay 50ms, 150ms, 250ms, ... */
>> -        timer_mod(timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
>> -                  self_announce_delay(count));
>> +        timer_mod(round->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
>> +                  self_announce_delay(round->count));
>>      } else {
>> -            timer_del(timer);
>> -            timer_free(timer);
>> +            timer_del(round->timer);
>> +            timer_free(round->timer);
>> +            g_free(round);
>>      }
>>  }
>> 
>>  void qemu_announce_self(void)
>>  {
>> -    static QEMUTimer *timer;
>> -    timer = timer_new_ms(QEMU_CLOCK_REALTIME, qemu_announce_self_once, 
>> &timer);
>> -    qemu_announce_self_once(&timer);
>> +    struct AnnounceRound *round = g_malloc(sizeof(struct AnnounceRound));
>
> I prefer g_new0 - i.e.
>    struct AnnounceRound *round = g_new0(struct AnnounceRound, 1)
>
>> +    if (!round)
>> +        return;
>> +    round->count = SELF_ANNOUNCE_ROUNDS;
>> +    round->timer = timer_new_ms(QEMU_CLOCK_REALTIME,
>> qemu_announce_self_once, round);
>
> An odd line break?
>
>> +    qemu_announce_self_once(round);
>> +}
>> +
>> +void qmp_announce_self(Error **errp)
>> +{
>> +    qemu_announce_self();
>>  }
>> 
>>  /***********************************************************/
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index baa0d26..0d9bffd 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -6080,3 +6080,21 @@
>>  #
>>  ##
>>  { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
>> +
>> +##
>> +# @announce-self:
>> +#
>> +# Trigger generation of broadcast RARP frames to update network switches.
>> +# This can be useful when network bonds fail-over the active slave.
>> +#
>> +# Arguments: None.

Please drop this line.

>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "announce-self" }
>> +# <- { "return": {} }
>> +#
>> +# Since: 2.9
>> +##
>> +{ 'command': 'announce-self' }
>> +

>From QMP's point of view, this command is as simple as they get: no
arguments, no return values, no errors.

I think a basic smoke test would do: try the command, check no magic
smoke comes out.  Untested sketch adapted from qmp-test.c:

    /*
     * Test cases for network-related QMP commands
     *
     * Copyright (c) 2017 Red Hat Inc.
     *
     * Authors:
     *  Markus Armbruster <address@hidden>,
     *
     * This work is licensed under the terms of the GNU GPL, version 2 or later.
     * See the COPYING file in the top-level directory.
     */

    #include "qemu/osdep.h"
    #include "libqtest.h"
    #include "qapi/error.h"

    const char common_args[] = "-nodefaults -machine none";

    static void test_qmp_announce_self(void)
    {
        QDict *resp, *ret;

        qtest_start(common_args);

        resp = qmp("{ 'execute': 'qmp_capabilities' }");
        ret = qdict_get_qdict(resp, "return");
        g_assert(ret && !qdict_size(ret));
        QDECREF(resp);

        qtest_end();
    }

    int main(int argc, char *argv[])
    {
        g_test_init(&argc, &argv, NULL);

        qtest_add_func("qmp/net/announce_self", test_qmp_announce_self);

        return g_test_run();
    }

If you want to go the extra mile: is there a way to set up networking so
you can observe the RARPs this should trigger?

I'd call this qmp-net-test.  Add to Makefile.include exactly like
qmp-test.

Test cases for existing networking-related QMP commands should be added,
but not in this patch, and not necessarily by you.

Alternatively, have a more general test program for networking stuff,
and make this one of its test cases.  Your choice.

Hope this helps!

>
> Please wire hmp up as well (see hmp-commands.hx).
>
> Dave
>
>> -- 
>> 2.9.3
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK