On Tue, Apr 28, 2020 at 09:42:52PM +0000, Martin Wilck wrote:
> On Fri, 2020-04-03 at 01:50 -0500, Benjamin Marzinski wrote:
> > +int mpath_get_mode(void)
> > +{
> > + int mode;
> > + struct config *conf;
> > +
> > + conf = load_config(DEFAULT_CONFIGFILE);
>
> By using this, you'd pull in a substantial part of libmultipath.
> Why don't you simply rely on the passed-in mode value?
>
> Actually, I wonder if we could split libmultipath into a part "below"
> libmpathvalid and a part "above" libmpathvalid. I wouldn't put
> "load_config()" in the "below" part. The problematic part is
> pathinfo(), which has to be "below" and which would pull in quite a bit
> of libmultipath functionality.
I would love to be able to not load the config, but I don't see how
that's possible. The config tells us things like what's blacklisted,
what find_multipaths value is, etc. If the library wants to get the
same result as multipathd, it needs to use the config. Or are you
suggesting something else here?
> > + if (!conf)
> > + return -1;
> > + mode = get_conf_mode(conf);
> > + free_config(conf);
> > + return mode;
> > +}
> > +
> > +/*
> > + * name: name of path to check
> > + * mode: mode to use for determination. MPATH_DEFAULT uses
> > configured mode
> > + * info: on success, contains the path wwid
> > + * paths: array of the returned mpath_info from other claimed paths
> > + * nr_paths: the size of the paths array
> > + */
> > +int
> > +mpath_is_path(const char *name, unsigned int mode, struct mpath_info
> > *info,
> > + struct mpath_info **paths_arg, unsigned int nr_paths)
>
> I wonder if you couldn't use a vector of "struct path*" instead
> of "struct mpath_info*". But well, I the data structures can be
> simply transformed into each other either way, so that's not a big
> issue.
I want to use a array of the same things we are passing back to the
caller. The idea is that the SID collects the mpath infos from earlier
calls, and passes them back here to help mpath_valid pick devices
correctly. And like you said, it's easy to build that array with the
wwids from existing stuct paths.
> > +{
> > + struct config *conf;
> > + struct path * pp;
> > + int r = MPATH_IS_ERROR;
> > + int fd = -1;
> > + unsigned int i, version[3];
> > + bool already_multipathed = false;
> > + /* stupid c implicit conversion rules fail */
> > + const struct mpath_info * const *paths = (const struct
> > mpath_info * const *)paths_arg;
> > +
> > + if (info)
> > + memset(info, 0, sizeof(struct mpath_info));
> > +
> > + if (!name || mode >= MPATH_MAX_MODE)
> > + return r;
> > +
> > + if (nr_paths > 0 && !paths)
> > + return r;
> > +
> > + skip_libmp_dm_init();
> > + udev = udev_new();
> > + if (!udev)
> > + goto out;
> > + conf = load_config(DEFAULT_CONFIGFILE);
> > + if (!conf)
> > + goto out_udev;
> > + conf->verbosity = -1;
>
> Wouldn't this basically preclude calling the function from "multipath
> -u", or any other place in multipath-tools assuming standard library
> initialization? I'd like to split this off into some wrapper.
Yes, to be useful for things other than SID (or even to play nicely with
SIDs verbosity settings), this needs to be configurable.
> > + if (mode == MPATH_DEFAULT)
> > + mode = get_conf_mode(conf);
> > +
> > + if (dm_prereq(version))
> > + goto out_config;
> > + memcpy(conf->version, version, sizeof(version));
> > + multipath_conf = conf;
> > +
> > + pp = alloc_path();
> > + if (!pp)
> > + goto out_config;
> > +
> > + if (safe_sprintf(pp->dev, "%s", name))
> > + goto out_path;
> > +
> > + if (sysfs_is_multipathed(pp, true)) {
> > + if (!info || pp->wwid[0] != '\0') {
> > + r = MPATH_IS_VALID;
> > + goto out_path;
> > + }
> > + already_multipathed = true;
>
> This is the "WWID overflow" case? I believe multipathd would never
> create a map with an WWID longer than WWID_SIZE, and thus this
> condition should be treated as an error.
Sure.
-Ben
> Other than that, you've done a magnificent job in making the logic
> of this function easy to understand. I'd love to replace the current
> "multipath -u" logic with this.
>
> > + }
> > +
> > + fd = __mpath_connect(1);
> > + if (fd < 0) {
> > + if (errno != EAGAIN && !systemd_service_enabled(name))
> > {
> > + r = MPATH_IS_NOT_VALID;
> > + goto out_path;
> > + }
> > + } else
> > + mpath_disconnect(fd);
> > +
> > + pp->udev = udev_device_new_from_subsystem_sysname(udev,
> > "block", name);
> > + if (!pp->udev)
> > + goto out_path;
> > +
> > + r = pathinfo(pp, conf, DI_SYSFS | DI_WWID | DI_BLACKLIST);
> > + if (r) {
> > + if (r == PATHINFO_SKIPPED)
> > + r = MPATH_IS_NOT_VALID;
> > + else
> > + r = MPATH_IS_ERROR;
> > + goto out_path;
> > + }
> > +
> > + if (pp->wwid[0] == '\0') {
> > + r = MPATH_IS_NOT_VALID;
> > + goto out_path;
> > + }
> > +
> > + if (already_multipathed)
> > + goto out_path;
> > +
> > + if (dm_map_present_by_uuid(pp->wwid) == 1) {
> > + r = MPATH_IS_VALID;
> > + goto out_path;
> > + }
> > +
> > + r = is_failed_wwid(pp->wwid);
> > + if (r != WWID_IS_NOT_FAILED) {
> > + if (r == WWID_IS_FAILED)
> > + r = MPATH_IS_NOT_VALID;
> > + else
> > + r = MPATH_IS_ERROR;
> > + goto out_path;
> > + }
> > +
> > + if (mode == MPATH_GREEDY) {
> > + r = MPATH_IS_VALID;
> > + goto out_path;
> > + }
> > +
> > + if (check_wwids_file(pp->wwid, 0) == 0) {
> > + r = MPATH_IS_VALID;
> > + goto out_path;
> > + }
> > +
> > + if (mode == MPATH_STRICT) {
> > + r = MPATH_IS_NOT_VALID;
> > + goto out_path;
> > + }
>
> It seems I misunderstood you before. This MPATH_STRICT logic looks
> fine.
>
> > +
> > + for (i = 0; i < nr_paths; i++) {
> > + if (strncmp(paths[i]->wwid, pp->wwid, 128) == 0) {
> > + r = MPATH_IS_VALID;
> > + goto out_path;
> > + }
> > + }
> > +
> > + /* mode == MPATH_SMART || mode == MPATH_FIND */
> > + r = MPATH_IS_MAYBE_VALID;
> > +
> > +out_path:
> > + if (already_multipathed)
> > + r = MPATH_IS_VALID;
> > + if (info && (r == MPATH_IS_VALID || r == MPATH_IS_MAYBE_VALID))
> > + strlcpy(info->wwid, pp->wwid, 128);
> > + free_path(pp);
> > +out_config:
> > + free_config(conf);
> > +out_udev:
> > + udev_unref(udev);
> > +out:
> > + return r;
> > +}
> > diff --git a/libmpathvalid/mpath_valid.h
> > b/libmpathvalid/mpath_valid.h
> > new file mode 100644
> > index 00000000..f832beff
> > --- /dev/null
> > +++ b/libmpathvalid/mpath_valid.h
> > @@ -0,0 +1,56 @@
> > +/*
> > + * Copyright (C) 2015 Red Hat, Inc.
> > + *
> > + * This file is part of the device-mapper multipath userspace tools.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > License
> > + * along with this program. If not, see <
> > http://www.gnu.org/licenses/>;.
> > + */
> > +
> > +#ifndef LIB_MPATH_VALID_H
> > +#define LIB_MPATH_VALID_H
> > +
> > +#ifdef __cpluscplus
> > +extern "C" {
> > +#endif
> > +
> > +enum mpath_valid_mode {
> > + MPATH_DEFAULT,
> > + MPATH_STRICT,
> > + MPATH_FIND,
> > + MPATH_SMART,
> > + MPATH_GREEDY,
> > + MPATH_MAX_MODE, /* used only for bounds checking */
> > +};
> > +
> > +enum mpath_valid_result {
> > + MPATH_IS_ERROR = -1,
> > + MPATH_IS_NOT_VALID,
> > + MPATH_IS_VALID,
> > + MPATH_IS_MAYBE_VALID,
> > +};
> > +
> > +struct mpath_info {
> > + char wwid[128];
> > +};
> > +
> > +int mpath_get_mode(void);
> > +int mpath_is_path(const char *name, unsigned int mode, struct
> > mpath_info *info,
> > + struct mpath_info **paths, unsigned int nr_paths);
> > +
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +#endif /* LIB_PATH_VALID_H */
> > +
> > diff --git a/libmultipath/Makefile b/libmultipath/Makefile
> > index ad690a49..7e2c00cf 100644
> > --- a/libmultipath/Makefile
> > +++ b/libmultipath/Makefile
> > @@ -69,6 +69,7 @@ nvme-ioctl.h: nvme/nvme-ioctl.h
> >
> > $(LIBS): $(OBJS)
> > $(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ -o $@ $(OBJS)
> > $(LIBDEPS)
> > + ar rcs libmultipath_nonshared.a $(OBJS)
> > $(LN) $@ $(DEVLIB)
> >
> > install:
> > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> > index 7ed494a1..92f61133 100644
> > --- a/libmultipath/devmapper.c
> > +++ b/libmultipath/devmapper.c
> > @@ -770,6 +770,55 @@ out:
> > return r;
> > }
> >
> > +/*
> > + * Return
> > + * 1 : map with uuid exists
> > + * 0 : map with uuid doesn't exist
> > + * -1 : error
> > + */
> > +int
> > +dm_map_present_by_uuid(const char *uuid)
> > +{
> > + struct dm_task *dmt;
> > + struct dm_info info;
> > + char prefixed_uuid[WWID_SIZE + UUID_PREFIX_LEN];
> > + int r = -1;
> > +
> > + if (!uuid || uuid[0] == '\0')
> > + return 0;
> > +
> > + if (safe_sprintf(prefixed_uuid, UUID_PREFIX "%s", uuid))
> > + goto out;
> > +
> > + if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> > + goto out;
> > +
> > + dm_task_no_open_count(dmt);
> > +
> > + if (!dm_task_set_uuid(dmt, prefixed_uuid))
> > + goto out_task;
> > +
> > + if (!dm_task_run(dmt))
> > + goto out_task;
> > +
> > + if (!dm_task_get_info(dmt, &info))
> > + goto out_task;
> > +
> > + r = 0;
> > +
> > + if (!info.exists)
> > + goto out_task;
> > +
> > + r = 1;
> > +out_task:
> > + dm_task_destroy(dmt);
> > +out:
> > + if (r < 0)
> > + condlog(3, "%s: dm command failed in %s: %s", uuid,
> > + __FUNCTION__, strerror(errno));
> > + return r;
> > +}
> > +
> > static int
> > dm_dev_t (const char * mapname, char * dev_t, int len)
> > {
> > diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> > index 17fc9faf..5ed7edc5 100644
> > --- a/libmultipath/devmapper.h
> > +++ b/libmultipath/devmapper.h
> > @@ -39,6 +39,7 @@ int dm_simplecmd_noflush (int, const char *,
> > uint16_t);
> > int dm_addmap_create (struct multipath *mpp, char *params);
> > int dm_addmap_reload (struct multipath *mpp, char *params, int
> > flush);
> > int dm_map_present (const char *);
> > +int dm_map_present_by_uuid(const char *uuid);
> > int dm_get_map(const char *, unsigned long long *, char *);
> > int dm_get_status(const char *, char *);
> > int dm_type(const char *, char *);
>
> --
> Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107
> SUSE Software Solutions Germany GmbH
> HRB 36809, AG Nürnberg GF: Felix
> Imendörffer
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
