On Thu, Sep 10, 2020 at 06:48:56PM +0200, Martin Wilck wrote: > On Thu, 2020-09-10 at 18:51 +0800, lixiaokeng wrote: > > In assemble_map func, f = STRDUP(mp->features) is just used > > for APPEND(). We can directly pass mp->features to APPEND(). > > The mpp->features, hwhandler and selector got form strdup > > should be check after select* function. > > > > Signed-off-by: Zhiqiang Liu <liuzhiqian...@huawei.com> > > Signed-off-by: Lixiaokeng <lixiaok...@huawei.com> > > Thanks for submitting the new version. Looking at your patch, I realize > that my previous suggestion wasn't perfect. > > > --- > > libmultipath/configure.c | 5 +++++ > > libmultipath/dmparser.c | 11 ++++------- > > 2 files changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/libmultipath/configure.c b/libmultipath/configure.c > > index 5bc65fd3..5d5d9415 100644 > > --- a/libmultipath/configure.c > > +++ b/libmultipath/configure.c > > @@ -355,6 +355,11 @@ int setup_map(struct multipath *mpp, char > > *params, int params_size, > > select_ghost_delay(conf, mpp); > > select_flush_on_last_del(conf, mpp); > > > > + if (!mpp->features || !mpp->hwhandler || !mpp->selector) { > > + condlog(0, "%s: map select failed", mpp->alias); > > + return 1; > > + } > > + > > We have a new failure mode for setup_map() here. While this is a good > thing in principle, there are issues. > > 1) by returning, we skip the rest of the initialization steps for the > map. Thus paths and pathgroups may be set up wrongly after setup_map(). > 2) Not every caller deletes the map from the mpvec after setup_map() > fails. For some callers like resize_map() or reload_map(), that's > actually not possible. > > 1) is a minor problem because no caller calls domap() after setup_map() > failure, afaics. But 2) is a problem, because the broken, partially > initialized struct multipath will remain in our data structures, and my > assumption that features etc. are always valid will be violated. > 2) is not a regression of this patch though, it has always been the > case. > > I'm not yet decided how to deal with this. Perhaps setup_map() > shouldn't free features, hwhandler, and selector until the new values > have been successfully obtained. > > (Actually, what's the point in running through *all* select_xyz() > functions just for a map resize?) > > Ben?
I don't know of any reason why that's necessary, or why it was originally done that way. If we've already configured the device, and we're not doing a reconfigure, there's not a lot to be gained by doing configuring again. Some to the select_* functions grab stuff from sysfs, and so could return different values, but I'm sure there is a lot of work that could be cut out here, with no noticeable changes in behavior. Ideally we should stick to a "do no harm" policy when updating the device. It seems better to have a device structure that's outdated than one that's invalid. But regardless of what we do in setup_map, the assemble_map() part of this patch is correct, and as you said, the setup_map() changes don't introduce any new problems. It seems like resolving the issue in setup_map() should be seperate work. Reviewed-by: Benjamin Marzinski <bmarz...@redhat.com> > Regards > Martin > > > > > > > > sysfs_set_scsi_tmo(mpp, conf->checkint); > > marginal_pathgroups = conf->marginal_pathgroups; > > pthread_cleanup_pop(1); > > diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c > > index 482e9d0e..685918da 100644 > > --- a/libmultipath/dmparser.c > > +++ b/libmultipath/dmparser.c > > @@ -65,7 +65,7 @@ assemble_map (struct multipath * mp, char * params, > > int len) > > int i, j; > > int minio; > > int nr_priority_groups, initial_pg_nr; > > - char * p, * f; > > + char * p; > > const char *const end = params + len; > > char no_path_retry[] = "queue_if_no_path"; > > char retain_hwhandler[] = "retain_attached_hw_handler"; > > @@ -86,10 +86,9 @@ assemble_map (struct multipath * mp, char * > > params, int len) > > get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) > > add_feature(&mp->features, retain_hwhandler); > > > > - f = STRDUP(mp->features); > > - > > - APPEND(p, end, "%s %s %i %i", f, mp->hwhandler, > > nr_priority_groups, > > - initial_pg_nr); > > + /* mp->features must not be NULL */ > > + APPEND(p, end, "%s %s %i %i", mp->features, mp->hwhandler, > > + nr_priority_groups, initial_pg_nr); > > > > vector_foreach_slot (mp->pg, pgp, i) { > > pgp = VECTOR_SLOT(mp->pg, i); > > @@ -110,12 +109,10 @@ assemble_map (struct multipath * mp, char * > > params, int len) > > } > > } > > > > - FREE(f); > > condlog(4, "%s: assembled map [%s]", mp->alias, params); > > return 0; > > > > err: > > - FREE(f); > > return 1; > > } > > > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel