| Message ID | 20230529075956.2235870-1-lizhijian@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/clx: Fix CFMW config memory leak | expand |
On Mon, 29 May 2023 15:59:56 +0800 Li Zhijian <lizhijian@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: 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 <lizhijian@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] > + g_free(fw->targets[i]); > + } > + g_free(fw->targets); > } > > void cxl_fmws_link_targets(CXLState *cxl_state, Error **errp)
On 30/05/2023 23:14, Jonathan Cameron via wrote: > On Mon, 29 May 2023 15:59:56 +0800 > Li Zhijian <lizhijian@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 <lizhijian@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) > >
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++) { + g_free(fw->targets[i]); + } + g_free(fw->targets); } void cxl_fmws_link_targets(CXLState *cxl_state, Error **errp)
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> --- hw/cxl/cxl-host.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)