[dm-devel] [PATCH 3/4] mpathpersist: fix leaks

[Benjamin Marzinski]

On Fri, Sep 13, 2019 at 07:56:13AM +0000, Martin Wilck wrote:
> On Tue, 2019-09-10 at 17:39 -0500, Benjamin Marzinski wrote:
> > If handle_args() fails while looping through the argument list, it
> > needs
> > to free batch_fn, if it has been set. Also handle_args() needs to
> > make
> > sure to free the file descriptor after it has been opened.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> > ---
> >  mpathpersist/main.c | 31 ++++++++++++++++++++-----------
> >  1 file changed, 20 insertions(+), 11 deletions(-)
> > 
> 
> 
> Looking at this code again, I wonder why we don't "goto out" in this
> code in the "else if (prin_flag)" case:
> 
> 		if (0 == num_prin_sa)
> 		{
> 			fprintf (stderr,
> 					" No service action given for Persistent Reserve IN\n");
> 			ret = MPATH_PR_SYNTAX_ERROR;
> 		}
> 		else if (num_prin_sa > 1)
> 		{
> 			fprintf (stderr, " Too many service actions given; choose "
> 					"one only\n");
> 			ret = MPATH_PR_SYNTAX_ERROR;
> 		}
> 
> At least in the first case, prin_sa would be -1, and trying to continue
> looks unhealthy. In the second case, the error message might be treated
> as a warning and the second prin action would override the first. I
> personally would think that it would be better to assume the user made
> a mistake, and do nothing, in this case as well.

I agree. I'll respin the patches with this included.

-Ben

> Anyway, that's not your patch's fault. So:
> 
> Reviewed-by: Martin Wilck <mwilck at suse.com>
> 
>