On 31/05/2023 13:45, Philippe Mathieu-Daudé wrote:
> Hi Li,
>
> On 31/5/23 04:34, Li Zhijian wrote:
>> Only 'fw' pointer is marked as g_autofree, so we shoud free other
>> resource manually in error path.
>>
>> Signed-off-by: Li Zhijian <lizhij...@cn.fujitsu.com>
>> ---
>> V2: Delete unnecesarry check
>> ---
>> 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..787a2e779d2 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; i++) {
>> + g_free(fw->targets[i]);
>> + }
>> + g_free(fw->targets);
>> }
>
> IIUC we don't need targets[] before checking errors. What about simply
> allocate when we can't fail?
Yeah, you are right.
i will update the patch as you suggest.
>
> -- >8 --
> diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> index 034c7805b3..f0920da956 100644
> --- a/hw/cxl/cxl-host.c
> +++ b/hw/cxl/cxl-host.c
> @@ -39,12 +39,6 @@ static void cxl_fixed_memory_window_config(CXLState
> *cxl_state,
> return;
> }
>
> - fw->targets = g_malloc0_n(fw->num_targets, sizeof(*fw->targets));
> - for (i = 0, target = object->targets; target; i++, target =
> target->next) {
> - /* This link cannot be resolved yet, so stash the name for now */
> - fw->targets[i] = g_strdup(target->value);
> - }
> -
> if (object->size % (256 * MiB)) {
> error_setg(errp,
> "Size of a CXL fixed memory window must be a multiple of
> 256MiB");
> @@ -64,6 +58,12 @@ static void cxl_fixed_memory_window_config(CXLState
> *cxl_state,
> fw->enc_int_gran = 0;
> }
>
> + fw->targets = g_malloc0_n(fw->num_targets, sizeof(*fw->targets));
> + for (i = 0, target = object->targets; target; i++, target =
> target->next) {
> + /* This link cannot be resolved yet, so stash the name for now */
> + fw->targets[i] = g_strdup(target->value);
> + }
> +
> cxl_state->fixed_windows = g_list_append(cxl_state->fixed_windows,
> g_steal_pointer(&fw));
>
> ---
>
