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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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