Re: [PATCH 3/6] libmultipath: refactor calls to get dm device info

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

That can be done in a separate patch, of course.

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




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux