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.
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
--
Matthias Bolte
http://photron.blogspot.com
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list