[libvirt] [PATCH 05/13] domain_conf: Resolve resource leaks found by Valgrind

[John Ferlan]

On 02/06/2013 09:06 PM, Osier Yang wrote:
> On 2013年02月07日 05:35, John Ferlan wrote:
>> Fix various resource leaks discovered while parsing through Valgrind
>> output
>> ---
>>   src/conf/domain_conf.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 27f5b5e..62a604f 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -7604,6 +7604,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node,
>>       VIR_FREE(ram);
>>       VIR_FREE(vram);
>>       VIR_FREE(heads);
>> +    VIR_FREE(primary);
>>
>>       return def;
>>
>> @@ -9587,6 +9588,7 @@ static virDomainDefPtr
>> virDomainDefParseXML(virCapsPtr caps,
>>                       }
>>
>>                       if ((value =
>> virDomainFeatureStateTypeFromString(tmp))<  0) {
>> +                        VIR_FREE(tmp);
>>                           virReportError(VIR_ERR_XML_ERROR,
>>                                          _("invalid value of state
>> argument "
>>                                            "for HyperV Enlightenment
>> feature '%s'"),
>> @@ -9594,6 +9596,7 @@ static virDomainDefPtr
>> virDomainDefParseXML(virCapsPtr caps,
>>                           goto error;
>>                       }
>>
>> +                    VIR_FREE(tmp);
>>                       def->hyperv_features[feature] = value;
>>                       break;
> 
> Good to fix the leak in the similar hunk as well:
> 
>                 tmp = virXPathString("string(./features/apic/@eoi)", ctxt);
>                 if (tmp) {
>                     int eoi;
>                     if ((eoi = virDomainFeatureStateTypeFromString(tmp))
> <= 0) {
>                         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                                        _("unknown value for attribute
> eoi: %s"),
>                                        tmp);
>                         goto error;
>                     }
>                     def->apic_eoi = eoi;
>                     VIR_FREE(tmp);
>                 }
> 
> 
>>
>> @@ -9922,6 +9925,7 @@ static virDomainDefPtr
>> virDomainDefParseXML(virCapsPtr caps,
>>           if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) {
>>               if (controller->model ==
>> VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) {
>>                   if (usb_other || usb_none) {
>> +                    virDomainControllerDefFree(controller);
>>                       virReportError(VIR_ERR_XML_DETAIL, "%s",
>>                                      _("Can't add another USB
>> controller: "
>>                                        "USB is disabled for this
>> domain"));
>> @@ -9930,6 +9934,7 @@ static virDomainDefPtr
>> virDomainDefParseXML(virCapsPtr caps,
>>                   usb_none = true;
>>               } else {
>>                   if (usb_none) {
>> +                    virDomainControllerDefFree(controller);
>>                       virReportError(VIR_ERR_XML_DETAIL, "%s",
>>                                      _("Can't add another USB
>> controller: "
>>                                        "USB is disabled for this
>> domain"));
>> @@ -10227,6 +10232,7 @@ static virDomainDefPtr
>> virDomainDefParseXML(virCapsPtr caps,
>>
>>           /* Check if USB bus is required */
>>           if (input->bus == VIR_DOMAIN_INPUT_BUS_USB&&  usb_none) {
>> +            virDomainInputDefFree(input);
>>               virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>                              _("Can't add USB input device. "
>>                                "USB bus is disabled"));
>> @@ -10324,6 +10330,7 @@ static virDomainDefPtr
>> virDomainDefParseXML(virCapsPtr caps,
>>
>>           if (video->primary) {
>>               if (primaryVideo) {
>> +                virDomainVideoDefFree(video);
>>                   virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>                                  _("Only one primary video device is
>> supported"));
>>                   goto error;
>> @@ -10335,8 +10342,10 @@ static virDomainDefPtr
>> virDomainDefParseXML(virCapsPtr caps,
>>           if (VIR_INSERT_ELEMENT_INPLACE(def->videos,
>>                                          ii,
>>                                          def->nvideos,
>> -                                       video)<  0)
>> +                                       video)<  0) {
>> +            virDomainVideoDefFree(video);
>>               goto error;
>> +        }
>>       }
>>       VIR_FREE(nodes);
>>
>> @@ -10452,6 +10461,7 @@ static virDomainDefPtr
>> virDomainDefParseXML(virCapsPtr caps,
>>               goto error;
>>
>>           if (hub->type == VIR_DOMAIN_HUB_TYPE_USB&&  usb_none) {
>> +            virDomainHubDefFree(hub);
>>               virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>                              _("Can't add USB hub: "
>>                                "USB is disabled for this domain"));
> 
> ACK with fixing the similar hunk.


In retrospect, the virDomainDefParseXML() will VIR_FREE(tmp) in the
error: path, so rather than add within the error path as I did at line
9592 and the error path for "./features/apic/@eoi", I removed that one
line.

I probably added that one as a result of adding the one in the non error
path and hyper-focusing on the code on the screen around the change.

John