Re: [Qemu-devel] [PATCH 02/11] Add migrate-set-capabilities and query-migrate-capabilities

[Orit Wasserman <owasserm@redhat.com>]

On 08/06/2012 07:14 PM, Eric Blake wrote:
> On 08/06/2012 10:04 AM, Orit Wasserman wrote:
>> On 08/06/2012 05:26 PM, Eric Blake wrote:
>>> On 08/05/2012 03:13 AM, Orit Wasserman wrote:
>>>> The management can enable/disable a capability for the next migration by using
>>>> migrate-set-apabilities QMP command.
>>>
>>> s/set-apabilities/set-capabilities/
>>>
> 
>>> In HMP, are migrate_supported_capabilities and migrate_capabilities
>>> redundant?  That is, I think I can use either command to answer both
>>> questions "what capabilities exist" and "what is the current state of
>>> all capabilities that exist", since _both_ commands output a list of
>>> capability names as well as an on/off designator.  If my analysis is
>>> right, then we don't need migrate_supported_capabilities.
>> No 'info migrate_supported_capabilities' shows the capabilities this version of QEMU can supports.
>> and 'info migrate_capabilities' show what are the state of capabilities for the migration, i.e what is enabled.
> 
> Let's compare:
> 
> patch 1/11:
> 
> +void hmp_info_migrate_supported_capabilities(Monitor *mon)
> +{
> +    MigrationCapabilityStatusList *caps_list, *cap;
> +
> +    caps_list = qmp_query_migrate_supported_capabilities(NULL);
> +    if (!caps_list) {
> +        monitor_printf(mon, "No supported migration capabilities found\n");
> +        return;
> +    }
> +
> +    for (cap = caps_list; cap; cap = cap->next) {
> +        monitor_printf(mon, "%s: %s ",
> +                       MigrationCapability_lookup[cap->value->capability],
> +                       cap->value->state ? "on" : "off");
> 
> 
> patch 2/11:
> 
> +void hmp_info_migrate_capabilities(Monitor *mon)
> +{
> +    MigrationCapabilityStatusList *caps, *cap;
> +
> +    caps = qmp_query_migrate_capabilities(NULL);
> +
> +    if (caps) {
> +        monitor_printf(mon, "capabilities: ");
> +        for (cap = caps; cap; cap = cap->next) {
> +            monitor_printf(mon, "%s: %s ",
> +
> MigrationCapability_lookup[cap->value->capability],
> +                           cap->value->state ? "on" : "off");
> 
> That is, BOTH commands end up iterating over a list of caps, and output
> identical information in the case where caps exist of 'name: state' for
> each capability.
> 
> They really ARE redundant - both commands are telling me:
> 
> capabilities:
> xbzrle: on
> foobar: off
> 
> which I can read to answer both my question of 'what is supported'
> (xbzrle and foobar) and 'what is enabled' (xbzrle).  I see no need to
> have to commands to tell me the same information, so I'd prefer the
> shorter name.
> 
The information is different:
the first:
MigrationCapabilityStatusList *
qmp_query_migrate_supported_capabilities(Error **errp)
{
    MigrationCapabilityStatusList *caps_list = g_malloc0(sizeof(*caps_list));

    caps_list->value = g_malloc(sizeof(*caps_list->value));
    caps_list->value->capability = MIGRATION_CAPABILITY_XBZRLE;
    caps_list->value->state = true;
    caps_list->next = NULL;

    return caps_list;
}

the second:
MigrationCapabilityStatusList *
qmp_query_migrate_supported_capabilities(Error **errp)
{
    MigrationCapabilityStatusList *caps_list = g_malloc0(sizeof(*caps_list));

    caps_list->value = g_malloc(sizeof(*caps_list->value));
    caps_list->value->capability = MIGRATION_CAPABILITY_XBZRLE;
    caps_list->value->state = true;
    caps_list->next = NULL;

    return caps_list;
}

you can look at it as 64bit support one is to know if the processor supports 64 bit
the other to know if the OS uses 64 bit

Orit