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