Re: [PATCH 5/8] {platform,super}-intel: Fix two resource leaks

[Jes Sorensen <Jes.Sorensen@redhat.com>]

NeilBrown <neilb@suse.de> writes:
> On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:
>
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> The code did not free 'dir' allocated by opendir(). An additional
>> benefit is that this simplifies the for() loops.
>>
>> Fixes: 60f0f54d ("IMSM: Add support for VMD")
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>  platform-intel.c | 7 ++++++-
>>  super-intel.c    | 6 +++++-
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/platform-intel.c b/platform-intel.c
>> index 88818f3..c60fd9e 100644
>> --- a/platform-intel.c
>> +++ b/platform-intel.c
>> @@ -724,8 +724,10 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
>>  		return NULL;
>>  
>>  	dir = opendir("/sys/bus/pci/drivers/vmd");
>> +	if (!dir)
>> +		return NULL;
>>  
>> -	for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
>> +	for (ent = readdir(dir); ent; ent = readdir(dir)) {
>>  		sprintf(path, "/sys/bus/pci/drivers/vmd/%s/domain/device",
>>  			ent->d_name);
>>  
>> @@ -734,8 +736,11 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
>>  
>>  		if (strncmp(buf, hba->path, strlen(buf)) == 0) {
>>  			sprintf(path, "/sys/bus/pci/drivers/vmd/%s", ent->d_name);
>> +			closedir(dir);
>>  			return realpath(path, buf);
>>  		}
>>  	}
>> +
>> +	closedir(dir);
>>  	return NULL;
>>  }
>> diff --git a/super-intel.c b/super-intel.c
>> index 158f4e8..e1bee75 100644
>> --- a/super-intel.c
>> +++ b/super-intel.c
>> @@ -1781,7 +1781,10 @@ static int print_vmd_attached_devs(struct sys_dev *hba)
>>  	 * this hba
>>  	 */
>>  	dir = opendir("/sys/bus/pci/drivers/nvme");
>> -	for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
>> +	if (!dir)
>> +		return 1;
>> +
>
> Returning '1' looks really weird here.  I can see it is consistent with
> 	if (hba->type != SYS_DEV_VMD)
> 		return 1;
>
> above, but still....
> As the return value is never used, should we just make it 'void' ??

Seems reasonable - I'll put that in a separate patch.

Cheers,
Jes