Re: [dm-devel] [PATCH 4/6] Unit tests for is_path_valid()

2020-05-18 Thread Benjamin Marzinski
On Fri, May 15, 2020 at 08:37:23PM +, Martin Wilck wrote:
> On Thu, 2020-05-14 at 20:59 -0500, Benjamin Marzinski wrote:
> > Signed-off-by: Benjamin Marzinski 
> 
> Two minor nits below, otherwise ack.

Sure. I can fix those up.

-Ben

> 
> > ---
> >  tests/Makefile |   4 +-
> >  tests/valid.c  | 424
> > +
> >  2 files changed, 427 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/valid.c
> > 
> > diff --git a/tests/Makefile b/tests/Makefile
> > index 1b8706a7..125553b8 100644
> > --- a/tests/Makefile
> > +++ b/tests/Makefile
> > @@ -13,7 +13,7 @@ CFLAGS += $(BIN_CFLAGS) -I$(multipathdir)
> > -I$(mpathcmddir) \
> >  LIBDEPS += -L$(multipathdir) -L$(mpathcmddir) -lmultipath -lmpathcmd
> > -lcmocka
> >  
> >  TESTS := uevent parser util dmevents hwtable blacklist unaligned vpd
> > pgpolicy \
> > -alias directio
> > +alias directio valid
> >  
> >  .SILENT: $(TESTS:%=%.o)
> >  .PRECIOUS: $(TESTS:%=%-test)
> > @@ -50,6 +50,8 @@ vpd-test_OBJDEPS :=  ../libmultipath/discovery.o
> >  vpd-test_LIBDEPS := -ludev -lpthread -ldl
> >  alias-test_TESTDEPS := test-log.o
> >  alias-test_LIBDEPS := -lpthread -ldl
> > +valid-test_OBJDEPS := ../libmultipath/valid.o
> > +valid-test_LIBDEPS := -ludev -lpthread -ldl
> >  ifneq ($(DIO_TEST_DEV),)
> >  directio-test_LIBDEPS := -laio
> >  endif
> > diff --git a/tests/valid.c b/tests/valid.c
> > new file mode 100644
> > index ..b128b029
> > --- /dev/null
> > +++ b/tests/valid.c
> > @@ -0,0 +1,424 @@
> > +/*
> > + * Copyright (c) 2020 Benjamin Marzinski, Redhat
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU 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 General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <
> > https://www.gnu.org/licenses/>;.
> > + *
> > + */
> > +
> > +#define _GNU_SOURCE
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include "globals.c"
> > +#include "util.h"
> > +#include "discovery.h"
> > +#include "wwids.h"
> > +#include "valid.h"
> > +
> > +int test_fd;
> > +struct udev_device {
> > +   int unused;
> > +} test_udev;
> > +
> > +bool __wrap_sysfs_is_multipathed(struct path *pp, bool set_wwid)
> > +{
> > +   bool is_multipathed = mock_type(bool);
> > +   assert_non_null(pp);
> > +   assert_int_not_equal(strlen(pp->dev), 0);
> > +   if (is_multipathed && set_wwid)
> > +   strlcpy(pp->wwid, mock_ptr_type(char *), WWID_SIZE);
> > +   return is_multipathed;
> > +}
> > +
> > +int __wrap___mpath_connect(int nonblocking)
> > +{
> > +   bool connected = mock_type(bool);
> > +   assert_int_equal(nonblocking, 1);
> > +   if (connected)
> > +   return test_fd;
> > +   errno = mock_type(int);
> > +   return -1;
> > +}
> > +
> > +int __wrap_systemd_service_enabled(const char *dev)
> > +{
> > +   return (int)mock_type(bool);
> > +}
> > +
> > +/* There's no point in checking the return value here */
> > +int __wrap_mpath_disconnect(int fd)
> > +{
> > +   assert_int_equal(fd, test_fd);
> > +   return 0;
> > +}
> > +
> > +struct udev_device
> > *__wrap_udev_device_new_from_subsystem_sysname(struct udev *udev,
> > const char *subsystem, const char *sysname)
> > +{
> > +   bool passed = mock_type(bool);
> > +   assert_string_equal(sysname, mock_ptr_type(char *));
> > +   if (passed)
> > +   return _udev;
> > +   return NULL;
> > +}
> > +
> > +int __wrap_pathinfo(struct path *pp, struct config *conf, int mask)
> > +{
> > +   int ret = mock_type(int);
> > +   assert_string_equal(pp->dev, mock_ptr_type(char *));
> > +   assert_int_equal(mask, DI_SYSFS | DI_WWID | DI_BLACKLIST);
> > +   if (ret == PATHINFO_OK)
> > +   strlcpy(pp->wwid, mock_ptr_type(char *), WWID_SIZE);
> > +   else
> > +   memset(pp->wwid, 0, WWID_SIZE);
> > +   return ret;
> > +}
> > +
> > +int __wrap_is_failed_wwid(const char *wwid)
> > +{
> > +   int ret = mock_type(int);
> > +   assert_string_equal(wwid, mock_ptr_type(char *));
> > +   return ret;
> > +}
> > +
> > +int __wrap_check_wwids_file(char *wwid, int write_wwid)
> > +{
> > +   bool passed = mock_type(bool);
> > +   assert_int_equal(write_wwid, 0);
> > +   assert_string_equal(wwid, mock_ptr_type(char *));
> > +   if (passed)
> > +   return 0;
> > +   else
> > +   return -1;
> > +}
> > +
> > +int __wrap_dm_map_present_by_uuid(const char *uuid)
> > +{
> > +   int ret = mock_type(int);
> > +   assert_string_equal(uuid, 

Re: [dm-devel] [PATCH 4/6] Unit tests for is_path_valid()

2020-05-16 Thread Martin Wilck
On Fri, 2020-05-15 at 22:37 +0200, Martin Wilck wrote:
> On Thu, 2020-05-14 at 20:59 -0500, Benjamin Marzinski wrote:
> > Signed-off-by: Benjamin Marzinski 
> 
> Two minor nits below, otherwise ack.
> 
> > ---
> >  tests/Makefile |   4 +-
> >  tests/valid.c  | 424
> > +
> >  2 files changed, 427 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/valid.c
> > 
> > diff --git a/tests/Makefile b/tests/Makefile
> > index 1b8706a7..125553b8 100644
> > --- a/tests/Makefile
> > +++ b/tests/Makefile
> > @@ -13,7 +13,7 @@ CFLAGS += $(BIN_CFLAGS) -I$(multipathdir)
> > -I$(mpathcmddir) \
> >  LIBDEPS += -L$(multipathdir) -L$(mpathcmddir) -lmultipath
> > -lmpathcmd
> > -lcmocka
> >  
> >  TESTS := uevent parser util dmevents hwtable blacklist unaligned
> > vpd
> > pgpolicy \
> > -alias directio
> > +alias directio valid
> >  
> >  .SILENT: $(TESTS:%=%.o)
> >  .PRECIOUS: $(TESTS:%=%-test)
> > @@ -50,6 +50,8 @@ vpd-test_OBJDEPS :=  ../libmultipath/discovery.o
> >  vpd-test_LIBDEPS := -ludev -lpthread -ldl
> >  alias-test_TESTDEPS := test-log.o
> >  alias-test_LIBDEPS := -lpthread -ldl
> > +valid-test_OBJDEPS := ../libmultipath/valid.o
> > +valid-test_LIBDEPS := -ludev -lpthread -ldl
> >  ifneq ($(DIO_TEST_DEV),)
> >  directio-test_LIBDEPS := -laio
> >  endif
> > diff --git a/tests/valid.c b/tests/valid.c
> > new file mode 100644
> > index ..b128b029
> > --- /dev/null
> > +++ b/tests/valid.c
> > @@ -0,0 +1,424 @@
> > +/*
> > + * Copyright (c) 2020 Benjamin Marzinski, Redhat
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU 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 General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public
> > License
> > + * along with this program.  If not, see <
> > https://www.gnu.org/licenses/>;;.
> > + *
> > + */
> > +
> > +#define _GNU_SOURCE
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include "globals.c"
> > +#include "util.h"
> > +#include "discovery.h"
> > +#include "wwids.h"
> > +#include "valid.h"
> > +
> > +int test_fd;
> > +struct udev_device {
> > +   int unused;
> > +} test_udev;
> > +
> > +bool __wrap_sysfs_is_multipathed(struct path *pp, bool set_wwid)
> > +{
> > +   bool is_multipathed = mock_type(bool);
> > +   assert_non_null(pp);

One general remark, just occured to me as I was looking at cmocka for a
different project: Perhaps we should rather use cmockas's
check_expected() functionality for testing parameter validity than the
assert...() macros.

We haven't done this consequently so far, but we could try to improve.

Regards
Martin


-- 
Dr. Martin Wilck , 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@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel



Re: [dm-devel] [PATCH 4/6] Unit tests for is_path_valid()

2020-05-15 Thread Martin Wilck
On Thu, 2020-05-14 at 20:59 -0500, Benjamin Marzinski wrote:
> Signed-off-by: Benjamin Marzinski 

Two minor nits below, otherwise ack.

> ---
>  tests/Makefile |   4 +-
>  tests/valid.c  | 424
> +
>  2 files changed, 427 insertions(+), 1 deletion(-)
>  create mode 100644 tests/valid.c
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index 1b8706a7..125553b8 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -13,7 +13,7 @@ CFLAGS += $(BIN_CFLAGS) -I$(multipathdir)
> -I$(mpathcmddir) \
>  LIBDEPS += -L$(multipathdir) -L$(mpathcmddir) -lmultipath -lmpathcmd
> -lcmocka
>  
>  TESTS := uevent parser util dmevents hwtable blacklist unaligned vpd
> pgpolicy \
> -  alias directio
> +  alias directio valid
>  
>  .SILENT: $(TESTS:%=%.o)
>  .PRECIOUS: $(TESTS:%=%-test)
> @@ -50,6 +50,8 @@ vpd-test_OBJDEPS :=  ../libmultipath/discovery.o
>  vpd-test_LIBDEPS := -ludev -lpthread -ldl
>  alias-test_TESTDEPS := test-log.o
>  alias-test_LIBDEPS := -lpthread -ldl
> +valid-test_OBJDEPS := ../libmultipath/valid.o
> +valid-test_LIBDEPS := -ludev -lpthread -ldl
>  ifneq ($(DIO_TEST_DEV),)
>  directio-test_LIBDEPS := -laio
>  endif
> diff --git a/tests/valid.c b/tests/valid.c
> new file mode 100644
> index ..b128b029
> --- /dev/null
> +++ b/tests/valid.c
> @@ -0,0 +1,424 @@
> +/*
> + * Copyright (c) 2020 Benjamin Marzinski, Redhat
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU 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 General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <
> https://www.gnu.org/licenses/>;.
> + *
> + */
> +
> +#define _GNU_SOURCE
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "globals.c"
> +#include "util.h"
> +#include "discovery.h"
> +#include "wwids.h"
> +#include "valid.h"
> +
> +int test_fd;
> +struct udev_device {
> + int unused;
> +} test_udev;
> +
> +bool __wrap_sysfs_is_multipathed(struct path *pp, bool set_wwid)
> +{
> + bool is_multipathed = mock_type(bool);
> + assert_non_null(pp);
> + assert_int_not_equal(strlen(pp->dev), 0);
> + if (is_multipathed && set_wwid)
> + strlcpy(pp->wwid, mock_ptr_type(char *), WWID_SIZE);
> + return is_multipathed;
> +}
> +
> +int __wrap___mpath_connect(int nonblocking)
> +{
> + bool connected = mock_type(bool);
> + assert_int_equal(nonblocking, 1);
> + if (connected)
> + return test_fd;
> + errno = mock_type(int);
> + return -1;
> +}
> +
> +int __wrap_systemd_service_enabled(const char *dev)
> +{
> + return (int)mock_type(bool);
> +}
> +
> +/* There's no point in checking the return value here */
> +int __wrap_mpath_disconnect(int fd)
> +{
> + assert_int_equal(fd, test_fd);
> + return 0;
> +}
> +
> +struct udev_device
> *__wrap_udev_device_new_from_subsystem_sysname(struct udev *udev,
> const char *subsystem, const char *sysname)
> +{
> + bool passed = mock_type(bool);
> + assert_string_equal(sysname, mock_ptr_type(char *));
> + if (passed)
> + return _udev;
> + return NULL;
> +}
> +
> +int __wrap_pathinfo(struct path *pp, struct config *conf, int mask)
> +{
> + int ret = mock_type(int);
> + assert_string_equal(pp->dev, mock_ptr_type(char *));
> + assert_int_equal(mask, DI_SYSFS | DI_WWID | DI_BLACKLIST);
> + if (ret == PATHINFO_OK)
> + strlcpy(pp->wwid, mock_ptr_type(char *), WWID_SIZE);
> + else
> + memset(pp->wwid, 0, WWID_SIZE);
> + return ret;
> +}
> +
> +int __wrap_is_failed_wwid(const char *wwid)
> +{
> + int ret = mock_type(int);
> + assert_string_equal(wwid, mock_ptr_type(char *));
> + return ret;
> +}
> +
> +int __wrap_check_wwids_file(char *wwid, int write_wwid)
> +{
> + bool passed = mock_type(bool);
> + assert_int_equal(write_wwid, 0);
> + assert_string_equal(wwid, mock_ptr_type(char *));
> + if (passed)
> + return 0;
> + else
> + return -1;
> +}
> +
> +int __wrap_dm_map_present_by_uuid(const char *uuid)
> +{
> + int ret = mock_type(int);
> + assert_string_equal(uuid, mock_ptr_type(char *));
> + return ret;
> +}
> +
> +enum {
> + STAGE_IS_MULTIPATHED,
> + STAGE_CHECK_MULTIPATHD,
> + STAGE_GET_UDEV_DEVICE,
> + STAGE_PATHINFO,
> + STAGE_IS_FAILED,
> + STAGE_CHECK_WWIDS,
> + STAGE_UUID_PRESENT,
> +};

nice :-)

> +
> +/* setup the test to continue past the given 

[dm-devel] [PATCH 4/6] Unit tests for is_path_valid()

2020-05-14 Thread Benjamin Marzinski
Signed-off-by: Benjamin Marzinski 
---
 tests/Makefile |   4 +-
 tests/valid.c  | 424 +
 2 files changed, 427 insertions(+), 1 deletion(-)
 create mode 100644 tests/valid.c

diff --git a/tests/Makefile b/tests/Makefile
index 1b8706a7..125553b8 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -13,7 +13,7 @@ CFLAGS += $(BIN_CFLAGS) -I$(multipathdir) -I$(mpathcmddir) \
 LIBDEPS += -L$(multipathdir) -L$(mpathcmddir) -lmultipath -lmpathcmd -lcmocka
 
 TESTS := uevent parser util dmevents hwtable blacklist unaligned vpd pgpolicy \
-alias directio
+alias directio valid
 
 .SILENT: $(TESTS:%=%.o)
 .PRECIOUS: $(TESTS:%=%-test)
@@ -50,6 +50,8 @@ vpd-test_OBJDEPS :=  ../libmultipath/discovery.o
 vpd-test_LIBDEPS := -ludev -lpthread -ldl
 alias-test_TESTDEPS := test-log.o
 alias-test_LIBDEPS := -lpthread -ldl
+valid-test_OBJDEPS := ../libmultipath/valid.o
+valid-test_LIBDEPS := -ludev -lpthread -ldl
 ifneq ($(DIO_TEST_DEV),)
 directio-test_LIBDEPS := -laio
 endif
diff --git a/tests/valid.c b/tests/valid.c
new file mode 100644
index ..b128b029
--- /dev/null
+++ b/tests/valid.c
@@ -0,0 +1,424 @@
+/*
+ * Copyright (c) 2020 Benjamin Marzinski, Redhat
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#define _GNU_SOURCE
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "globals.c"
+#include "util.h"
+#include "discovery.h"
+#include "wwids.h"
+#include "valid.h"
+
+int test_fd;
+struct udev_device {
+   int unused;
+} test_udev;
+
+bool __wrap_sysfs_is_multipathed(struct path *pp, bool set_wwid)
+{
+   bool is_multipathed = mock_type(bool);
+   assert_non_null(pp);
+   assert_int_not_equal(strlen(pp->dev), 0);
+   if (is_multipathed && set_wwid)
+   strlcpy(pp->wwid, mock_ptr_type(char *), WWID_SIZE);
+   return is_multipathed;
+}
+
+int __wrap___mpath_connect(int nonblocking)
+{
+   bool connected = mock_type(bool);
+   assert_int_equal(nonblocking, 1);
+   if (connected)
+   return test_fd;
+   errno = mock_type(int);
+   return -1;
+}
+
+int __wrap_systemd_service_enabled(const char *dev)
+{
+   return (int)mock_type(bool);
+}
+
+/* There's no point in checking the return value here */
+int __wrap_mpath_disconnect(int fd)
+{
+   assert_int_equal(fd, test_fd);
+   return 0;
+}
+
+struct udev_device *__wrap_udev_device_new_from_subsystem_sysname(struct udev 
*udev, const char *subsystem, const char *sysname)
+{
+   bool passed = mock_type(bool);
+   assert_string_equal(sysname, mock_ptr_type(char *));
+   if (passed)
+   return _udev;
+   return NULL;
+}
+
+int __wrap_pathinfo(struct path *pp, struct config *conf, int mask)
+{
+   int ret = mock_type(int);
+   assert_string_equal(pp->dev, mock_ptr_type(char *));
+   assert_int_equal(mask, DI_SYSFS | DI_WWID | DI_BLACKLIST);
+   if (ret == PATHINFO_OK)
+   strlcpy(pp->wwid, mock_ptr_type(char *), WWID_SIZE);
+   else
+   memset(pp->wwid, 0, WWID_SIZE);
+   return ret;
+}
+
+int __wrap_is_failed_wwid(const char *wwid)
+{
+   int ret = mock_type(int);
+   assert_string_equal(wwid, mock_ptr_type(char *));
+   return ret;
+}
+
+int __wrap_check_wwids_file(char *wwid, int write_wwid)
+{
+   bool passed = mock_type(bool);
+   assert_int_equal(write_wwid, 0);
+   assert_string_equal(wwid, mock_ptr_type(char *));
+   if (passed)
+   return 0;
+   else
+   return -1;
+}
+
+int __wrap_dm_map_present_by_uuid(const char *uuid)
+{
+   int ret = mock_type(int);
+   assert_string_equal(uuid, mock_ptr_type(char *));
+   return ret;
+}
+
+enum {
+   STAGE_IS_MULTIPATHED,
+   STAGE_CHECK_MULTIPATHD,
+   STAGE_GET_UDEV_DEVICE,
+   STAGE_PATHINFO,
+   STAGE_IS_FAILED,
+   STAGE_CHECK_WWIDS,
+   STAGE_UUID_PRESENT,
+};
+
+/* setup the test to continue past the given stage in is_path_valid() */
+static void setup_passing(char *name, char *wwid, bool check_multipathd,
+ unsigned int stage)
+{
+   will_return(__wrap_sysfs_is_multipathed, false);
+   if (stage == STAGE_IS_MULTIPATHED)
+   return;
+   if (check_multipathd)
+