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

[Martin Wilck]

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.

Anyway, that's not your patch's fault. So:

Reviewed-by: Martin Wilck <mwilck at suse.com>