Re: [PATCH 05/14] multipathd: use MALLOC and check return value in, cli_getprkey func

[Martin Wilck <mwilck@suse.com>]

On Fri, 2020-09-04 at 13:25 -0500, Benjamin Marzinski wrote:
> On Thu, Sep 03, 2020 at 09:13:04PM +0200, Martin Wilck wrote:
> > On Wed, 2020-09-02 at 15:18 +0800, lixiaokeng wrote:
> > > In cli_getprkey func, we use MALLOC instead of malloc, and check
> > > the return value of MALLOC.
> > > 
> > > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> > > Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
> > > Signed-off-by: Linfeilong <linfeilong@huawei.com>
> > > ---
> > >  multipathd/cli_handlers.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/multipathd/cli_handlers.c
> > > b/multipathd/cli_handlers.c
> > > index 27e4574f..d345afd3 100644
> > > --- a/multipathd/cli_handlers.c
> > > +++ b/multipathd/cli_handlers.c
> > > @@ -1535,7 +1535,11 @@ cli_getprkey(void * v, char ** reply, int
> > > *
> > > len, void * data)
> > >  	if (!mpp)
> > >  		return 1;
> > > 
> > > -	*reply = malloc(26);
> > > +	*reply = MALLOC(26);
> > > +	if (!*reply) {
> > > +		condlog(0, "malloc *reply failed.");
> > > +		return 1;
> > > +	}
> > 
> > MALLOC is not necessary (*reply isn't left uninialized), nor is the
> > error message.
> 
> What's you objection to the error message? Admittedly there is
> basically
> no chance that malloc(26) would ever actually fail. But when things
> fail, having error messages so that we can debug them faster is
> helpful.
> 
> If your objection is that malloc checks are mostly just there for
> good
> form, and so those error messages won't actually help in practice, I
> agree. But as a general rule, I think we should print error messages
> on
> things that are unambiguoulsy errors.

See my reply to 00/14. I'd like to standardize and streamline "out of
memory" error messages, rather than hand-coding them in every
procedure. I think that in 99% of cases, if multipathd crashes or
errors out due to OOM, the information in which function OOM occured
first will not be helpful. Even if we had a major memory leak, its
unlikely that such error messages would help us find it.

Do you disagree?

Martin