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

["Baldysiak, Pawel" <pawel.baldysiak@intel.com>]

On Wed, 2016-03-09 at 11:23 -0500, Jes Sorensen wrote:
> 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
Hello,
Thanks for the fix Jes.

I thinks that instead of making it "void" we can actually check the return value and print a proper message if something goes wrong.
Also "simplified for() loop" can be applied to ahci_enumerate_ports() as well.

@@ -1624,7 +1624,10 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
 	 * this hba
 	 */
 	dir = opendir("/sys/dev/block");
-	for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
+	if (!dir)
+		return 1;
+
+	for (ent = readdir(dir); ent; ent = readdir(dir)) {
 		int fd;
 		char model[64];
 		char vendor[64];
@@ -2021,7 +2024,11 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle
 					print_imsm_capability(&entry->orom);
 					printf(" I/O Controller : %s (%s)\n",
 						vmd_domain_to_controller(hba, buf), get_sys_dev_type(hba->type));
-					print_vmd_attached_devs(hba);
+					if (print_vmd_attached_devs(hba)) {
+						if (verbose > 0)
+							pr_err("failed to get devices attached to VMD domain.\n");
+						result |= 2;
+					}
 					printf("\n");
 				}
 			}

Is that ok with you? Should I prepare a proper patch with this?

Thanks,
Pawel Baldysiak