On 2025/09/16 5:30, Peter Xu wrote:
On Sun, Sep 14, 2025 at 06:06:44PM +0900, Akihiko Odaki wrote:
It makes sense to have a through review, but my argument here is the
de-duplication of object_unparent() and the replacement of g_free() with
object_new() are logically distinct and should be split into distinct
patches. Each patch can independently have through review, be
applied/backported, or be reverted in case of regression.
We're discussing a change in the memory.rst on suggested way to use dynamic
MRs, so I think we can do it in one shot rather than making it confusing.
It's not a huge change even in one go.
> > It's fine. You're right we can remove the object_unparent() first when
it's always a no-op. We'll update the doc twice, though I assume it's fine.
If you would, please consider sending this part as a separate series.
The subject should be something like "remove unnecessary object_unparent()
for dynamic MRs" or something like that. It has nothing to do with
memleaks on this part.
I think "Do not unparent in instance_finalize()" is good enough.
"unnecessary" sounds it is correct but only extraneous; in reality it is
semantically problematic.
"In instance_finalize()" is more appropriate to represent the scope of
the change than "for dynamic MRs". Unparenting objects when finalizing
their parents is wrong, whether they are MRs or not. On the other hand,
unparenting MRs before instance_finalize() is still allowed.
In v2, I dropped patches to fix memory leaks so the series only contains
ones to fix use-after-finalization. I'll rename the series accordingly.
Please cover tests as much as possible and if we touch the doc we need to
convert everything that uses dynamic MRs, including the missing ones in
VFIO, and also the rest occurances.
I'll search for object_unparent() called from instance_finalize().
Regards,
Akihiko Odaki
