[1/9] hw/i386/sgx: Do not open-code qdev_realize_and_unref()

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/sgx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/i386/sgx.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> index db004d17a6..5ddc5d7ea2 100644
> --- a/hw/i386/sgx.c
> +++ b/hw/i386/sgx.c
> @@ -299,7 +299,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>                                  &sgx_epc->mr);
>  
>      for (list = x86ms->sgx_epc_list; list; list = list->next) {
> -        obj = object_new("sgx-epc");
> +        obj = object_new(TYPE_SGX_EPC);

I wonder why this doesn't use qdev_new().
>  
>          /* set the memdev link with memory backend */
>          object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
> @@ -307,8 +307,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>          /* set the numa node property for sgx epc object */
>          object_property_set_uint(obj, SGX_EPC_NUMA_NODE_PROP, list->value->node,
>                               &error_fatal);
> -        object_property_set_bool(obj, "realized", true, &error_fatal);
> -        object_unref(obj);
> +        qdev_realize_and_unref(DEVICE(obj), NULL, &error_fatal);

Yes, please!  Must have crept in after I converted all realizations.
I can see two more:

hw/pci/pcie_sriov.c:        object_property_set_bool(OBJECT(vf), "realized", false, &local_err);
linux-user/syscall.c:            object_property_set_bool(OBJECT(cpu), "realized", false, NULL);

>      }
>  
>      if ((sgx_epc->base + sgx_epc->size) < sgx_epc->base) {

Reviewed-by: Markus Armbruster <armbru@redhat.com>

On 3/2/23 13:32, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/i386/sgx.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
>> index db004d17a6..5ddc5d7ea2 100644
>> --- a/hw/i386/sgx.c
>> +++ b/hw/i386/sgx.c
>> @@ -299,7 +299,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>>                                   &sgx_epc->mr);
>>   
>>       for (list = x86ms->sgx_epc_list; list; list = list->next) {
>> -        obj = object_new("sgx-epc");
>> +        obj = object_new(TYPE_SGX_EPC);
> 
> I wonder why this doesn't use qdev_new().

OK.

>>           /* set the memdev link with memory backend */
>>           object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
>> @@ -307,8 +307,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>>           /* set the numa node property for sgx epc object */
>>           object_property_set_uint(obj, SGX_EPC_NUMA_NODE_PROP, list->value->node,
>>                                &error_fatal);
>> -        object_property_set_bool(obj, "realized", true, &error_fatal);
>> -        object_unref(obj);
>> +        qdev_realize_and_unref(DEVICE(obj), NULL, &error_fatal);
> 
> Yes, please!  Must have crept in after I converted all realizations.
> I can see two more:
> 
> hw/pci/pcie_sriov.c:        object_property_set_bool(OBJECT(vf), "realized", false, &local_err);
> linux-user/syscall.c:            object_property_set_bool(OBJECT(cpu), "realized", false, NULL);

No: these would be qdev_UNrealize_and_unref(). Do we want it?
Maybe to avoid open-coding it, yes?

>>       }
>>   
>>       if ((sgx_epc->base + sgx_epc->size) < sgx_epc->base) {
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 3/2/23 13:32, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/i386/sgx.c | 5 ++---
>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
>>> index db004d17a6..5ddc5d7ea2 100644
>>> --- a/hw/i386/sgx.c
>>> +++ b/hw/i386/sgx.c
>>> @@ -299,7 +299,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>>>                                   &sgx_epc->mr);
>>>   
>>>       for (list = x86ms->sgx_epc_list; list; list = list->next) {
>>> -        obj = object_new("sgx-epc");
>>> +        obj = object_new(TYPE_SGX_EPC);
>> 
>> I wonder why this doesn't use qdev_new().
>
> OK.

Observation, not demand!

>>>           /* set the memdev link with memory backend */
>>>           object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
>>> @@ -307,8 +307,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>>>           /* set the numa node property for sgx epc object */
>>>           object_property_set_uint(obj, SGX_EPC_NUMA_NODE_PROP, list->value->node,
>>>                                &error_fatal);
>>> -        object_property_set_bool(obj, "realized", true, &error_fatal);
>>> -        object_unref(obj);
>>> +        qdev_realize_and_unref(DEVICE(obj), NULL, &error_fatal);
>> 
>> Yes, please!  Must have crept in after I converted all realizations.
>> I can see two more:
>> 
>> hw/pci/pcie_sriov.c:        object_property_set_bool(OBJECT(vf), "realized", false, &local_err);
>> linux-user/syscall.c:            object_property_set_bool(OBJECT(cpu), "realized", false, NULL);
>
> No: these would be qdev_UNrealize_and_unref().

You're right.

>                                                Do we want it?
> Maybe to avoid open-coding it, yes?

I think so.  See

    dc3edf8d8a qdev: Convert to qdev_unrealize() manually
    981c3dcd94 qdev: Convert to qdev_unrealize() with Coccinelle

>>>       }
>>>   
>>>       if ((sgx_epc->base + sgx_epc->size) < sgx_epc->base) {
>> 
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Thanks!