On 10/05/2016 03:12 AM, Dr. David Alan Gilbert wrote:
> * Jianjun Duan (du...@linux.vnet.ibm.com) wrote:
>> In QOM(QEMU Object Model) migrated objects are identified with instance_id
>> which is calculated automatically using their path in the QOM composition
>> tree. For some objects, this path could change from source to target in
>> migration. To migrate such objects, we need to make sure the instance_id does
>> not change from source to target. We add a hook in DeviceClass to do
>> customized
>> instance_id calculation in such cases.
>
> Can you explain a bit about why the path changes from source to destination;
> the path here should be a feature of the guest state not the host, and so I
> don't understand why it changes.
>
>> As a result, in these cases compat will not be set in the concerned
>> SaveStateEntry. This will prevent the inconsistent idstr to be sent over in
>> migration. We could have set alias_id in a similar way. But that will be
>> overloading the purpose of alias_id.
>>
>> The first application will be setting instance_id for DRC using its unique
>> index. Doing this makes the instance_id of DRC to be consistent across
>> migration
>> and supports flexible management of DRC objects in migration.
>
> Is there a reason to use a custom instance_id rather than a custom idstr?
>
>>
>> Signed-off-by: Jianjun Duan <du...@linux.vnet.ibm.com>
>> ---
>> include/hw/qdev-core.h | 6 ++++++
>> migration/savevm.c | 20 ++++++++++++++++++--
>> 2 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 2c97347..a012e8e 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -139,6 +139,12 @@ typedef struct DeviceClass {
>> qdev_initfn init; /* TODO remove, once users are converted to realize */
>> qdev_event exit; /* TODO remove, once users are converted to unrealize
>> */
>> const char *bus_type;
>> +
>> + /* When this field is set, qemu will use it to get an unique instance_id
>> + * instead of calculating an auto idstr and instanc_id for the relevant
>> + * SaveStateEntry
>> + */
>> + int (*dev_get_instance_id)(DeviceState *dev);
>> } DeviceClass;
>>
>> typedef struct NamedGPIOList NamedGPIOList;
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 33a2911..ef5c3d1 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -495,6 +495,11 @@ int register_savevm_live(DeviceState *dev,
>> void *opaque)
>> {
>> SaveStateEntry *se;
>> + /* when it is a device and it provides a way to get instance_id,
>> + * we will use it and skip setting idstr and compat.
>> + */
>> + bool flag = (dev != NULL) &&
>> + (DEVICE_GET_CLASS(dev)->dev_get_instance_id != NULL);
>
> We must be able to get a more descriptive name than 'flag'; how about
> 'has_custom_id'
Sure. Will wait for more comments and address them together.
>> se = g_new0(SaveStateEntry, 1);
>> se->version_id = version_id;
>> @@ -507,7 +512,7 @@ int register_savevm_live(DeviceState *dev,
>> se->is_ram = 1;
>> }
>>
>> - if (dev) {
>> + if (dev && !flag) {
>> char *id = qdev_get_dev_path(dev);
>> if (id) {
>> pstrcpy(se->idstr, sizeof(se->idstr), id);
>> @@ -523,6 +528,9 @@ int register_savevm_live(DeviceState *dev,
>> }
>> pstrcat(se->idstr, sizeof(se->idstr), idstr);
>>
>> + if (flag) {
>> + instance_id = DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev);
>> + }
>> if (instance_id == -1) {
>> se->instance_id = calculate_new_instance_id(se->idstr);
>> } else {
>> @@ -580,6 +588,11 @@ int vmstate_register_with_alias_id(DeviceState *dev,
>> int instance_id,
>> int required_for_version)
>> {
>> SaveStateEntry *se;
>> + /* when it is a device and it provides a way to get instance_id,
>> + * we will use it and skip setting idstr and compat.
>> + */
>> + bool flag = (dev != NULL) &&
>> + (DEVICE_GET_CLASS(dev)->dev_get_instance_id != NULL);
>>
>> /* If this triggers, alias support can be dropped for the vmsd. */
>> assert(alias_id == -1 || required_for_version >=
>> vmsd->minimum_version_id);
>> @@ -591,7 +604,7 @@ int vmstate_register_with_alias_id(DeviceState *dev, int
>> instance_id,
>> se->vmsd = vmsd;
>> se->alias_id = alias_id;
>>
>> - if (dev) {
>> + if (dev && !flag) {
>> char *id = qdev_get_dev_path(dev);
>> if (id) {
>> pstrcpy(se->idstr, sizeof(se->idstr), id);
>> @@ -607,6 +620,9 @@ int vmstate_register_with_alias_id(DeviceState *dev, int
>> instance_id,
>> }
>> pstrcat(se->idstr, sizeof(se->idstr), vmsd->name);
>>
>> + if (flag) {
>> + instance_id = DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev);
>> + }
>> if (instance_id == -1) {
>> se->instance_id = calculate_new_instance_id(se->idstr);
>> } else {
>> --
>> 1.9.1
>
> Dave
>>
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>
Thanks,
Jianjun
