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@xxxxxxxxxx> > --- > 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@xxxxxxxx> -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel