* [PATCH v2] hw/cxl: Fix CFMW config memory leak
@ 2023-05-31 2:34 Li Zhijian
2023-05-31 5:45 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 3+ messages in thread
From: Li Zhijian @ 2023-05-31 2:34 UTC (permalink / raw)
To: jonathan.cameron, fan.ni; +Cc: qemu-devel, Li Zhijian
Only 'fw' pointer is marked as g_autofree, so we shoud free other
resource manually in error path.
Signed-off-by: Li Zhijian <lizhijian@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);
}
void cxl_fmws_link_targets(CXLState *cxl_state, Error **errp)
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] hw/cxl: Fix CFMW config memory leak
2023-05-31 2:34 [PATCH v2] hw/cxl: Fix CFMW config memory leak Li Zhijian
@ 2023-05-31 5:45 ` Philippe Mathieu-Daudé
2023-05-31 6:08 ` Zhijian Li (Fujitsu)
0 siblings, 1 reply; 3+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-05-31 5:45 UTC (permalink / raw)
To: Li Zhijian, jonathan.cameron, fan.ni; +Cc: qemu-devel
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 <lizhijian@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?
-- >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));
---
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] hw/cxl: Fix CFMW config memory leak
2023-05-31 5:45 ` Philippe Mathieu-Daudé
@ 2023-05-31 6:08 ` Zhijian Li (Fujitsu)
0 siblings, 0 replies; 3+ messages in thread
From: Zhijian Li (Fujitsu) @ 2023-05-31 6:08 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Zhijian Li (Fujitsu),
jonathan.cameron@huawei.com, fan.ni@samsung.com
Cc: qemu-devel@nongnu.org
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 <lizhijian@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));
>
> ---
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-31 6:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-31 2:34 [PATCH v2] hw/cxl: Fix CFMW config memory leak Li Zhijian
2023-05-31 5:45 ` Philippe Mathieu-Daudé
2023-05-31 6:08 ` Zhijian Li (Fujitsu)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).