On 01/10/2013 04:55 PM, Matthias Bolte wrote:
> 2013/1/10 John Ferlan <jferlan@xxxxxxxxxx>:
>> On 01/10/2013 10:49 AM, Matthias Bolte wrote:
>>> 2013/1/10 Eric Blake <eblake@xxxxxxxxxx>:
>>>> On 01/09/2013 07:54 AM, John Ferlan wrote:
>>>>> Because result was used to determine whether or not to free 'priv'
>>>>> resources Coverity tagged the code as having a resource leak. This
>>>>> change addresses that concern.
>>>>> ---
>>>>> src/esx/esx_driver.c | 8 ++++----
>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
>>>>> index 1366c81..9befa38 100644
>>>>> --- a/src/esx/esx_driver.c
>>>>> +++ b/src/esx/esx_driver.c
>>>>> @@ -998,6 +998,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth,
>>>>> virReportOOMError();
>>>>> goto cleanup;
>>>>> }
>>>>> + conn->privateData = priv;
>>>>>
>>>>> if (esxUtil_ParseUri(&priv->parsedUri, conn->uri) < 0) {
>>>>> goto cleanup;
>>>>
>>>> I
>>>>
>>>>> @@ -1008,8 +1009,6 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth,
>>>>> priv->supportsLongMode = esxVI_Boolean_Undefined;
>>>>> priv->usedCpuTimeCounterId = -1;
>>>>>
>>>>> - conn->privateData = priv;
>>>>> -
>>>>> /*
>>>>> * Set the port dependent on the transport protocol if no port is
>>>>> * specified. This allows us to rely on the port parameter being
>>>>> @@ -1104,9 +1103,10 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth,
>>>>> result = VIR_DRV_OPEN_SUCCESS;
>>>>>
>>>>> cleanup:
>>>>> - if (result == VIR_DRV_OPEN_ERROR) {
>>>>> + if (result == VIR_DRV_OPEN_ERROR)
>>>>> + conn->privateData = NULL;
>>>>> + if (priv && !conn->privateData)
>>>>> esxFreePrivate(&priv);
>>>>> - }
>>>>
>>>> This feels a bit complex;
>>>
>>> I agree.
>>>
>>
>> +1... for a reason though...
>>
>>>> I had to go read esxFreePrivate() to make sure
>>>> it would behave if called on a partial object. Would it be any easier
>>>> to delay the assignment to conn->privateData, and use transfer of
>>>> ownership semantics, so that the cleanup is unconditional?
>>>>
>>>> conn->privateData = priv;
>>>> priv = NULL;
>>>> result = VIR_DRV_OPEN_SUCCESS;
>>>> cleanup:
>>>> esxFreePrivate(&priv);
>>>
>>> No! This cannot be done that easily, see commit
>>> b126715a48cd0cbe32ec6468c267cd8cf2961c55 "esx: Fix segfault in
>>> esxConnectToHost".
>>>
>>
>> And this is why... It is a "false positive" and possibly could be dealt
>> with by adding a /* coverity[leaked_storage] */ in the right place as
>> well. Although I've had some issues trying to do that for another set
>> of changes because the going out of scope happens on the return and
>> putting a coverity commit prior to a return hasn't done what I'd expect.
>
> Well, you're commit message didn't state this as a false positive, so
> I looked carefully at esxOpen whether or not Coverity was right here,
> but couldn't find anything.
>
Sorry- I had put that in the cover letter
> Anyway, I'd like to simplify the logic in esxOpen a bit the way Eric
> suggested. Here's a patch that does this:
>
> https://www.redhat.com/archives/libvir-list/2013-January/msg00657.html
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list