[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 06/12] qmp: Expose qemu_announce_self as a qmp c
From: |
Vlad Yasevich |
Subject: |
Re: [Qemu-devel] [PATCH 06/12] qmp: Expose qemu_announce_self as a qmp command |
Date: |
Tue, 30 May 2017 11:01:08 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
On 05/30/2017 10:14 AM, Daniel P. Berrange wrote:
> On Wed, May 24, 2017 at 02:05:22PM -0400, Vladislav Yasevich wrote:
>> Add a qmp command that can trigger guest announcements.
>>
>> Based on work of Germano Veit Michel <address@hidden>
>>
>> Signed-off-by: Vladislav Yasevich <address@hidden>
>> ---
>> migration/savevm.c | 14 ++++++++++++++
>> qapi-schema.json | 19 +++++++++++++++++++
>> 2 files changed, 33 insertions(+)
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index a4097c9..b55ce6a 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -265,6 +265,20 @@ void qemu_announce_self(AnnounceParameters *params)
>> qemu_announce_self_once(timer);
>> }
>>
>> +void qmp_announce_self(bool has_params, AnnounceParameters *params,
>> + Error **errp)
>> +{
>> + AnnounceParameters announce_params;
>> +
>> + memcpy(&announce_params, qemu_get_announce_params(),
>> + sizeof(announce_params));
>> +
>> + if (has_params)
>> + qemu_set_announce_parameters(&announce_params, params);
>> +
>> + qemu_announce_self(&announce_params);
>> +}
>
> I'm not a huge fan of the idea of setting announce parameters in a
> global namespace, via a previously issued separate command.
>
> I realize this is already done for the 'migrate' command, but there
> it has been the cause of a number of bugs in libvirt mgmt of QEMU.
> At least with migrate it makes sense for some parameters which need
> to be tuned 'on the fly' after migrate already started. That doesn't
> apply to this command though.
>
> So I tend to think it'd be nicer to just make the parameters mandatory
> for this command, so it is self-contanied does and not rely on previously
> set (or potentially unknown/indeterminate) global state.
>
The reason I didn't do this is that I didn't want to require the user
to know or even think about the timeout values. I did want to provide
an option though for those who may want to tweak things. In most circumstances,
the timeout values would never really be touched and we'll just use the
qemu defaults. in rare circumstances, values might need to be changed
during migration. Here, I can definitely see the need to give migration
it's own values.
> I'd also suggest that instad of adding an 'announce_set_parameters',
> the parameters for the automatic announce at end of migration should
> be set via the 'migrate_set_parameters' command.
I can do that.
>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 2030087..126b09d 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -654,6 +654,25 @@
>> 'returns': 'AnnounceParameters' }
>>
>> ##
>> +# @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.
>
> "None", but it has some arguments listed right below....
will fix
Thanks
-vlad
>
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "announce-self"
>> +# "arguments": { "announce-rounds": 5 } }
>> +# <- { "return": {} }
>> +#
>> +# Since: 2.10
>> +##
>> +{ 'command': 'announce-self',
>> + 'data' : {'*params': 'AnnounceParameters'} }
>> +
>> +##
>> # @MigrationStats:
>> #
>> # Detailed migration status.
>> --
>> 2.7.4
>>
>>
>
> Regards,
> Daniel
>
[Qemu-devel] [PATCH 07/12] migration: Allow for a limited number of announce timers, Vladislav Yasevich, 2017/05/24
[Qemu-devel] [PATCH 08/12] announce_timer: Add ability to reset an existing, Vladislav Yasevich, 2017/05/24
[Qemu-devel] [PATCH 10/12] hmp: Add hmp_announce_self, Vladislav Yasevich, 2017/05/24
[Qemu-devel] [PATCH 09/12] hmp: add announce paraters info/set, Vladislav Yasevich, 2017/05/24