Re: [PATCH v4 10/33] error: define g_autoptr() cleanup function for the Error type
On 3.02.2025 22:13, Daniel P. Berrangé wrote: On Thu, Jan 30, 2025 at 11:08:31AM +0100, Maciej S. Szmigiero wrote: From: "Maciej S. Szmigiero" Automatic memory management helps avoid memory safety issues. Signed-off-by: Maciej S. Szmigiero --- include/qapi/error.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/qapi/error.h b/include/qapi/error.h index 71f8fb2c50ee..649ec8f1b6a2 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -437,6 +437,8 @@ Error *error_copy(const Error *err);> q */ void error_free(Error *err); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free) + This has been rejected by Markus in the past when I proposed. See the rationale at the time here: https://lists.nongnu.org/archive/html/qemu-devel/2024-07/msg05503.html Thanks for the pointer, I wasn't expecting this change to be controversial. If you want this, the commit message will need to explain the use case and justify why the existing error usage patterns are insufficient. In this case it's about giving received Error to migrate_set_error() which does *not* take ownership of it. And the reason why migrate_set_error() does not take ownership of incoming Error is that it might have an Error already set in MigrationState, in this case it simply ignores the passed Error (almost like being a NOP in this case). I don't know whether this is enough of a justification for introducing g_autoptr(Error). I'm happy to drop this commit and change it to manual memory management instead if it is not. @Markus, what's your opinion here? With regards, Daniel Thanks, Maciej
Re: [PATCH v4 10/33] error: define g_autoptr() cleanup function for the Error type
On Thu, Jan 30, 2025 at 11:08:31AM +0100, Maciej S. Szmigiero wrote: > From: "Maciej S. Szmigiero" > > Automatic memory management helps avoid memory safety issues. > > Signed-off-by: Maciej S. Szmigiero > --- > include/qapi/error.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/qapi/error.h b/include/qapi/error.h > index 71f8fb2c50ee..649ec8f1b6a2 100644 > --- a/include/qapi/error.h > +++ b/include/qapi/error.h > @@ -437,6 +437,8 @@ Error *error_copy(const Error *err);> q */ > void error_free(Error *err); > > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free) > + This has been rejected by Markus in the past when I proposed. See the rationale at the time here: https://lists.nongnu.org/archive/html/qemu-devel/2024-07/msg05503.html If you want this, the commit message will need to explain the use case and justify why the existing error usage patterns are insufficient. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v4 10/33] error: define g_autoptr() cleanup function for the Error type
On Thu, Jan 30, 2025 at 11:08:31AM +0100, Maciej S. Szmigiero wrote: > From: "Maciej S. Szmigiero" > > Automatic memory management helps avoid memory safety issues. > > Signed-off-by: Maciej S. Szmigiero Reviewed-by: Peter Xu -- Peter Xu
