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 <[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.
I agree. I'll respin the patches with this included. -Ben > 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
