[dm-devel] [PATCH 09/14] libmultipath: check whether mp->features is NULl in, assemble_map

[Martin Wilck]

On Wed, 2020-09-09 at 11:18 +0800, lixiaokeng wrote:
> 
> On 2020/9/9 0:35, Martin Wilck wrote:
> > 
> > What about this:
> > 
> > assemble_map() is only called from setup_map(), which sets mp-
> > >features 
> > in select_features(). So what we should do is check for NULL after
> > select_features(), or have that function return an error code if
> > strdup
> > fails, and bail out early in setup_map() in that case.
> > 
> > Then we simply need to add a comment in assemble_map() saying that 
> > mp->features must be non-null when this function is called.
> > 
> > As I said earlier, I'm of course not against checking function
> > parameters, but here we should fail to setup a "struct multipath"
> > in
> > the first place in setup map(), rather than returning an
> > incompletely
> > initialized one. If we handle it this way, we don't need to check
> > the
> > fields of struct multipath over and over again. Similar arguments
> > hold
> > for other structs.
> > 
> > Of course this kind of assumption needs to be better documented in
> > the
> > code.
> > 
> > Regards
> > Martin
> > 
> > 
> Hi Martin,
>    If I don't misunderstand, we check feature after select_features
> and
> return 1 if feature is NULL in setup_map. We delete strdup and add
> message "mp->features must be non-null" in assemble_map. Like this:
> ---
> select_features(conf, mpp);
> if (!mpp->featrues)
> 	return 1;
> 
> ---
> /* mp->feature must not be NULL */
> APPEND(p, end, "%s %s %i %i", mp->features, mp->hwhandler
> ---

Yes. I guess error handling in setup_map() needs a closer look, too.

Martin