Re: [PATCH 35/44] libmultipath: use libmp_pathinfo() in update_multipath_table()

[Martin Wilck <martin.wilck@xxxxxxxx>]

On Thu, 2024-07-11 at 01:16 -0400, Benjamin Marzinski wrote:
> On Tue, Jul 09, 2024 at 11:39:26PM +0200, Martin Wilck wrote:
> > This allows us to remove dm_get_status(), and dm_get_map(), of
> > which
> > update_multipath_table() was the last caller.
> > 
> > Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> > ---
> >  libmultipath/devmapper.c   | 89 ----------------------------------
> > ----
> >  libmultipath/devmapper.h   |  2 -
> >  libmultipath/structs_vec.c | 22 +++++-----
> >  3 files changed, 12 insertions(+), 101 deletions(-)
> > 
> > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> > index 185c76b..105e838 100644
> > --- a/libmultipath/devmapper.c
> > +++ b/libmultipath/devmapper.c
> > @@ -804,45 +804,6 @@ static int dm_get_dm_uuid(const char *mapname,
> > char uuid[DM_UUID_LEN])
> >  			     (mapinfo_t) { .uuid = uuid });
> >  }
> >  
> > -int dm_get_map(const char *name, unsigned long long *size, char
> > **outparams)
> > -{
> > -	struct dm_task __attribute__((cleanup(cleanup_dm_task)))
> > *dmt = NULL;
> > -	uint64_t start, length;
> > -	char *target_type = NULL;
> > -	char *params = NULL;
> > -
> > -	if (!(dmt = libmp_dm_task_create(DM_DEVICE_TABLE)))
> > -		return DMP_ERR;
> > -
> > -	if (!dm_task_set_name(dmt, name))
> > -		return DMP_ERR;
> > -
> > -	errno = 0;
> > -	if (!libmp_dm_task_run(dmt)) {
> > -		dm_log_error(3, DM_DEVICE_TABLE, dmt);
> > -		if (dm_task_get_errno(dmt) == ENXIO)
> > -			return DMP_NOT_FOUND;
> > -		else
> > -			return DMP_ERR;
> > -	}
> > -
> > -	/* Fetch 1st target */
> > -	if (dm_get_next_target(dmt, NULL, &start, &length,
> > -			       &target_type, &params) != NULL ||
> > !params)
> > -		/* more than one target or not found target */
> > -		return DMP_NOT_FOUND;
> > -
> > -	if (size)
> > -		*size = length;
> > -
> > -	if (!outparams)
> > -		return DMP_OK;
> > -	else {
> > -		*outparams = strdup(params);
> > -		return *outparams ? DMP_OK : DMP_ERR;
> > -	}
> > -}
> > -
> >  bool is_mpath_uuid(const char uuid[DM_UUID_LEN])
> >  {
> >  	return !strncmp(uuid, UUID_PREFIX, UUID_PREFIX_LEN);
> > @@ -886,56 +847,6 @@ static bool is_mpath_part_uuid(const char
> > part_uuid[DM_UUID_LEN],
> >  	return !strcmp(part_uuid + nc, map_uuid);
> >  }
> >  
> > -int dm_get_status(const char *name, char **outstatus)
> > -{
> > -	int r = DMP_ERR;
> > -	struct dm_task __attribute__((cleanup(cleanup_dm_task)))
> > *dmt = NULL;
> > -	uint64_t start, length;
> > -	char *target_type = NULL;
> > -	char *status = NULL;
> > -
> > -	if (!(dmt = libmp_dm_task_create(DM_DEVICE_STATUS)))
> > -		return r;
> > -
> > -	if (!dm_task_set_name(dmt, name))
> > -		goto out;
> > -
> > -	errno = 0;
> > -	if (!libmp_dm_task_run(dmt)) {
> > -		dm_log_error(3, DM_DEVICE_STATUS, dmt);
> > -		if (dm_task_get_errno(dmt) == ENXIO)
> > -			r = DMP_NOT_FOUND;
> > -		goto out;
> > -	}
> > -
> > -	r = DMP_NOT_FOUND;
> > -	/* Fetch 1st target */
> > -	if (dm_get_next_target(dmt, NULL, &start, &length,
> > -			       &target_type, &status) != NULL)
> > -		goto out;
> > -
> > -	if (!target_type || strcmp(target_type, TGT_MPATH) != 0)
> > -		goto out;
> > -
> > -	if (!status) {
> > -		condlog(2, "got null status.");
> > -		goto out;
> > -	}
> > -
> > -	if (!outstatus)
> > -		r = DMP_OK;
> > -	else {
> > -		*outstatus = strdup(status);
> > -		r = *outstatus ? DMP_OK : DMP_ERR;
> > -	}
> > -out:
> > -	if (r != DMP_OK)
> > -		condlog(0, "%s: %s: error getting map status
> > string: %d",
> > -			__func__, name, r);
> > -
> > -	return r;
> > -}
> > -
> >  int dm_is_mpath(const char *name)
> >  {
> >  	char uuid[DM_UUID_LEN];
> > diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> > index ed5e866..cb5151e 100644
> > --- a/libmultipath/devmapper.h
> > +++ b/libmultipath/devmapper.h
> > @@ -126,8 +126,6 @@ int dm_simplecmd_noflush (int task, const char
> > *name, uint16_t udev_flags);
> >  int dm_addmap_create (struct multipath *mpp, char *params);
> >  int dm_addmap_reload (struct multipath *mpp, char *params, int
> > flush);
> >  int dm_map_present_by_wwid(const char *uuid);
> > -int dm_get_map(const char *name, unsigned long long *size, char
> > **outparams);
> > -int dm_get_status(const char *name, char **outstatus);
> >  
> >  enum {
> >  	DM_IS_MPATH_NO,
> > diff --git a/libmultipath/structs_vec.c
> > b/libmultipath/structs_vec.c
> > index ccc4efc..770e498 100644
> > --- a/libmultipath/structs_vec.c
> > +++ b/libmultipath/structs_vec.c
> > @@ -484,30 +484,32 @@ int
> >  update_multipath_table (struct multipath *mpp, vector pathvec, int
> > flags)
> >  {
> >  	int r = DMP_ERR;
> > -	char *params = NULL;
> > +	char __attribute__((cleanup(cleanup_charp))) *params =
> > NULL;
> > +	char __attribute__((cleanup(cleanup_charp))) *status =
> > NULL;
> >  
> >  	if (!mpp)
> >  		return r;
> >  
> > -	r = dm_get_map(mpp->alias, &mpp->size, &params);
> 
> Shouldn't we update mpp->size like we were with dm_get_map()?

Yes. Thanks for catching this.
Although if we do we should probably check whether it changed...

Martin