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 <[email protected]>
> ---
> 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 <[email protected]>
--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel
