On 30/05/2023 23:14, Jonathan Cameron via wrote:
> On Mon, 29 May 2023 15:59:56 +0800
> Li Zhijian <lizhij...@cn.fujitsu.com> wrote:
>
>> Only 'fw' pointer is marked as g_autofree, so we shoud free other
>> resource manually in error path.
>>
> Good spot.
>
> Patch title typo
> hw/cxl:>
OMG, it was the 2nd time i have this typo. i hate my stupid finger.
> It's a bit annoying we can't handle this with more autofree magic.
> That would work for fw->targets but not for the string duplicates.
>
> One other comment inline,
>
> Thanks,
>
> Jonathan
>
>
>> Signed-off-by: Li Zhijian <lizhij...@cn.fujitsu.com>
>> ---
>> hw/cxl/cxl-host.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
>> index 034c7805b3e..dd1a7c83f71 100644
>> --- a/hw/cxl/cxl-host.c
>> +++ b/hw/cxl/cxl-host.c
>> @@ -48,7 +48,7 @@ static void cxl_fixed_memory_window_config(CXLState
>> *cxl_state,
>> if (object->size % (256 * MiB)) {
>> error_setg(errp,
>> "Size of a CXL fixed memory window must be a multiple
>> of 256MiB");
>> - return;
>> + goto err_free;
>
>
>> }
>> fw->size = object->size;
>>
>> @@ -57,7 +57,7 @@ static void cxl_fixed_memory_window_config(CXLState
>> *cxl_state,
>> cxl_interleave_granularity_enc(object->interleave_granularity,
>> errp);
>> if (*errp) {
>> - return;
>> + goto err_free;
>> }
>> } else {
>> /* Default to 256 byte interleave */
>> @@ -68,6 +68,12 @@ static void cxl_fixed_memory_window_config(CXLState
>> *cxl_state,
>> g_steal_pointer(&fw));
>>
>> return;
>> +
>> +err_free:
>> + for (i = 0; i < fw->num_targets && fw->targets[i]; i++) {
>
> No need to check fw->targets[i]
I knew it's safe to free a NULL pointer. The initial idea was just to make the
loop end as soon as possible :)
Of course, I would also be happy to delete this.
Thanks
Zhijian
>
>> + g_free(fw->targets[i]);
>> + }
>> + g_free(fw->targets);
>> }
>>
>> void cxl_fmws_link_targets(CXLState *cxl_state, Error **errp)
>
>
