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

[Markus Armbruster <armbru@redhat.com>]

Markus Armbruster <armbru@redhat.com> writes:

> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>
>> * Germano Veit Michel (germano@redhat.com) 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 <germano@redhat.com>
>>> ---
>>>  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:

Missing the obvious: should test both the success and the error case!

[...]