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