Re: [PATCH v2 1/3] monitor: Add HMP and QMP interfaces

[Daniel P . Berrangé]

On Mon, Sep 13, 2021 at 08:52:28PM +0800, Yang Zhong wrote:
> On Mon, Sep 13, 2021 at 01:56:13PM +0100, Daniel P. Berrangé wrote:
> > On Mon, Sep 13, 2021 at 02:48:37PM +0200, Paolo Bonzini wrote:
> > > On 13/09/21 11:35, Daniel P. Berrangé wrote:
> > > > >    g_autoptr(Error) err = NULL
> > > > I was mistaken here - Error shouldn't use g_autoptr, just
> > > > 
> > > >     Error err = NULL;
> > > > 
> > > > > > +    SGXInfo *info = qmp_query_sgx(NULL);
> > > > > Pass in &err not NULL;
> > > > > 
> > > > > Also  declare it with  'g_autoptr(SGXInfo) info = ...'
> > > > > 
> > > > > And then
> > > > > 
> > > > >     if (err) {
> > > > >        monitor_printf(mon, "%s\n", error_get_pretty(err);
> > > > Then use the simpler:
> > > > 
> > > >      error_report_err(err);
> > > 
> > > Indeed.
> > > 
> > > That said, more long term (but this is something Coccinelle could help 
> > > with)
> > > perhaps error_report_err should not free the error, and instead we should
> > > use g_autoptr(Error) in the callers.  I don't like functions that do not
> > > have free in their name and yet free a pointer...
> > 
> > Yes, this error_report_err surprises me every 6 months when I
> > come to deal with it. So I think using g_autoptr would be a
> > nice replacement, with no additional burden in terms of lines
> > of code in callers too.
> >
> 
>   Do we need call qapi_free_SGXInfo(info) here?
> 
>   In previous code design, the code like below:
> 
>   SGXInfo *info = qmp_query_sgx(&err);
>   ......
>   qapi_free_SGXInfo(info);

I suggested "g_autoptr(SGXInfo) info" for the declaration to avoid
the need for qapi_free_SGXInfo calls


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 :|