* [PATCH 2/6] multipath: add comments
2018-03-30 3:36 [PATCH 0/6] multipath cleanup patches Benjamin Marzinski
2018-03-30 3:36 ` [PATCH 1/6] multipathd: remove incorrect pthread_testcancel Benjamin Marzinski
@ 2018-03-30 3:36 ` Benjamin Marzinski
2018-04-04 15:34 ` Martin Wilck
2018-03-30 3:37 ` [PATCH 3/6] multipathd: minor dmevents polling code cleanups Benjamin Marzinski
` (3 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Benjamin Marzinski @ 2018-03-30 3:36 UTC (permalink / raw)
To: device-mapper development; +Cc: Martin Wilck
This commit simply adds a number of comments based on suggestions by
Martin Wilck.
Cc: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
multipathd/dmevents.c | 4 ++-
tests/dmevents.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++-
tests/util.c | 2 ++
3 files changed, 76 insertions(+), 2 deletions(-)
diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
index 2281a10..0b0d0ce 100644
--- a/multipathd/dmevents.c
+++ b/multipathd/dmevents.c
@@ -122,6 +122,8 @@ static int arm_dm_event_poll(int fd)
dmi.version[0] = DM_VERSION_MAJOR;
dmi.version[1] = DM_VERSION_MINOR;
dmi.version[2] = DM_VERSION_PATCHLEVEL;
+ /* This flag currently does nothing. It simply exists to
+ * duplicate the behavior of libdevmapper */
dmi.flags = 0x4;
dmi.data_start = offsetof(struct dm_ioctl, data);
dmi.data_size = sizeof(dmi);
@@ -189,7 +191,7 @@ fail:
return -1;
}
-/* You must call update_multipath() after calling this function, to
+/* You must call __setup_multipath() after calling this function, to
* deal with any events that came in before the device was added */
int watch_dmevents(char *name)
{
diff --git a/tests/dmevents.c b/tests/dmevents.c
index 4442fc2..bba51dc 100644
--- a/tests/dmevents.c
+++ b/tests/dmevents.c
@@ -33,10 +33,13 @@
/* I have to do this to get at the static variables */
#include "../multipathd/dmevents.c"
+/* pretend dm device */
struct dm_device {
char name[WWID_SIZE];
+ /* is this a mpath device, or other dm device */
int is_mpath;
uint32_t evt_nr;
+ /* tracks the event number when the multipath device was updated */
uint32_t update_nr;
};
@@ -48,6 +51,9 @@ struct test_data {
struct test_data data;
+/* Add a pretend dm device, or update its event number. This is used to build
+ * up the dm devices that the dmevents code queries with dm_task_get_names,
+ * dm_geteventnr, and dm_is_mpath */
int add_dm_device_event(char *name, int is_mpath, uint32_t evt_nr)
{
struct dm_device *dev;
@@ -77,6 +83,7 @@ int add_dm_device_event(char *name, int is_mpath, uint32_t evt_nr)
return 0;
}
+/* helper function for pretend dm devices */
struct dm_device *find_dm_device(const char *name)
{
struct dm_device *dev;
@@ -88,6 +95,7 @@ struct dm_device *find_dm_device(const char *name)
return NULL;
}
+/* helper function for pretend dm devices */
int remove_dm_device_event(const char *name)
{
struct dm_device *dev;
@@ -103,6 +111,7 @@ int remove_dm_device_event(const char *name)
return -1;
}
+/* helper function for pretend dm devices */
void remove_all_dm_device_events(void)
{
struct dm_device *dev;
@@ -122,7 +131,9 @@ static inline void *align_ptr(void *ptr)
return (void *)align_val((size_t)ptr);
}
-/* copied off of list_devices in dm-ioctl.c */
+/* copied off of list_devices in dm-ioctl.c except that it uses
+ * the pretend dm devices, and saves the output to the test_data
+ * structure */
struct dm_names *build_dm_names(void)
{
struct dm_names *names, *np, *old_np = NULL;
@@ -199,12 +210,16 @@ int __wrap_open(const char *pathname, int flags)
return mock_type(int);
}
+/* We never check the result of the close(), so there's no need to
+ * to mock a return value */
int __wrap_close(int fd)
{
assert_int_equal(fd, waiter->fd);
return 0;
}
+/* the pretend dm device code checks the input and supplies the
+ * return value, so there's no need to do that here */
int __wrap_dm_is_mpath(const char *name)
{
struct dm_device *dev;
@@ -216,6 +231,8 @@ int __wrap_dm_is_mpath(const char *name)
return 0;
}
+/* either get return info from the pretend dm device, or
+ * override it to test -1 return */
int __wrap_dm_geteventnr(const char *name)
{
struct dm_device *dev;
@@ -257,6 +274,8 @@ int __wrap_dm_task_run(struct dm_task *dmt)
return mock_type(int);
}
+/* either get return info from the pretend dm device, or
+ * override it to test NULL return */
struct dm_names * __wrap_dm_task_get_names(struct dm_task *dmt)
{
int good = mock_type(int);
@@ -299,6 +318,9 @@ void __wrap_remove_map_by_alias(const char *alias, struct vectors * vecs,
assert_int_equal(purge_vec, 1);
}
+/* pretend update the pretend dm devices. If fail is set, it
+ * simulates having the dm device removed. Otherwise it just sets
+ * update_nr to record when the update happened */
int __wrap_update_multipath(struct vectors *vecs, char *mapname, int reset)
{
int fail;
@@ -325,6 +347,9 @@ int __wrap_update_multipath(struct vectors *vecs, char *mapname, int reset)
return fail;
}
+/* helper function used to check if the dmevents list of devices
+ * includes a specific device. To make sure that dmevents is
+ * in the correct state after running a function */
struct dev_event *find_dmevents(const char *name)
{
struct dev_event *dev_evt;
@@ -336,14 +361,19 @@ struct dev_event *find_dmevents(const char *name)
return NULL;
}
+/* null vecs pointer when initialized dmevents */
static void test_init_waiter_bad0(void **state)
{
+ /* this boilerplate code just skips the test if
+ * dmevents polling is not supported */
struct test_data *datap = (struct test_data *)(*state);
if (datap == NULL)
skip();
+
assert_int_equal(init_dmevent_waiter(NULL), -1);
}
+/* fail to open /dev/mapper/control */
static void test_init_waiter_bad1(void **state)
{
struct test_data *datap = (struct test_data *)(*state);
@@ -354,6 +384,7 @@ static void test_init_waiter_bad1(void **state)
assert_ptr_equal(waiter, NULL);
}
+/* waiter remains initialized after this test */
static void test_init_waiter_good0(void **state)
{
struct test_data *datap = (struct test_data *)(*state);
@@ -364,6 +395,7 @@ static void test_init_waiter_good0(void **state)
assert_ptr_not_equal(waiter, NULL);
}
+/* No dm device named foo */
static void test_watch_dmevents_bad0(void **state)
{
struct test_data *datap = (struct test_data *)(*state);
@@ -373,6 +405,7 @@ static void test_watch_dmevents_bad0(void **state)
assert_ptr_equal(find_dmevents("foo"), NULL);
}
+/* foo is not a multipath device */
static void test_watch_dmevents_bad1(void **state)
{
struct test_data *datap = (struct test_data *)(*state);
@@ -384,6 +417,7 @@ static void test_watch_dmevents_bad1(void **state)
assert_ptr_equal(find_dmevents("foo"), NULL);
}
+/* failed getting the dmevent number */
static void test_watch_dmevents_bad2(void **state)
{
struct test_data *datap = (struct test_data *)(*state);
@@ -396,6 +430,8 @@ static void test_watch_dmevents_bad2(void **state)
assert_int_equal(watch_dmevents("foo"), -1);
assert_ptr_equal(find_dmevents("foo"), NULL);
}
+
+/* verify that you can watch and unwatch dm multipath device "foo" */
static void test_watch_dmevents_good0(void **state)
{
struct dev_event *dev_evt;
@@ -407,16 +443,20 @@ static void test_watch_dmevents_good0(void **state)
assert_int_equal(add_dm_device_event("foo", 1, 5), 0);
will_return(__wrap_dm_geteventnr, 0);
assert_int_equal(watch_dmevents("foo"), 0);
+ /* verify foo is being watched */
dev_evt = find_dmevents("foo");
assert_ptr_not_equal(dev_evt, NULL);
assert_int_equal(dev_evt->evt_nr, 5);
assert_int_equal(dev_evt->action, EVENT_NOTHING);
assert_int_equal(VECTOR_SIZE(waiter->events), 1);
unwatch_dmevents("foo");
+ /* verify foo is no longer being watched */
assert_int_equal(VECTOR_SIZE(waiter->events), 0);
assert_ptr_equal(find_dmevents("foo"), NULL);
}
+/* verify that if you try to watch foo multiple times, it only
+ * is placed on the waiter list once */
static void test_watch_dmevents_good1(void **state)
{
struct dev_event *dev_evt;
@@ -445,6 +485,7 @@ static void test_watch_dmevents_good1(void **state)
assert_ptr_equal(find_dmevents("foo"), NULL);
}
+/* watch and then unwatch multiple devices */
static void test_watch_dmevents_good2(void **state)
{
struct dev_event *dev_evt;
@@ -480,6 +521,7 @@ static void test_watch_dmevents_good2(void **state)
assert_ptr_equal(find_dmevents("bar"), NULL);
}
+/* dm_task_create fails */
static void test_get_events_bad0(void **state)
{
struct test_data *datap = (struct test_data *)(*state);
@@ -493,6 +535,7 @@ static void test_get_events_bad0(void **state)
assert_int_equal(dm_get_events(), -1);
}
+/* dm_task_run fails */
static void test_get_events_bad1(void **state)
{
struct test_data *datap = (struct test_data *)(*state);
@@ -505,6 +548,7 @@ static void test_get_events_bad1(void **state)
assert_int_equal(dm_get_events(), -1);
}
+/* dm_task_get_names fails */
static void test_get_events_bad2(void **state)
{
struct test_data *datap = (struct test_data *)(*state);
@@ -518,6 +562,7 @@ static void test_get_events_bad2(void **state)
assert_int_equal(dm_get_events(), -1);
}
+/* If the device isn't being watched, dm_get_events returns NULL */
static void test_get_events_good0(void **state)
{
struct test_data *datap = (struct test_data *)(*state);
@@ -534,6 +579,11 @@ static void test_get_events_good0(void **state)
assert_int_equal(VECTOR_SIZE(waiter->events), 0);
}
+/* There are 5 dm devices. 4 of them are multipath devices.
+ * Only 3 of them are being watched. "foo" has a new event
+ * "xyzzy" gets removed. Nothing happens to bar. Verify
+ * that all the events are properly set, and that nothing
+ * happens with the two devices that aren't being watched */
static void test_get_events_good1(void **state)
{
struct dev_event *dev_evt;
@@ -577,6 +627,8 @@ static void test_get_events_good1(void **state)
assert_int_equal(VECTOR_SIZE(waiter->events), 3);
}
+/* poll does not return an event. nothing happens. The
+ * devices remain after this test */
static void test_dmevent_loop_bad0(void **state)
{
struct dm_device *dev;
@@ -603,6 +655,7 @@ static void test_dmevent_loop_bad0(void **state)
assert_int_equal(dev->update_nr, 5);
}
+/* arm_dm_event_poll's ioctl fails. Nothing happens */
static void test_dmevent_loop_bad1(void **state)
{
struct dm_device *dev;
@@ -624,6 +677,7 @@ static void test_dmevent_loop_bad1(void **state)
assert_int_equal(dev->update_nr, 5);
}
+/* dm_get_events fails. Nothing happens */
static void test_dmevent_loop_bad2(void **state)
{
struct dm_device *dev;
@@ -646,6 +700,8 @@ static void test_dmevent_loop_bad2(void **state)
assert_int_equal(dev->update_nr, 5);
}
+/* verify dmevent_loop runs successfully when no devices are being
+ * watched */
static void test_dmevent_loop_good0(void **state)
{
struct test_data *datap = (struct test_data *)(*state);
@@ -663,6 +719,11 @@ static void test_dmevent_loop_good0(void **state)
assert_int_equal(dmevent_loop(), 1);
}
+/* Watch 3 devices, where one device has an event (foo), one device is
+ * removed (xyzzy), and one device does nothing (bar). Verify that
+ * the device with the event gets updated, the device that is removed
+ * gets unwatched, and the device with no events stays the same.
+ * The devices remain after this test */
static void test_dmevent_loop_good1(void **state)
{
struct dm_device *dev;
@@ -717,6 +778,10 @@ static void test_dmevent_loop_good1(void **state)
assert_ptr_equal(find_dm_device("xyzzy"), NULL);
}
+/* watch another dm device and add events to two of them, so bar and
+ * baz have new events, and foo doesn't. Set update_multipath to
+ * fail for baz. Verify that baz is unwatched, bar is updated, and
+ * foo stays the same. */
static void test_dmevent_loop_good2(void **state)
{
struct dm_device *dev;
@@ -762,6 +827,8 @@ static void test_dmevent_loop_good2(void **state)
assert_ptr_equal(find_dm_device("baz"), NULL);
}
+/* remove dm device foo, and unwatch events on bar. Verify that
+ * foo is cleaned up and unwatched, and bar is no longer updated */
static void test_dmevent_loop_good3(void **state)
{
struct dm_device *dev;
@@ -790,6 +857,8 @@ static void test_dmevent_loop_good3(void **state)
assert_ptr_equal(find_dm_device("foo"), NULL);
}
+
+/* verify that rearming the dmevents polling works */
static void test_arm_poll(void **state)
{
struct test_data *datap = (struct test_data *)(*state);
@@ -799,6 +868,7 @@ static void test_arm_poll(void **state)
assert_int_equal(arm_dm_event_poll(waiter->fd), 0);
}
+/* verify that the waiter is cleaned up */
static void test_cleanup_waiter(void **state)
{
struct test_data *datap = (struct test_data *)(*state);
diff --git a/tests/util.c b/tests/util.c
index e9a004f..113b134 100644
--- a/tests/util.c
+++ b/tests/util.c
@@ -74,6 +74,8 @@ static void test_basenamecpy_good5(void **state)
assert_string_equal(dst, "bar");
}
+/* multipath expects any trailing whitespace to be stripped off the basename,
+ * so that it will match pp->dev */
static void test_basenamecpy_good6(void **state)
{
char dst[6];
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 4/6] multipathd: remove unneeded function parameter
2018-03-30 3:36 [PATCH 0/6] multipath cleanup patches Benjamin Marzinski
` (2 preceding siblings ...)
2018-03-30 3:37 ` [PATCH 3/6] multipathd: minor dmevents polling code cleanups Benjamin Marzinski
@ 2018-03-30 3:37 ` Benjamin Marzinski
2018-04-04 15:50 ` Martin Wilck
2018-03-30 3:37 ` [PATCH 5/6] mpathcmd: fix libmpathcmd license Benjamin Marzinski
2018-03-30 3:37 ` [PATCH 6/6] libmultipath: don't print undefined values Benjamin Marzinski
5 siblings, 1 reply; 12+ messages in thread
From: Benjamin Marzinski @ 2018-03-30 3:37 UTC (permalink / raw)
To: device-mapper development; +Cc: Martin Wilck
remove_map_and_stop_waiter was always called with purge_vecs = 1, so
it can simply be removed, as suggested by Martin Wilck
Cc: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
multipathd/main.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index 9a4f671..841d3e9 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -311,14 +311,13 @@ wait_for_events(struct multipath *mpp, struct vectors *vecs)
}
static void
-remove_map_and_stop_waiter(struct multipath *mpp, struct vectors *vecs,
- int purge_vec)
+remove_map_and_stop_waiter(struct multipath *mpp, struct vectors *vecs)
{
/* devices are automatically removed by the dmevent polling code,
* so they don't need to be manually removed here */
if (!poll_dmevents)
stop_waiter_thread(mpp, vecs);
- remove_map(mpp, vecs, purge_vec);
+ remove_map(mpp, vecs, PURGE_VEC);
}
static void
@@ -400,7 +399,7 @@ int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
return 0;
out:
- remove_map_and_stop_waiter(mpp, vecs, PURGE_VEC);
+ remove_map_and_stop_waiter(mpp, vecs);
return 1;
}
@@ -637,7 +636,7 @@ flush_map(struct multipath * mpp, struct vectors * vecs, int nopaths)
}
orphan_paths(vecs->pathvec, mpp);
- remove_map_and_stop_waiter(mpp, vecs, 1);
+ remove_map_and_stop_waiter(mpp, vecs);
return 0;
}
@@ -769,7 +768,7 @@ uev_remove_map (struct uevent * uev, struct vectors * vecs)
}
orphan_paths(vecs->pathvec, mpp);
- remove_map_and_stop_waiter(mpp, vecs, 1);
+ remove_map_and_stop_waiter(mpp, vecs);
out:
lock_cleanup_pop(vecs->lock);
FREE(alias);
@@ -1154,7 +1153,7 @@ out:
return retval;
fail:
- remove_map_and_stop_waiter(mpp, vecs, 1);
+ remove_map_and_stop_waiter(mpp, vecs);
return 1;
}
@@ -1612,7 +1611,7 @@ mpvec_garbage_collector (struct vectors * vecs)
vector_foreach_slot (vecs->mpvec, mpp, i) {
if (mpp && mpp->alias && !dm_map_present(mpp->alias)) {
condlog(2, "%s: remove dead map", mpp->alias);
- remove_map_and_stop_waiter(mpp, vecs, 1);
+ remove_map_and_stop_waiter(mpp, vecs);
i--;
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 6/6] libmultipath: don't print undefined values
2018-03-30 3:36 [PATCH 0/6] multipath cleanup patches Benjamin Marzinski
` (4 preceding siblings ...)
2018-03-30 3:37 ` [PATCH 5/6] mpathcmd: fix libmpathcmd license Benjamin Marzinski
@ 2018-03-30 3:37 ` Benjamin Marzinski
5 siblings, 0 replies; 12+ messages in thread
From: Benjamin Marzinski @ 2018-03-30 3:37 UTC (permalink / raw)
To: device-mapper development; +Cc: Martin Wilck
commit 48e9fd9f ("libmultipath: parser: use call-by-value for "snprint"
methods") removed some of the code that checked for undefined values to
avoid printing them. This lead to device configurations that printed
out values for parameters that they hadn't configured. This patch adds
that code back in.
Fixes: 48e9fd9f ("libmultipath: parser: use call-by-value for "snprint" methods")
Cc: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/dict.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index ac9216c..1a18337 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -101,12 +101,16 @@ print_int (char *buff, int len, long v)
static int
print_nonzero (char *buff, int len, long v)
{
+ if (!v)
+ return 0;
return snprintf(buff, len, "%li", v);
}
static int
print_str (char *buff, int len, const char *ptr)
{
+ if (!ptr)
+ return 0;
return snprintf(buff, len, "\"%s\"", ptr);
}
@@ -120,6 +124,8 @@ print_yes_no (char *buff, int len, long v)
static int
print_yes_no_undef (char *buff, int len, long v)
{
+ if (!v)
+ return 0;
return snprintf(buff, len, "\"%s\"",
(v == YNU_NO)? "no" : "yes");
}
@@ -665,6 +671,8 @@ set_dev_loss(vector strvec, void *ptr)
int
print_dev_loss(char * buff, int len, unsigned long v)
{
+ if (!v)
+ return 0;
if (v >= MAX_DEV_LOSS_TMO)
return snprintf(buff, len, "\"infinity\"");
return snprintf(buff, len, "%lu", v);
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread