RE: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Sent: 12 June 2020 10:54
> To: paul@xxxxxxx; 'Xen-devel' <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Cc: 'Ian Jackson' <Ian.Jackson@xxxxxxxxxx>; 'Wei Liu' <wl@xxxxxxx>; 'Juergen 
> Gross' <jgross@xxxxxxxx>
> Subject: Re: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()
> 
> On 12/06/2020 08:22, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> Sent: 10 June 2020 12:40
> >> To: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> >> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Ian Jackson 
> >> <Ian.Jackson@xxxxxxxxxx>; Wei Liu
> >> <wl@xxxxxxx>; Juergen Gross <jgross@xxxxxxxx>; Paul Durrant <paul@xxxxxxx>
> >> Subject: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()
> >>
> >> c/s 6902cb00e03 "tools/libxendevicemodel: extract functions and add a 
> >> compat
> >> layer" introduced calls to both xencall_open() and 
> >> osdep_xendevicemodel_open()
> >> but failed to fix up the error path.
> >>
> >> c/s f68c7c618a3 "libs/devicemodel: free xencall handle in error path in
> >> _open()" fixed up the xencall_open() aspect of the error path (missing the
> >> osdep_xendevicemodel_open() aspect), but positioned the xencall_close()
> >> incorrectly, creating the same pattern proved to be problematic by c/s
> >> 30a72f02870 "tools: fix error path of xenhypfs_open()".
> >>
> >> Reposition xtl_logger_destroy(), and introduce the missing
> >> osdep_xendevicemodel_close().
> >>
> >> Fixes: 6902cb00e03 ("tools/libxendevicemodel: extract functions and add a 
> >> compat layer")
> >> Fixes: f68c7c618a3 ("libs/devicemodel: free xencall handle in error path 
> >> in _open()")
> >> Backport: 4.9+
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> ---
> >> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxx>
> >> CC: Wei Liu <wl@xxxxxxx>
> >> CC: Juergen Gross <jgross@xxxxxxxx>
> >> CC: Paul Durrant <paul@xxxxxxx>
> >>
> >> RFC - this is still broken.
> >>
> > I'm slightly confused. Do you want this in 4.14 in this form or are you 
> > expecting to update it?
> 
> In this form, it is an improvement over before.
> 
> There is still the crash described below which needs some form of
> figuring out and fixing.
> 

Ok, in which case consider it...

Release-acked-by: Paul Durrant <paul@xxxxxxx>

> ~Andrew
> 
> >
> >   Paul
> >
> >> Failure to create the logger will still hit the NULL deference, in all of 
> >> the
> >> stable libs, not just devicemodel.
> >>
> >> Also, unless I'd triple checked the history, I was about to reintroduce the
> >> deadlock from c/s 9976f3874d4, because it totally counterintuitive wrong to
> >> expect setup and teardown in opposite orders.
> >> ---
> >>  tools/libs/devicemodel/core.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> >> index db501d9e80..4d4063956d 100644
> >> --- a/tools/libs/devicemodel/core.c
> >> +++ b/tools/libs/devicemodel/core.c
> >> @@ -67,9 +67,10 @@ xendevicemodel_handle 
> >> *xendevicemodel_open(xentoollog_logger *logger,
> >>      return dmod;
> >>
> >>  err:
> >> -    xtl_logger_destroy(dmod->logger_tofree);
> >> +    osdep_xendevicemodel_close(dmod);
> >>      xentoolcore__deregister_active_handle(&dmod->tc_ah);
> >>      xencall_close(dmod->xcall);
> >> +    xtl_logger_destroy(dmod->logger_tofree);
> >>      free(dmod);
> >>      return NULL;
> >>  }
> >> --
> >> 2.11.0
> >