On Thu, May 11, 2017 at 10:26:00PM +0200, Martin Wilck wrote:
> On Tue, 2017-05-09 at 11:57 -0500, Benjamin Marzinski wrote:
> > Multipath has a number of functions that all do basically the same
> > thing to get members of the dm_info structure. This just refactors
> > them
> > to use common code to do it.
>
> This is nice, but it might be even better to simply store all relevant
> information from DM_DEVICE_INFO in the mpp structure in the first
> place, avoiding the need to do this ioctl multiple times to get various
> pieces of information.
I totally agree that we are likely underusing mpp->dmi, but in a number
of these cases, you actually need the current information. In still
others, you don't necessarily even have a multipath structure to put it
in. So, I think dm_get_info needs to simply return the info.
>
> That can be done in a separate patch, of course.
Yeah. We definitely need to look at all the times where we grab a fresh
copy of the device info, and see if we are actually looking at a field
that can change over time, where we need the current data. mpp->dmi is
great for when we don't need current data, and we should definitely make
sure we're using it there. On the other hand, it's pointless to keep
freeing and mallocing it when we really need current info.
-Ben
> Reviewed-by: Martin Wilck <mwilck@xxxxxxxx>
>
> Regards
> Martin
>
>
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> > ---
> > libmultipath/devmapper.c | 171 +++++++++---------------------------
> > ----------
> > libmultipath/devmapper.h | 3 +-
> > multipathd/cli_handlers.c | 24 ++-----
> > 3 files changed, 42 insertions(+), 156 deletions(-)
> >
> > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> > index 1c6b87e..2c4a13a 100644
> > --- a/libmultipath/devmapper.c
> > +++ b/libmultipath/devmapper.c
> > @@ -399,16 +399,16 @@ int dm_addmap_reload(struct multipath *mpp,
> > char *params, int flush)
> > return r;
> > }
> >
> > -int dm_map_present(const char * str)
> > +static int
> > +do_get_info(const char *name, struct dm_info *info)
> > {
> > - int r = 0;
> > + int r = -1;
> > struct dm_task *dmt;
> > - struct dm_info info;
> >
> > if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> > - return 0;
> > + return r;
> >
> > - if (!dm_task_set_name(dmt, str))
> > + if (!dm_task_set_name(dmt, name))
> > goto out;
> >
> > dm_task_no_open_count(dmt);
> > @@ -416,16 +416,25 @@ int dm_map_present(const char * str)
> > if (!dm_task_run(dmt))
> > goto out;
> >
> > - if (!dm_task_get_info(dmt, &info))
> > + if (!dm_task_get_info(dmt, info))
> > goto out;
> >
> > - if (info.exists)
> > - r = 1;
> > + if (!info->exists)
> > + goto out;
> > +
> > + r = 0;
> > out:
> > dm_task_destroy(dmt);
> > return r;
> > }
> >
> > +int dm_map_present(const char * str)
> > +{
> > + struct dm_info info;
> > +
> > + return (do_get_info(str, &info) == 0);
> > +}
> > +
> > int dm_get_map(const char *name, unsigned long long *size, char
> > *outparams)
> > {
> > int r = 1;
> > @@ -646,29 +655,15 @@ out:
> > static int
> > dm_dev_t (const char * mapname, char * dev_t, int len)
> > {
> > - int r = 1;
> > - struct dm_task *dmt;
> > struct dm_info info;
> >
> > - if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> > - return 0;
> > -
> > - if (!dm_task_set_name(dmt, mapname))
> > - goto out;
> > -
> > - if (!dm_task_run(dmt))
> > - goto out;
> > -
> > - if (!dm_task_get_info(dmt, &info) || !info.exists)
> > - goto out;
> > + if (do_get_info(mapname, &info) != 0)
> > + return 1;
> >
> > if (snprintf(dev_t, len, "%i:%i", info.major, info.minor) >
> > len)
> > - goto out;
> > + return 1;
> >
> > - r = 0;
> > -out:
> > - dm_task_destroy(dmt);
> > - return r;
> > + return 0;
> > }
> >
> > int
> > @@ -700,59 +695,16 @@ out:
> > }
> >
> > int
> > -dm_get_major (char * mapname)
> > +dm_get_major_minor(const char *name, int *major, int *minor)
> > {
> > - int r = -1;
> > - struct dm_task *dmt;
> > struct dm_info info;
> >
> > - if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> > - return 0;
> > -
> > - if (!dm_task_set_name(dmt, mapname))
> > - goto out;
> > -
> > - if (!dm_task_run(dmt))
> > - goto out;
> > -
> > - if (!dm_task_get_info(dmt, &info))
> > - goto out;
> > -
> > - if (!info.exists)
> > - goto out;
> > -
> > - r = info.major;
> > -out:
> > - dm_task_destroy(dmt);
> > - return r;
> > -}
> > -
> > -int
> > -dm_get_minor (char * mapname)
> > -{
> > - int r = -1;
> > - struct dm_task *dmt;
> > - struct dm_info info;
> > -
> > - if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> > - return 0;
> > -
> > - if (!dm_task_set_name(dmt, mapname))
> > - goto out;
> > -
> > - if (!dm_task_run(dmt))
> > - goto out;
> > -
> > - if (!dm_task_get_info(dmt, &info))
> > - goto out;
> > -
> > - if (!info.exists)
> > - goto out;
> > + if (do_get_info(name, &info) != 0)
> > + return -1;
> >
> > - r = info.minor;
> > -out:
> > - dm_task_destroy(dmt);
> > - return r;
> > + *major = info.major;
> > + *minor = info.minor;
> > + return 0;
> > }
> >
> > static int
> > @@ -1070,31 +1022,12 @@ out:
> > int
> > dm_geteventnr (char *name)
> > {
> > - struct dm_task *dmt;
> > struct dm_info info;
> > - int event = -1;
> >
> > - if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> > + if (do_get_info(name, &info) != 0)
> > return -1;
> >
> > - if (!dm_task_set_name(dmt, name))
> > - goto out;
> > -
> > - dm_task_no_open_count(dmt);
> > -
> > - if (!dm_task_run(dmt))
> > - goto out;
> > -
> > - if (!dm_task_get_info(dmt, &info))
> > - goto out;
> > -
> > - if (info.exists)
> > - event = info.event_nr;
> > -
> > -out:
> > - dm_task_destroy(dmt);
> > -
> > - return event;
> > + return info.event_nr;
> > }
> >
> > char *
> > @@ -1262,26 +1195,12 @@ cancel_remove_partmap (const char *name, void
> > *unused)
> > static int
> > dm_get_deferred_remove (char * mapname)
> > {
> > - int r = -1;
> > - struct dm_task *dmt;
> > struct dm_info info;
> >
> > - if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> > + if (do_get_info(mapname, &info) != 0)
> > return -1;
> >
> > - if (!dm_task_set_name(dmt, mapname))
> > - goto out;
> > -
> > - if (!dm_task_run(dmt))
> > - goto out;
> > -
> > - if (!dm_task_get_info(dmt, &info))
> > - goto out;
> > -
> > - r = info.deferred_remove;
> > -out:
> > - dm_task_destroy(dmt);
> > - return r;
> > + return info.deferred_remove;
> > }
> >
> > static int
> > @@ -1328,9 +1247,6 @@ alloc_dminfo (void)
> > int
> > dm_get_info (char * mapname, struct dm_info ** dmi)
> > {
> > - int r = 1;
> > - struct dm_task *dmt = NULL;
> > -
> > if (!mapname)
> > return 1;
> >
> > @@ -1340,32 +1256,13 @@ dm_get_info (char * mapname, struct dm_info
> > ** dmi)
> > if (!*dmi)
> > return 1;
> >
> > - if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> > - goto out;
> > -
> > - if (!dm_task_set_name(dmt, mapname))
> > - goto out;
> > -
> > - dm_task_no_open_count(dmt);
> > -
> > - if (!dm_task_run(dmt))
> > - goto out;
> > -
> > - if (!dm_task_get_info(dmt, *dmi))
> > - goto out;
> > -
> > - r = 0;
> > -out:
> > - if (r) {
> > + if (do_get_info(mapname, *dmi) != 0) {
> > memset(*dmi, 0, sizeof(struct dm_info));
> > FREE(*dmi);
> > *dmi = NULL;
> > + return 1;
> > }
> > -
> > - if (dmt)
> > - dm_task_destroy(dmt);
> > -
> > - return r;
> > + return 0;
> > }
> >
> > struct rename_data {
> > diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> > index aca4454..5b66865 100644
> > --- a/libmultipath/devmapper.h
> > +++ b/libmultipath/devmapper.h
> > @@ -52,8 +52,7 @@ int dm_enablegroup(char * mapname, int index);
> > int dm_disablegroup(char * mapname, int index);
> > int dm_get_maps (vector mp);
> > int dm_geteventnr (char *name);
> > -int dm_get_major (char *name);
> > -int dm_get_minor (char *name);
> > +int dm_get_major_minor (const char *name, int *major, int *minor);
> > char * dm_mapname(int major, int minor);
> > int dm_remove_partmaps (const char * mapname, int need_sync,
> > int deferred_remove);
> > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> > index d598039..04c7386 100644
> > --- a/multipathd/cli_handlers.c
> > +++ b/multipathd/cli_handlers.c
> > @@ -746,7 +746,7 @@ cli_add_map (void * v, char ** reply, int * len,
> > void * data)
> > char * param = get_keyparam(v, MAP);
> > int major, minor;
> > char dev_path[PATH_SIZE];
> > - char *alias, *refwwid;
> > + char *refwwid, *alias = NULL;
> > int rc, count = 0;
> > struct config *conf;
> >
> > @@ -763,14 +763,12 @@ cli_add_map (void * v, char ** reply, int *
> > len, void * data)
> > }
> > put_multipath_config(conf);
> > do {
> > - minor = dm_get_minor(param);
> > - if (minor < 0)
> > + if (dm_get_major_minor(param, &major, &minor) < 0)
> > condlog(2, "%s: not a device mapper table",
> > param);
> > - major = dm_get_major(param);
> > - if (major < 0)
> > - condlog(2, "%s: not a device mapper table",
> > param);
> > - sprintf(dev_path, "dm-%d", minor);
> > - alias = dm_mapname(major, minor);
> > + else {
> > + sprintf(dev_path, "dm-%d", minor);
> > + alias = dm_mapname(major, minor);
> > + }
> > /*if there is no mapname found, we first create the
> > device*/
> > if (!alias && !count) {
> > condlog(2, "%s: mapname not found for
> > %d:%d",
> > @@ -804,23 +802,15 @@ cli_del_map (void * v, char ** reply, int *
> > len, void * data)
> > struct vectors * vecs = (struct vectors *)data;
> > char * param = get_keyparam(v, MAP);
> > int major, minor;
> > - char dev_path[PATH_SIZE];
> > char *alias;
> > int rc;
> >
> > param = convert_dev(param, 0);
> > condlog(2, "%s: remove map (operator)", param);
> > - minor = dm_get_minor(param);
> > - if (minor < 0) {
> > - condlog(2, "%s: not a device mapper table", param);
> > - return 1;
> > - }
> > - major = dm_get_major(param);
> > - if (major < 0) {
> > + if (dm_get_major_minor(param, &major, &minor) < 0) {
> > condlog(2, "%s: not a device mapper table", param);
> > return 1;
> > }
> > - sprintf(dev_path,"dm-%d", minor);
> > alias = dm_mapname(major, minor);
> > if (!alias) {
> > condlog(2, "%s: mapname not found for %d:%d",
>
> --
> Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
