[dm-devel] [PATCH 00/14] multipath: fixes for sysfs accessors

[dm-devel] [PATCH 00/14] multipath: fixes for sysfs accessors

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This set fixes some strangeness in our sysfs accessors which I
found while looking at
https://github.com/opensvc/multipath-tools/issues/35#issuecomment-1175901745.
(The patches don't fix this issue, which seems to be related to
Debian's initramfs setup).

Most importantly, sysfs_attr_get_value() and sysfs_attr_set_value()
would return 0 if the number of bytes read/written was different from
the expected value, which is non-standard and unexpected. This
series changes the return value semantics of these functions:

 - in sysfs_attr_get_value(), if a read buffer is too small to hold
   the string read plus a terminating 0 byte, the return value
   equals the buffer size.
 - in sysfs_bin_attr_get_value(), no 0 bytes are appended. It's not
   an error if the read buffer is completely filled, and no warning
   is printed in this case.
 - sysfs_attr_set_value() always returns the number of bytes written
   unless an error occured in open() or write().

Tests for the new semantics are added. Moreover, the sysfs.c code
is slightly refactored to avoid code duplication.

Martin Wilck (14):
  libmultipath: alua: remove get_sysfs_pg83()
  libmultipath: remove sysfs_get_binary()
  libmultipath: sysfs_bin_attr_get_value(): no error if buffer is filled
  libmultipath: common code path for sysfs_attr_get_value()
  libmultipath: sanitize error checking in sysfs accessors
  libmultipath: get rid of PATH_SIZE
  libmultipath: sysfs_attr_get_value(): don't return 0 if buffer too
    small
  libmultipath: sysfs_attr_set_value(): don't return 0 on partial write
  libmultipath: sysfs: cleanup file descriptors on pthread_cancel()
  libmultipath, multipathd: log failure setting sysfs attributes
  multipath tests: expect_condlog: skip depending on verbosity
  multipath tests: __wrap_dlog: print log message
  multipath tests: add sysfs test
  libmultipath.version: bump version for sysfs accessors

 libmultipath/configure.c              |  30 +-
 libmultipath/discovery.c              | 120 +++----
 libmultipath/libmultipath.version     |   8 +-
 libmultipath/prioritizers/alua_rtpg.c |  57 +--
 libmultipath/propsel.c                |   6 +-
 libmultipath/structs.h                |   3 -
 libmultipath/sysfs.c                  | 191 ++++------
 libmultipath/sysfs.h                  |  23 ++
 libmultipath/util.c                   |   8 +-
 multipathd/cli_handlers.c             |   2 +-
 multipathd/fpin_handlers.c            |  11 +-
 multipathd/main.c                     |  40 ++-
 tests/Makefile                        |   5 +-
 tests/sysfs.c                         | 494 ++++++++++++++++++++++++++
 tests/test-lib.c                      |   1 -
 tests/test-log.c                      |   5 +
 16 files changed, 751 insertions(+), 253 deletions(-)
 create mode 100644 tests/sysfs.c

-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 01/14] libmultipath: alua: remove get_sysfs_pg83()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Since b72e753 ("libmultipath: alua: try to retrieve inquiry data from sysfs"),
we fetch inquiry and VPD data from sysfs anyway. No need to do this twice.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/prioritizers/alua_rtpg.c | 57 ++++++++-------------------
 1 file changed, 16 insertions(+), 41 deletions(-)

diff --git a/libmultipath/prioritizers/alua_rtpg.c b/libmultipath/prioritizers/alua_rtpg.c
index 3f9c0e7..4db13c2 100644
--- a/libmultipath/prioritizers/alua_rtpg.c
+++ b/libmultipath/prioritizers/alua_rtpg.c
@@ -228,25 +228,6 @@ get_target_port_group_support(const struct path *pp, unsigned int timeout)
 	return rc;
 }
 
-static int
-get_sysfs_pg83(const struct path *pp, unsigned char *buff, int buflen)
-{
-	struct udev_device *parent = pp->udev;
-
-	while (parent) {
-		const char *subsys = udev_device_get_subsystem(parent);
-		if (subsys && !strncmp(subsys, "scsi", 4))
-			break;
-		parent = udev_device_get_parent(parent);
-	}
-
-	if (!parent || sysfs_get_vpd(parent, 0x83, buff, buflen) <= 0) {
-		PRINT_DEBUG("failed to read sysfs vpd pg83");
-		return -1;
-	}
-	return 0;
-}
-
 int
 get_target_port_group(const struct path * pp, unsigned int timeout)
 {
@@ -265,32 +246,26 @@ get_target_port_group(const struct path * pp, unsigned int timeout)
 	}
 
 	memset(buf, 0, buflen);
+	rc = do_inquiry(pp, 1, 0x83, buf, buflen, timeout);
+	if (rc < 0)
+		goto out;
 
-	rc = get_sysfs_pg83(pp, buf, buflen);
-
-	if (rc < 0) {
+	scsi_buflen = get_unaligned_be16(&buf[2]) + 4;
+	if (scsi_buflen >= USHRT_MAX)
+		scsi_buflen = USHRT_MAX;
+	if (buflen < scsi_buflen) {
+		free(buf);
+		buf = (unsigned char *)malloc(scsi_buflen);
+		if (!buf) {
+			PRINT_DEBUG("malloc failed: could not allocate"
+				    "%u bytes", scsi_buflen);
+			return -RTPG_RTPG_FAILED;
+		}
+		buflen = scsi_buflen;
+		memset(buf, 0, buflen);
 		rc = do_inquiry(pp, 1, 0x83, buf, buflen, timeout);
 		if (rc < 0)
 			goto out;
-
-		scsi_buflen = get_unaligned_be16(&buf[2]) + 4;
-		/* Paranoia */
-		if (scsi_buflen >= USHRT_MAX)
-			scsi_buflen = USHRT_MAX;
-		if (buflen < scsi_buflen) {
-			free(buf);
-			buf = (unsigned char *)malloc(scsi_buflen);
-			if (!buf) {
-				PRINT_DEBUG("malloc failed: could not allocate"
-					    "%u bytes", scsi_buflen);
-				return -RTPG_RTPG_FAILED;
-			}
-			buflen = scsi_buflen;
-			memset(buf, 0, buflen);
-			rc = do_inquiry(pp, 1, 0x83, buf, buflen, timeout);
-			if (rc < 0)
-				goto out;
-		}
 	}
 
 	vpd83 = (struct vpd83_data *) buf;
-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 02/14] libmultipath: remove sysfs_get_binary()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This function adds no value on top of sysfs_bin_attr_get_value().
Remove it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 26 ++------------------------
 tests/test-lib.c         |  1 -
 2 files changed, 2 insertions(+), 25 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 0d8a558..7e09e4e 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -263,41 +263,19 @@ declare_sysfs_get_str(vendor);
 declare_sysfs_get_str(model);
 declare_sysfs_get_str(rev);
 
-static ssize_t
-sysfs_get_binary (struct udev_device * udev, const char *attrname,
-		  unsigned char *buff, size_t len)
-{
-	ssize_t attr_len;
-	const char * devname;
-
-	if (!udev) {
-		condlog(3, "No udev device given\n");
-		return -ENOSYS;
-	}
-
-	devname = udev_device_get_sysname(udev);
-	attr_len = sysfs_bin_attr_get_value(udev, attrname, buff, len);
-	if (attr_len < 0) {
-		condlog(3, "%s: attribute %s not found in sysfs",
-			devname, attrname);
-		return attr_len;
-	}
-	return attr_len;
-}
-
 ssize_t sysfs_get_vpd(struct udev_device * udev, unsigned char pg,
 		      unsigned char *buff, size_t len)
 {
 	char attrname[9];
 
 	snprintf(attrname, sizeof(attrname), "vpd_pg%02x", pg);
-	return sysfs_get_binary(udev, attrname, buff, len);
+	return sysfs_bin_attr_get_value(udev, attrname, buff, len);
 }
 
 ssize_t sysfs_get_inquiry(struct udev_device * udev,
 			  unsigned char *buff, size_t len)
 {
-	return sysfs_get_binary(udev, "inquiry", buff, len);
+	return sysfs_bin_attr_get_value(udev, "inquiry", buff, len);
 }
 
 int
diff --git a/tests/test-lib.c b/tests/test-lib.c
index 6dd3ee8..0bc49d5 100644
--- a/tests/test-lib.c
+++ b/tests/test-lib.c
@@ -334,7 +334,6 @@ void mock_pathinfo(int mask, const struct mocked_path *mp)
 	if (mask & DI_SERIAL) {
 		will_return(__wrap_udev_device_get_subsystem, "scsi");
 		will_return(__wrap_udev_device_get_sysname, hbtl);
-		will_return(__wrap_udev_device_get_sysname, hbtl);
 	}
 
 	if (mask & DI_WWID) {
-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 03/14] libmultipath: sysfs_bin_attr_get_value(): no error if buffer is filled

[mwilck]

From: Martin Wilck <mwilck@suse.com>

sysfs_bin_attr_get_value() sets the length of bytes read to 0
when the provided buffer was too small, truncating potentially
useful data. This is harmful e.g. in do_inquiry(), if the "inquiry"
sysfs attribute contains more than 96 bytes (which is possible).

Actually, binary attributes don't need to be 0-terminated. Thus,
unlike for string attributes, it's not an error if the requested number of
bytes is exactly equal to the number of bytes read. We expect that
the caller knows how many bytes it needs to read.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 10 ++++++----
 libmultipath/sysfs.c     |  5 +----
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 7e09e4e..f5b8401 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1341,13 +1341,15 @@ static int
 get_vpd_sysfs (struct udev_device *parent, int pg, char * str, int maxlen)
 {
 	int len;
-	size_t buff_len;
+	ssize_t buff_len;
 	unsigned char buff[VPD_BUFLEN];
 
 	memset(buff, 0x0, VPD_BUFLEN);
-	if (!parent || sysfs_get_vpd(parent, pg, buff, VPD_BUFLEN) <= 0) {
-		condlog(3, "failed to read sysfs vpd pg%02x", pg);
-		return -EINVAL;
+	buff_len = sysfs_get_vpd(parent, pg, buff, VPD_BUFLEN);
+	if (buff_len < 0) {
+		condlog(3, "failed to read sysfs vpd pg%02x: %s",
+			pg, strerror(-buff_len));
+		return buff_len;
 	}
 
 	if (buff[1] != pg) {
diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index f45dbee..3ec9251 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -146,10 +146,7 @@ ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char *attr_name,
 	if (size < 0) {
 		condlog(4, "read from %s failed: %s", devpath, strerror(errno));
 		size = -errno;
-	} else if (size == (ssize_t)value_len) {
-		condlog(4, "overflow while reading from %s", devpath);
-		size = 0;
-	}
+	};
 
 	close(fd);
 	return size;
-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 04/14] libmultipath: common code path for sysfs_attr_get_value()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The code for sysfs_attr_get_value and sysfs_bin_attr_get_value() was
almost identical. Use a common code path.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/sysfs.c | 70 +++++++++++---------------------------------
 1 file changed, 17 insertions(+), 53 deletions(-)

diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index 3ec9251..4db911c 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -44,8 +44,8 @@
  * as libudev lacks the capability to update an attribute value.
  * So for modified attributes we need to implement our own function.
  */
-ssize_t sysfs_attr_get_value(struct udev_device *dev, const char *attr_name,
-			     char * value, size_t value_len)
+static ssize_t __sysfs_attr_get_value(struct udev_device *dev, const char *attr_name,
+				      char *value, size_t value_len, bool binary)
 {
 	char devpath[PATH_SIZE];
 	struct stat statbuf;
@@ -87,12 +87,14 @@ ssize_t sysfs_attr_get_value(struct udev_device *dev, const char *attr_name,
 	if (size < 0) {
 		condlog(4, "read from %s failed: %s", devpath, strerror(errno));
 		size = -errno;
-		value[0] = '\0';
-	} else if (size == (ssize_t)value_len) {
+		if (!binary)
+			value[0] = '\0';
+	} else if (!binary && size == (ssize_t)value_len) {
+		condlog(3, "%s: overflow reading from %s (required len: %zu)",
+			__func__, devpath, size);
 		value[size - 1] = '\0';
-		condlog(4, "overflow while reading from %s", devpath);
 		size = 0;
-	} else {
+	} else if (!binary) {
 		value[size] = '\0';
 		size = strchop(value);
 	}
@@ -101,55 +103,17 @@ ssize_t sysfs_attr_get_value(struct udev_device *dev, const char *attr_name,
 	return size;
 }
 
-ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char *attr_name,
-				 unsigned char * value, size_t value_len)
+ssize_t sysfs_attr_get_value(struct udev_device *dev, const char *attr_name,
+			     char *value, size_t value_len)
 {
-	char devpath[PATH_SIZE];
-	struct stat statbuf;
-	int fd;
-	ssize_t size = -1;
+	return __sysfs_attr_get_value(dev, attr_name, value, value_len, false);
+}
 
-	if (!dev || !attr_name || !value)
-		return 0;
-
-	snprintf(devpath, PATH_SIZE, "%s/%s", udev_device_get_syspath(dev),
-		 attr_name);
-	condlog(4, "open '%s'", devpath);
-	/* read attribute value */
-	fd = open(devpath, O_RDONLY);
-	if (fd < 0) {
-		condlog(4, "attribute '%s' can not be opened: %s",
-			devpath, strerror(errno));
-		return -errno;
-	}
-	if (fstat(fd, &statbuf) != 0) {
-		condlog(4, "stat '%s' failed: %s", devpath, strerror(errno));
-		close(fd);
-		return -ENXIO;
-	}
-
-	/* skip directories */
-	if (S_ISDIR(statbuf.st_mode)) {
-		condlog(4, "%s is a directory", devpath);
-		close(fd);
-		return -EISDIR;
-	}
-
-	/* skip non-writeable files */
-	if ((statbuf.st_mode & S_IRUSR) == 0) {
-		condlog(4, "%s is not readable", devpath);
-		close(fd);
-		return -EPERM;
-	}
-
-	size = read(fd, value, value_len);
-	if (size < 0) {
-		condlog(4, "read from %s failed: %s", devpath, strerror(errno));
-		size = -errno;
-	};
-
-	close(fd);
-	return size;
+ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char *attr_name,
+				 unsigned char *value, size_t value_len)
+{
+	return __sysfs_attr_get_value(dev, attr_name, (char *)value,
+				      value_len, true);
 }
 
 ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 05/14] libmultipath: sanitize error checking in sysfs accessors

[mwilck]

From: Martin Wilck <mwilck@suse.com>

udev_device_get_syspath() may return NULL; check for it, and check
for pathname overflow. Disallow a zero buffer length. The fstat()
calls were superfluous, as a read() or write() on the fd would
return the respective error codes depending on file type or permissions,
the extra system call and code complexity adds no value.

Log levels should be moderate in sysfs.c, because it depends
on the caller whether errors getting/setting certain attributes are
fatal.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/sysfs.c | 94 ++++++++++++++++++--------------------------
 1 file changed, 39 insertions(+), 55 deletions(-)

diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index 4db911c..1f0f207 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -47,46 +47,38 @@
 static ssize_t __sysfs_attr_get_value(struct udev_device *dev, const char *attr_name,
 				      char *value, size_t value_len, bool binary)
 {
+	const char *syspath;
 	char devpath[PATH_SIZE];
-	struct stat statbuf;
 	int fd;
 	ssize_t size = -1;
 
-	if (!dev || !attr_name || !value)
-		return 0;
+	if (!dev || !attr_name || !value || !value_len) {
+		condlog(1, "%s: invalid parameters", __func__);
+		return -EINVAL;
+	}
 
-	snprintf(devpath, PATH_SIZE, "%s/%s", udev_device_get_syspath(dev),
-		 attr_name);
+	syspath = udev_device_get_syspath(dev);
+	if (!syspath) {
+		condlog(3, "%s: invalid udevice", __func__);
+		return -EINVAL;
+	}
+	if (safe_sprintf(devpath, "%s/%s", syspath, attr_name)) {
+		condlog(3, "%s: devpath overflow", __func__);
+		return -EOVERFLOW;
+	}
 	condlog(4, "open '%s'", devpath);
 	/* read attribute value */
 	fd = open(devpath, O_RDONLY);
 	if (fd < 0) {
-		condlog(4, "attribute '%s' can not be opened: %s",
-			devpath, strerror(errno));
+		condlog(3, "%s: attribute '%s' can not be opened: %s",
+			__func__, devpath, strerror(errno));
 		return -errno;
 	}
-	if (fstat(fd, &statbuf) < 0) {
-		condlog(4, "stat '%s' failed: %s", devpath, strerror(errno));
-		close(fd);
-		return -ENXIO;
-	}
-	/* skip directories */
-	if (S_ISDIR(statbuf.st_mode)) {
-		condlog(4, "%s is a directory", devpath);
-		close(fd);
-		return -EISDIR;
-	}
-	/* skip non-writeable files */
-	if ((statbuf.st_mode & S_IRUSR) == 0) {
-		condlog(4, "%s is not readable", devpath);
-		close(fd);
-		return -EPERM;
-	}
-
 	size = read(fd, value, value_len);
 	if (size < 0) {
-		condlog(4, "read from %s failed: %s", devpath, strerror(errno));
 		size = -errno;
+		condlog(3, "%s: read from %s failed: %s", __func__, devpath,
+			strerror(errno));
 		if (!binary)
 			value[0] = '\0';
 	} else if (!binary && size == (ssize_t)value_len) {
@@ -119,51 +111,43 @@ ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char *attr_name,
 ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
 			     const char * value, size_t value_len)
 {
+	const char *syspath;
 	char devpath[PATH_SIZE];
-	struct stat statbuf;
 	int fd;
 	ssize_t size = -1;
 
-	if (!dev || !attr_name || !value || !value_len)
-		return 0;
+	if (!dev || !attr_name || !value || !value_len) {
+		condlog(1, "%s: invalid parameters", __func__);
+		return -EINVAL;
+	}
+
+	syspath = udev_device_get_syspath(dev);
+	if (!syspath) {
+		condlog(3, "%s: invalid udevice", __func__);
+		return -EINVAL;
+	}
+	if (safe_sprintf(devpath, "%s/%s", syspath, attr_name)) {
+		condlog(3, "%s: devpath overflow", __func__);
+		return -EOVERFLOW;
+	}
 
-	snprintf(devpath, PATH_SIZE, "%s/%s", udev_device_get_syspath(dev),
-		 attr_name);
 	condlog(4, "open '%s'", devpath);
 	/* write attribute value */
 	fd = open(devpath, O_WRONLY);
 	if (fd < 0) {
-		condlog(4, "attribute '%s' can not be opened: %s",
-			devpath, strerror(errno));
+		condlog(2, "%s: attribute '%s' can not be opened: %s",
+			__func__, devpath, strerror(errno));
 		return -errno;
 	}
-	if (fstat(fd, &statbuf) != 0) {
-		condlog(4, "stat '%s' failed: %s", devpath, strerror(errno));
-		close(fd);
-		return -errno;
-	}
-
-	/* skip directories */
-	if (S_ISDIR(statbuf.st_mode)) {
-		condlog(4, "%s is a directory", devpath);
-		close(fd);
-		return -EISDIR;
-	}
-
-	/* skip non-writeable files */
-	if ((statbuf.st_mode & S_IWUSR) == 0) {
-		condlog(4, "%s is not writeable", devpath);
-		close(fd);
-		return -EPERM;
-	}
 
 	size = write(fd, value, value_len);
 	if (size < 0) {
-		condlog(4, "write to %s failed: %s", devpath, strerror(errno));
 		size = -errno;
+		condlog(3, "%s: write to %s failed: %s", __func__, 
+			devpath, strerror(errno));
 	} else if (size < (ssize_t)value_len) {
-		condlog(4, "tried to write %ld to %s. Wrote %ld",
-			(long)value_len, devpath, (long)size);
+		condlog(3, "%s: underflow writing %zu bytes to %s. Wrote %zd bytes",
+			__func__, value_len, devpath, size);
 		size = 0;
 	}
 
-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 05/14] libmultipath: sanitize error checking in sysfs accessors

[Benjamin Marzinski]

On Wed, Jul 06, 2022 at 04:38:13PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> udev_device_get_syspath() may return NULL; check for it, and check
> for pathname overflow. Disallow a zero buffer length. The fstat()
> calls were superfluous, as a read() or write() on the fd would
> return the respective error codes depending on file type or permissions,
> the extra system call and code complexity adds no value.
> 
> Log levels should be moderate in sysfs.c, because it depends
> on the caller whether errors getting/setting certain attributes are
> fatal.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/sysfs.c | 94 ++++++++++++++++++--------------------------
>  1 file changed, 39 insertions(+), 55 deletions(-)
> 
> diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
> index 4db911c..1f0f207 100644
> --- a/libmultipath/sysfs.c
> +++ b/libmultipath/sysfs.c
> @@ -47,46 +47,38 @@
>  static ssize_t __sysfs_attr_get_value(struct udev_device *dev, const char *attr_name,
>  				      char *value, size_t value_len, bool binary)
>  {
> +	const char *syspath;
>  	char devpath[PATH_SIZE];
> -	struct stat statbuf;
>  	int fd;
>  	ssize_t size = -1;
>  
> -	if (!dev || !attr_name || !value)
> -		return 0;
> +	if (!dev || !attr_name || !value || !value_len) {
> +		condlog(1, "%s: invalid parameters", __func__);
> +		return -EINVAL;
> +	}
>  
> -	snprintf(devpath, PATH_SIZE, "%s/%s", udev_device_get_syspath(dev),
> -		 attr_name);
> +	syspath = udev_device_get_syspath(dev);
> +	if (!syspath) {
> +		condlog(3, "%s: invalid udevice", __func__);
> +		return -EINVAL;
> +	}
> +	if (safe_sprintf(devpath, "%s/%s", syspath, attr_name)) {
> +		condlog(3, "%s: devpath overflow", __func__);
> +		return -EOVERFLOW;
> +	}
>  	condlog(4, "open '%s'", devpath);
>  	/* read attribute value */
>  	fd = open(devpath, O_RDONLY);
>  	if (fd < 0) {
> -		condlog(4, "attribute '%s' can not be opened: %s",
> -			devpath, strerror(errno));
> +		condlog(3, "%s: attribute '%s' can not be opened: %s",
> +			__func__, devpath, strerror(errno));
>  		return -errno;
>  	}
> -	if (fstat(fd, &statbuf) < 0) {
> -		condlog(4, "stat '%s' failed: %s", devpath, strerror(errno));
> -		close(fd);
> -		return -ENXIO;
> -	}
> -	/* skip directories */
> -	if (S_ISDIR(statbuf.st_mode)) {
> -		condlog(4, "%s is a directory", devpath);
> -		close(fd);
> -		return -EISDIR;
> -	}
> -	/* skip non-writeable files */
> -	if ((statbuf.st_mode & S_IRUSR) == 0) {
> -		condlog(4, "%s is not readable", devpath);
> -		close(fd);
> -		return -EPERM;
> -	}
> -
>  	size = read(fd, value, value_len);
>  	if (size < 0) {
> -		condlog(4, "read from %s failed: %s", devpath, strerror(errno));
>  		size = -errno;
> +		condlog(3, "%s: read from %s failed: %s", __func__, devpath,
> +			strerror(errno));
>  		if (!binary)
>  			value[0] = '\0';
>  	} else if (!binary && size == (ssize_t)value_len) {
> @@ -119,51 +111,43 @@ ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char *attr_name,
>  ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
>  			     const char * value, size_t value_len)
>  {
> +	const char *syspath;
>  	char devpath[PATH_SIZE];
> -	struct stat statbuf;
>  	int fd;
>  	ssize_t size = -1;
>  
> -	if (!dev || !attr_name || !value || !value_len)
> -		return 0;
> +	if (!dev || !attr_name || !value || !value_len) {
> +		condlog(1, "%s: invalid parameters", __func__);
> +		return -EINVAL;
> +	}
> +
> +	syspath = udev_device_get_syspath(dev);
> +	if (!syspath) {
> +		condlog(3, "%s: invalid udevice", __func__);
> +		return -EINVAL;
> +	}
> +	if (safe_sprintf(devpath, "%s/%s", syspath, attr_name)) {
> +		condlog(3, "%s: devpath overflow", __func__);
> +		return -EOVERFLOW;
> +	}
>  
> -	snprintf(devpath, PATH_SIZE, "%s/%s", udev_device_get_syspath(dev),
> -		 attr_name);
>  	condlog(4, "open '%s'", devpath);
>  	/* write attribute value */
>  	fd = open(devpath, O_WRONLY);
>  	if (fd < 0) {
> -		condlog(4, "attribute '%s' can not be opened: %s",
> -			devpath, strerror(errno));
> +		condlog(2, "%s: attribute '%s' can not be opened: %s",
> +			__func__, devpath, strerror(errno));

You log at loglevel 2 here, but at 3 for an open() failure in
__sysfs_attr_get_value(). However I notice that this gets resolved in
PATCH 8/14, so it's no big deal.

-Ben

>  		return -errno;
>  	}
> -	if (fstat(fd, &statbuf) != 0) {
> -		condlog(4, "stat '%s' failed: %s", devpath, strerror(errno));
> -		close(fd);
> -		return -errno;
> -	}
> -
> -	/* skip directories */
> -	if (S_ISDIR(statbuf.st_mode)) {
> -		condlog(4, "%s is a directory", devpath);
> -		close(fd);
> -		return -EISDIR;
> -	}
> -
> -	/* skip non-writeable files */
> -	if ((statbuf.st_mode & S_IWUSR) == 0) {
> -		condlog(4, "%s is not writeable", devpath);
> -		close(fd);
> -		return -EPERM;
> -	}
>  
>  	size = write(fd, value, value_len);
>  	if (size < 0) {
> -		condlog(4, "write to %s failed: %s", devpath, strerror(errno));
>  		size = -errno;
> +		condlog(3, "%s: write to %s failed: %s", __func__, 
> +			devpath, strerror(errno));
>  	} else if (size < (ssize_t)value_len) {
> -		condlog(4, "tried to write %ld to %s. Wrote %ld",
> -			(long)value_len, devpath, (long)size);
> +		condlog(3, "%s: underflow writing %zu bytes to %s. Wrote %zd bytes",
> +			__func__, value_len, devpath, size);
>  		size = 0;
>  	}
>  
> -- 
> 2.36.1
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 06/14] libmultipath: get rid of PATH_SIZE

[mwilck]

From: Martin Wilck <mwilck@suse.com>

replace PATH_SIZE with the system limit PATH_MAX. In some places,
PATH_SIZE was used for file names. Use FILE_NAME_SIZE in these cases.
Also, use a constant for "multipathd.service" in systemd_service_enabled_in().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs.h    | 3 ---
 libmultipath/sysfs.c      | 8 ++++----
 libmultipath/util.c       | 8 +++++---
 multipathd/cli_handlers.c | 2 +-
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index a6a0944..dfa12ff 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -17,7 +17,6 @@
 #define FILE_NAME_SIZE		256
 #define CALLOUT_MAX_SIZE	256
 #define BLK_DEV_SIZE		33
-#define PATH_SIZE		512
 #define NAME_SIZE		512
 #define HOST_NAME_LEN		16
 #define SLOT_NAME_SIZE		40
@@ -519,6 +518,4 @@ int pathcmp (const struct pathgroup *, const struct pathgroup *);
 int add_feature (char **, const char *);
 int remove_feature (char **, const char *);
 
-extern char sysfs_path[PATH_SIZE];
-
 #endif /* _STRUCTS_H */
diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index 1f0f207..e48b05e 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -48,7 +48,7 @@ static ssize_t __sysfs_attr_get_value(struct udev_device *dev, const char *attr_
 				      char *value, size_t value_len, bool binary)
 {
 	const char *syspath;
-	char devpath[PATH_SIZE];
+	char devpath[PATH_MAX];
 	int fd;
 	ssize_t size = -1;
 
@@ -112,7 +112,7 @@ ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
 			     const char * value, size_t value_len)
 {
 	const char *syspath;
-	char devpath[PATH_SIZE];
+	char devpath[PATH_MAX];
 	int fd;
 	ssize_t size = -1;
 
@@ -184,7 +184,7 @@ sysfs_get_size (struct path *pp, unsigned long long * size)
 int sysfs_check_holders(char * check_devt, char * new_devt)
 {
 	unsigned int major, new_minor, table_minor;
-	char path[PATH_MAX], check_dev[PATH_SIZE];
+	char path[PATH_MAX], check_dev[FILE_NAME_SIZE];
 	char * table_name;
 	DIR *dirfd;
 	struct dirent *holder;
@@ -194,7 +194,7 @@ int sysfs_check_holders(char * check_devt, char * new_devt)
 		return 0;
 	}
 
-	if (devt2devname(check_dev, PATH_SIZE, check_devt)) {
+	if (devt2devname(check_dev, sizeof(check_dev), check_devt)) {
 		condlog(1, "can't get devname for %s", check_devt);
 		return 0;
 	}
diff --git a/libmultipath/util.c b/libmultipath/util.c
index ce5ea73..e7e7d4c 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -242,13 +242,15 @@ setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached)
 
 int systemd_service_enabled_in(const char *dev, const char *prefix)
 {
-	char path[PATH_SIZE], file[PATH_MAX], service[PATH_SIZE];
+	static const char service[] = "multipathd.service";
+	char path[PATH_MAX], file[PATH_MAX];
 	DIR *dirfd;
 	struct dirent *d;
 	int found = 0;
 
-	snprintf(service, PATH_SIZE, "multipathd.service");
-	snprintf(path, PATH_SIZE, "%s/systemd/system", prefix);
+	if (safe_sprintf(path, "%s/systemd/system", prefix))
+		return 0;
+
 	condlog(3, "%s: checking for %s in %s", dev, service, path);
 
 	dirfd = opendir(path);
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index d79cdd7..db4d441 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -688,7 +688,7 @@ cli_add_map (void * v, struct strbuf *reply, void * data)
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, MAP);
 	int major = -1, minor = -1;
-	char dev_path[PATH_SIZE];
+	char dev_path[FILE_NAME_SIZE];
 	char *refwwid, *alias = NULL;
 	int rc, count = 0;
 	struct config *conf;
-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 07/14] libmultipath: sysfs_attr_get_value(): don't return 0 if buffer too small

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If the passed read buffer is too small to hold the value read plus
terminating 0 byte, return the given size value rather than 0.

This way we get similar semantics as for sysfs_bin_attr_get_get_value(),
except that sysfs_attr_get_value() must always 0-terminate the value;
thus a return value equal to the length parameter is an error for
the non-binary case.

Provide a helper macro to test this "overflow" condition.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c |  2 +-
 libmultipath/discovery.c | 14 +++++++-------
 libmultipath/propsel.c   |  6 +++++-
 libmultipath/sysfs.c     |  3 +--
 libmultipath/sysfs.h     | 13 +++++++++++++
 multipathd/main.c        |  2 +-
 6 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 09ae708..467bbaa 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -589,7 +589,7 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int is_reload)
 		ret = sysfs_attr_get_value(udd, "queue/max_sectors_kb", buff,
 					   sizeof(buff));
 		udev_device_unref(udd);
-		if (ret <= 0) {
+		if (!sysfs_attr_value_ok(ret, sizeof(buff))) {
 			condlog(1, "failed to get current max_sectors_kb from %s", mpp->alias);
 			return 1;
 		}
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index f5b8401..54b1caf 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -560,10 +560,10 @@ sysfs_get_asymmetric_access_state(struct path *pp, char *buff, int buflen)
 	if (!parent)
 		return -1;
 
-	if (sysfs_attr_get_value(parent, "access_state", buff, buflen) <= 0)
+	if (!sysfs_attr_get_value_ok(parent, "access_state", buff, buflen))
 		return -1;
 
-	if (sysfs_attr_get_value(parent, "preferred_path", value, 16) <= 0)
+	if (!sysfs_attr_get_value_ok(parent, "preferred_path", value, sizeof(value)))
 		return 0;
 
 	preferred = strtoul(value, &eptr, 0);
@@ -638,8 +638,8 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp)
 	/*
 	 * read the current dev_loss_tmo value from sysfs
 	 */
-	ret = sysfs_attr_get_value(rport_dev, "dev_loss_tmo", value, 16);
-	if (ret <= 0) {
+	ret = sysfs_attr_get_value(rport_dev, "dev_loss_tmo", value, sizeof(value));
+	if (!sysfs_attr_value_ok(ret, sizeof(value))) {
 		condlog(0, "%s: failed to read dev_loss_tmo value, "
 			"error %d", rport_id, -ret);
 		goto out;
@@ -1737,8 +1737,8 @@ path_offline (struct path * pp)
 	}
 
 	memset(buff, 0x0, SCSI_STATE_SIZE);
-	err = sysfs_attr_get_value(parent, "state", buff, SCSI_STATE_SIZE);
-	if (err <= 0) {
+	err = sysfs_attr_get_value(parent, "state", buff, sizeof(buff));
+	if (!sysfs_attr_value_ok(err, sizeof(buff))) {
 		if (err == -ENXIO)
 			return PATH_REMOVED;
 		else
@@ -2142,7 +2142,7 @@ static ssize_t uid_fallback(struct path *pp, int path_state,
 			return -1;
 		len = sysfs_attr_get_value(pp->udev, "wwid", value,
 					   sizeof(value));
-		if (len <= 0)
+		if (!sysfs_attr_value_ok(len, sizeof(value)))
 			return -1;
 		len = strlcpy(pp->wwid, value, WWID_SIZE);
 		if (len >= WWID_SIZE) {
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 50d0b5c..18f2277 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -435,6 +435,7 @@ out:
 static int get_dh_state(struct path *pp, char *value, size_t value_len)
 {
 	struct udev_device *ud;
+	ssize_t rc;
 
 	if (pp->udev == NULL)
 		return -1;
@@ -444,7 +445,10 @@ static int get_dh_state(struct path *pp, char *value, size_t value_len)
 	if (ud == NULL)
 		return -1;
 
-	return sysfs_attr_get_value(ud, "dh_state", value, value_len);
+	rc = sysfs_attr_get_value(ud, "dh_state", value, value_len);
+	if (!sysfs_attr_value_ok(rc, value_len))
+		return -1;
+	return rc;
 }
 
 int select_hwhandler(struct config *conf, struct multipath *mp)
diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index e48b05e..125f1c2 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -85,7 +85,6 @@ static ssize_t __sysfs_attr_get_value(struct udev_device *dev, const char *attr_
 		condlog(3, "%s: overflow reading from %s (required len: %zu)",
 			__func__, devpath, size);
 		value[size - 1] = '\0';
-		size = 0;
 	} else if (!binary) {
 		value[size] = '\0';
 		size = strchop(value);
@@ -165,7 +164,7 @@ sysfs_get_size (struct path *pp, unsigned long long * size)
 		return 1;
 
 	attr[0] = '\0';
-	if (sysfs_attr_get_value(pp->udev, "size", attr, 255) <= 0) {
+	if (!sysfs_attr_get_value_ok(pp->udev, "size", attr, sizeof(attr))) {
 		condlog(3, "%s: No size attribute in sysfs", pp->dev);
 		return 1;
 	}
diff --git a/libmultipath/sysfs.h b/libmultipath/sysfs.h
index 72b39ab..cdc84e4 100644
--- a/libmultipath/sysfs.h
+++ b/libmultipath/sysfs.h
@@ -12,6 +12,19 @@ ssize_t sysfs_attr_get_value(struct udev_device *dev, const char *attr_name,
 			     char * value, size_t value_len);
 ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char *attr_name,
 				 unsigned char * value, size_t value_len);
+#define sysfs_attr_value_ok(rc, value_len)			\
+	({							\
+		ssize_t __r = rc;				\
+		__r >= 0 && (size_t)__r < (size_t)value_len;	\
+	})
+
+#define sysfs_attr_get_value_ok(dev, attr, val, len) \
+	({ \
+		size_t __l = (len);					\
+		ssize_t __rc = sysfs_attr_get_value(dev, attr, val, __l); \
+		sysfs_attr_value_ok(__rc, __l); \
+	})
+
 int sysfs_get_size (struct path *pp, unsigned long long * size);
 int sysfs_check_holders(char * check_devt, char * new_devt);
 bool sysfs_is_multipathed(struct path *pp, bool set_wwid);
diff --git a/multipathd/main.c b/multipathd/main.c
index 2f2b9d4..68eca92 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1126,7 +1126,7 @@ sysfs_get_ro (struct path *pp)
 	if (!pp->udev)
 		return -1;
 
-	if (sysfs_attr_get_value(pp->udev, "ro", buff, sizeof(buff)) <= 0) {
+	if (!sysfs_attr_get_value_ok(pp->udev, "ro", buff, sizeof(buff))) {
 		condlog(3, "%s: Cannot read ro attribute in sysfs", pp->dev);
 		return -1;
 	}
-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 08/14] libmultipath: sysfs_attr_set_value(): don't return 0 on partial write

[mwilck]

From: Martin Wilck <mwilck@suse.com>

sysfs_attr_set_value() returned 0 if not all requested bytes were written.
Change this to return the number of bytes written. Error checking is now
somewhat more involved; provide a helper macro fo it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 10 ++++--
 libmultipath/discovery.c | 70 ++++++++++++++++++++++++++--------------
 libmultipath/sysfs.c     |  6 ++--
 libmultipath/sysfs.h     | 10 ++++++
 4 files changed, 64 insertions(+), 32 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 467bbaa..0607dba 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -568,6 +568,7 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int is_reload)
 	struct pathgroup * pgp;
 	struct path *pp;
 	char buff[11];
+	ssize_t len;
 	int i, j, ret, err = 0;
 	struct udev_device *udd;
 	int max_sectors_kb;
@@ -600,14 +601,17 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int is_reload)
 		}
 	}
 	snprintf(buff, 11, "%d", max_sectors_kb);
+	len = strlen(buff);
 
 	vector_foreach_slot (mpp->pg, pgp, i) {
 		vector_foreach_slot(pgp->paths, pp, j) {
 			ret = sysfs_attr_set_value(pp->udev,
 						   "queue/max_sectors_kb",
-						   buff, strlen(buff));
-			if (ret < 0) {
-				condlog(1, "failed setting max_sectors_kb on %s : %s", pp->dev, strerror(-ret));
+						   buff, len);
+			if (ret != len) {
+				log_sysfs_attr_set_value(1, ret,
+					"failed setting max_sectors_kb on %s",
+					pp->dev);
 				err = 1;
 			}
 		}
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 54b1caf..1137629 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -603,8 +603,10 @@ sysfs_set_eh_deadline(struct path *pp)
 	 * not all scsi drivers support setting eh_deadline, so failing
 	 * is totally reasonable
 	 */
-	if (ret <= 0)
-		condlog(3, "%s: failed to set eh_deadline to %s, error %d", udev_device_get_sysname(hostdev), value, -ret);
+	if (ret != len + 1)
+		log_sysfs_attr_set_value(3, ret,
+			"%s: failed to set eh_deadline to %s",
+			udev_device_get_sysname(hostdev), value);
 
 	udev_device_unref(hostdev);
 	return (ret <= 0);
@@ -667,19 +669,22 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp)
 	    pp->fast_io_fail != MP_FAST_IO_FAIL_OFF) {
 		/* Check if we need to temporarily increase dev_loss_tmo */
 		if ((unsigned int)pp->fast_io_fail >= tmo) {
+			ssize_t len;
+
 			/* Increase dev_loss_tmo temporarily */
 			snprintf(value, sizeof(value), "%u",
 				 (unsigned int)pp->fast_io_fail + 1);
+			len = strlen(value);
 			ret = sysfs_attr_set_value(rport_dev, "dev_loss_tmo",
-						   value, strlen(value));
-			if (ret <= 0) {
+						   value, len);
+			if (ret != len) {
 				if (ret == -EBUSY)
 					condlog(3, "%s: rport blocked",
 						rport_id);
 				else
-					condlog(0, "%s: failed to set "
-						"dev_loss_tmo to %s, error %d",
-						rport_id, value, -ret);
+					log_sysfs_attr_set_value(0, ret,
+						"%s: failed to set dev_loss_tmo to %s",
+						rport_id, value);
 				goto out;
 			}
 		}
@@ -691,32 +696,39 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp)
 		pp->dev_loss = DEFAULT_DEV_LOSS_TMO;
 	}
 	if (pp->fast_io_fail != MP_FAST_IO_FAIL_UNSET) {
+		ssize_t len;
+
 		if (pp->fast_io_fail == MP_FAST_IO_FAIL_OFF)
 			sprintf(value, "off");
 		else if (pp->fast_io_fail == MP_FAST_IO_FAIL_ZERO)
 			sprintf(value, "0");
 		else
 			snprintf(value, 16, "%u", pp->fast_io_fail);
+		len = strlen(value);
 		ret = sysfs_attr_set_value(rport_dev, "fast_io_fail_tmo",
-					   value, strlen(value));
-		if (ret <= 0) {
+					   value, len);
+		if (ret != len) {
 			if (ret == -EBUSY)
 				condlog(3, "%s: rport blocked", rport_id);
 			else
-				condlog(0, "%s: failed to set fast_io_fail_tmo to %s, error %d",
-					rport_id, value, -ret);
+				log_sysfs_attr_set_value(0, ret,
+					"%s: failed to set fast_io_fail_tmo to %s",
+					rport_id, value);
 		}
 	}
 	if (pp->dev_loss != DEV_LOSS_TMO_UNSET) {
+		ssize_t len;
+
 		snprintf(value, 16, "%u", pp->dev_loss);
-		ret = sysfs_attr_set_value(rport_dev, "dev_loss_tmo",
-					   value, strlen(value));
+		len = strlen(value);
+		ret = sysfs_attr_set_value(rport_dev, "dev_loss_tmo", value, len);
 		if (ret <= 0) {
 			if (ret == -EBUSY)
 				condlog(3, "%s: rport blocked", rport_id);
 			else
-				condlog(0, "%s: failed to set dev_loss_tmo to %s, error %d",
-					rport_id, value, -ret);
+				log_sysfs_attr_set_value(0, ret,
+					"%s: failed to set dev_loss_tmo to %s",
+					rport_id, value);
 		}
 	}
 out:
@@ -754,12 +766,16 @@ sysfs_set_session_tmo(struct path *pp)
 			condlog(3, "%s: can't set fast_io_fail_tmo to '0'"
 				"on iSCSI", pp->dev);
 		} else {
+			ssize_t len, ret;
+
 			snprintf(value, 11, "%u", pp->fast_io_fail);
-			if (sysfs_attr_set_value(session_dev, "recovery_tmo",
-						 value, strlen(value)) <= 0) {
-				condlog(3, "%s: Failed to set recovery_tmo, "
-					" error %d", pp->dev, errno);
-			}
+			len = strlen(value);
+			ret = sysfs_attr_set_value(session_dev, "recovery_tmo",
+						   value, len);
+			if (ret != len)
+				log_sysfs_attr_set_value(3, ret,
+					"%s: Failed to set recovery_tmo to %s",
+							 pp->dev, value);
 		}
 	}
 	udev_device_unref(session_dev);
@@ -802,12 +818,16 @@ sysfs_set_nexus_loss_tmo(struct path *pp)
 		pp->sg_id.channel, pp->sg_id.scsi_id, end_dev_id);
 
 	if (pp->dev_loss != DEV_LOSS_TMO_UNSET) {
+		ssize_t len, ret;
+
 		snprintf(value, 11, "%u", pp->dev_loss);
-		if (sysfs_attr_set_value(sas_dev, "I_T_nexus_loss_timeout",
-					 value, strlen(value)) <= 0)
-			condlog(3, "%s: failed to update "
-				"I_T Nexus loss timeout, error %d",
-				pp->dev, errno);
+		len = strlen(value);
+		ret = sysfs_attr_set_value(sas_dev, "I_T_nexus_loss_timeout",
+					   value, len);
+		if (ret != len)
+			log_sysfs_attr_set_value(3, ret,
+				"%s: failed to update I_T Nexus loss timeout",
+				pp->dev);
 	}
 	udev_device_unref(sas_dev);
 	return;
diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index 125f1c2..9c84af7 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -134,7 +134,7 @@ ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
 	/* write attribute value */
 	fd = open(devpath, O_WRONLY);
 	if (fd < 0) {
-		condlog(2, "%s: attribute '%s' can not be opened: %s",
+		condlog(3, "%s: attribute '%s' can not be opened: %s",
 			__func__, devpath, strerror(errno));
 		return -errno;
 	}
@@ -144,11 +144,9 @@ ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
 		size = -errno;
 		condlog(3, "%s: write to %s failed: %s", __func__, 
 			devpath, strerror(errno));
-	} else if (size < (ssize_t)value_len) {
+	} else if (size < (ssize_t)value_len)
 		condlog(3, "%s: underflow writing %zu bytes to %s. Wrote %zd bytes",
 			__func__, value_len, devpath, size);
-		size = 0;
-	}
 
 	close(fd);
 	return size;
diff --git a/libmultipath/sysfs.h b/libmultipath/sysfs.h
index cdc84e4..799f68e 100644
--- a/libmultipath/sysfs.h
+++ b/libmultipath/sysfs.h
@@ -5,6 +5,7 @@
 #ifndef _LIBMULTIPATH_SYSFS_H
 #define _LIBMULTIPATH_SYSFS_H
 #include <stdbool.h>
+#include "strbuf.h"
 
 ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
 			     const char * value, size_t value_len);
@@ -25,6 +26,15 @@ ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char *attr_name,
 		sysfs_attr_value_ok(__rc, __l); \
 	})
 
+#define log_sysfs_attr_set_value(prio, rc, fmt, __args...)		\
+do {									\
+	STRBUF_ON_STACK(__buf);						\
+	if (print_strbuf(&__buf, fmt, ##__args) >= 0 &&			\
+	    print_strbuf(&__buf, ": %s", rc < 0 ? strerror(-rc) :	\
+					"write underflow") >= 0)	\
+		condlog(prio, "%s", get_strbuf_str(&__buf));		\
+} while(0)
+
 int sysfs_get_size (struct path *pp, unsigned long long * size);
 int sysfs_check_holders(char * check_devt, char * new_devt);
 bool sysfs_is_multipathed(struct path *pp, bool set_wwid);
-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 08/14] libmultipath: sysfs_attr_set_value(): don't return 0 on partial write

[Benjamin Marzinski]

On Wed, Jul 06, 2022 at 04:38:16PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> sysfs_attr_set_value() returned 0 if not all requested bytes were written.
> Change this to return the number of bytes written. Error checking is now
> somewhat more involved; provide a helper macro fo it.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c | 10 ++++--
>  libmultipath/discovery.c | 70 ++++++++++++++++++++++++++--------------
>  libmultipath/sysfs.c     |  6 ++--
>  libmultipath/sysfs.h     | 10 ++++++
>  4 files changed, 64 insertions(+), 32 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 467bbaa..0607dba 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -568,6 +568,7 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int is_reload)
>  	struct pathgroup * pgp;
>  	struct path *pp;
>  	char buff[11];
> +	ssize_t len;
>  	int i, j, ret, err = 0;
>  	struct udev_device *udd;
>  	int max_sectors_kb;
> @@ -600,14 +601,17 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int is_reload)
>  		}
>  	}
>  	snprintf(buff, 11, "%d", max_sectors_kb);
> +	len = strlen(buff);
>  
>  	vector_foreach_slot (mpp->pg, pgp, i) {
>  		vector_foreach_slot(pgp->paths, pp, j) {
>  			ret = sysfs_attr_set_value(pp->udev,
>  						   "queue/max_sectors_kb",
> -						   buff, strlen(buff));
> -			if (ret < 0) {
> -				condlog(1, "failed setting max_sectors_kb on %s : %s", pp->dev, strerror(-ret));
> +						   buff, len);
> +			if (ret != len) {
> +				log_sysfs_attr_set_value(1, ret,
> +					"failed setting max_sectors_kb on %s",
> +					pp->dev);
>  				err = 1;
>  			}
>  		}
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 54b1caf..1137629 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -603,8 +603,10 @@ sysfs_set_eh_deadline(struct path *pp)
>  	 * not all scsi drivers support setting eh_deadline, so failing
>  	 * is totally reasonable
>  	 */
> -	if (ret <= 0)
> -		condlog(3, "%s: failed to set eh_deadline to %s, error %d", udev_device_get_sysname(hostdev), value, -ret);
> +	if (ret != len + 1)

I know that this was originally my error, but I don't see any reason why
we should check (or write) the NULL byte here.  I can fix this is in a
separate patch, or you can fix it along with my other issue for this
patch.

> +		log_sysfs_attr_set_value(3, ret,
> +			"%s: failed to set eh_deadline to %s",
> +			udev_device_get_sysname(hostdev), value);
>  
>  	udev_device_unref(hostdev);
>  	return (ret <= 0);
> @@ -667,19 +669,22 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp)
>  	    pp->fast_io_fail != MP_FAST_IO_FAIL_OFF) {
>  		/* Check if we need to temporarily increase dev_loss_tmo */
>  		if ((unsigned int)pp->fast_io_fail >= tmo) {
> +			ssize_t len;
> +
>  			/* Increase dev_loss_tmo temporarily */
>  			snprintf(value, sizeof(value), "%u",
>  				 (unsigned int)pp->fast_io_fail + 1);
> +			len = strlen(value);
>  			ret = sysfs_attr_set_value(rport_dev, "dev_loss_tmo",
> -						   value, strlen(value));
> -			if (ret <= 0) {
> +						   value, len);
> +			if (ret != len) {
>  				if (ret == -EBUSY)
>  					condlog(3, "%s: rport blocked",
>  						rport_id);
>  				else
> -					condlog(0, "%s: failed to set "
> -						"dev_loss_tmo to %s, error %d",
> -						rport_id, value, -ret);
> +					log_sysfs_attr_set_value(0, ret,
> +						"%s: failed to set dev_loss_tmo to %s",
> +						rport_id, value);
>  				goto out;
>  			}
>  		}
> @@ -691,32 +696,39 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp)
>  		pp->dev_loss = DEFAULT_DEV_LOSS_TMO;
>  	}
>  	if (pp->fast_io_fail != MP_FAST_IO_FAIL_UNSET) {
> +		ssize_t len;
> +
>  		if (pp->fast_io_fail == MP_FAST_IO_FAIL_OFF)
>  			sprintf(value, "off");
>  		else if (pp->fast_io_fail == MP_FAST_IO_FAIL_ZERO)
>  			sprintf(value, "0");
>  		else
>  			snprintf(value, 16, "%u", pp->fast_io_fail);
> +		len = strlen(value);
>  		ret = sysfs_attr_set_value(rport_dev, "fast_io_fail_tmo",
> -					   value, strlen(value));
> -		if (ret <= 0) {
> +					   value, len);
> +		if (ret != len) {
>  			if (ret == -EBUSY)
>  				condlog(3, "%s: rport blocked", rport_id);
>  			else
> -				condlog(0, "%s: failed to set fast_io_fail_tmo to %s, error %d",
> -					rport_id, value, -ret);
> +				log_sysfs_attr_set_value(0, ret,
> +					"%s: failed to set fast_io_fail_tmo to %s",
> +					rport_id, value);
>  		}
>  	}
>  	if (pp->dev_loss != DEV_LOSS_TMO_UNSET) {
> +		ssize_t len;
> +
>  		snprintf(value, 16, "%u", pp->dev_loss);
> -		ret = sysfs_attr_set_value(rport_dev, "dev_loss_tmo",
> -					   value, strlen(value));
> +		len = strlen(value);
> +		ret = sysfs_attr_set_value(rport_dev, "dev_loss_tmo", value, len);
>  		if (ret <= 0) {

This check should be (ret != len)

-Ben

>  			if (ret == -EBUSY)
>  				condlog(3, "%s: rport blocked", rport_id);
>  			else
> -				condlog(0, "%s: failed to set dev_loss_tmo to %s, error %d",
> -					rport_id, value, -ret);
> +				log_sysfs_attr_set_value(0, ret,
> +					"%s: failed to set dev_loss_tmo to %s",
> +					rport_id, value);
>  		}
>  	}
>  out:
> @@ -754,12 +766,16 @@ sysfs_set_session_tmo(struct path *pp)
>  			condlog(3, "%s: can't set fast_io_fail_tmo to '0'"
>  				"on iSCSI", pp->dev);
>  		} else {
> +			ssize_t len, ret;
> +
>  			snprintf(value, 11, "%u", pp->fast_io_fail);
> -			if (sysfs_attr_set_value(session_dev, "recovery_tmo",
> -						 value, strlen(value)) <= 0) {
> -				condlog(3, "%s: Failed to set recovery_tmo, "
> -					" error %d", pp->dev, errno);
> -			}
> +			len = strlen(value);
> +			ret = sysfs_attr_set_value(session_dev, "recovery_tmo",
> +						   value, len);
> +			if (ret != len)
> +				log_sysfs_attr_set_value(3, ret,
> +					"%s: Failed to set recovery_tmo to %s",
> +							 pp->dev, value);
>  		}
>  	}
>  	udev_device_unref(session_dev);
> @@ -802,12 +818,16 @@ sysfs_set_nexus_loss_tmo(struct path *pp)
>  		pp->sg_id.channel, pp->sg_id.scsi_id, end_dev_id);
>  
>  	if (pp->dev_loss != DEV_LOSS_TMO_UNSET) {
> +		ssize_t len, ret;
> +
>  		snprintf(value, 11, "%u", pp->dev_loss);
> -		if (sysfs_attr_set_value(sas_dev, "I_T_nexus_loss_timeout",
> -					 value, strlen(value)) <= 0)
> -			condlog(3, "%s: failed to update "
> -				"I_T Nexus loss timeout, error %d",
> -				pp->dev, errno);
> +		len = strlen(value);
> +		ret = sysfs_attr_set_value(sas_dev, "I_T_nexus_loss_timeout",
> +					   value, len);
> +		if (ret != len)
> +			log_sysfs_attr_set_value(3, ret,
> +				"%s: failed to update I_T Nexus loss timeout",
> +				pp->dev);
>  	}
>  	udev_device_unref(sas_dev);
>  	return;
> diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
> index 125f1c2..9c84af7 100644
> --- a/libmultipath/sysfs.c
> +++ b/libmultipath/sysfs.c
> @@ -134,7 +134,7 @@ ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
>  	/* write attribute value */
>  	fd = open(devpath, O_WRONLY);
>  	if (fd < 0) {
> -		condlog(2, "%s: attribute '%s' can not be opened: %s",
> +		condlog(3, "%s: attribute '%s' can not be opened: %s",
>  			__func__, devpath, strerror(errno));
>  		return -errno;
>  	}
> @@ -144,11 +144,9 @@ ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
>  		size = -errno;
>  		condlog(3, "%s: write to %s failed: %s", __func__, 
>  			devpath, strerror(errno));
> -	} else if (size < (ssize_t)value_len) {
> +	} else if (size < (ssize_t)value_len)
>  		condlog(3, "%s: underflow writing %zu bytes to %s. Wrote %zd bytes",
>  			__func__, value_len, devpath, size);
> -		size = 0;
> -	}
>  
>  	close(fd);
>  	return size;
> diff --git a/libmultipath/sysfs.h b/libmultipath/sysfs.h
> index cdc84e4..799f68e 100644
> --- a/libmultipath/sysfs.h
> +++ b/libmultipath/sysfs.h
> @@ -5,6 +5,7 @@
>  #ifndef _LIBMULTIPATH_SYSFS_H
>  #define _LIBMULTIPATH_SYSFS_H
>  #include <stdbool.h>
> +#include "strbuf.h"
>  
>  ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
>  			     const char * value, size_t value_len);
> @@ -25,6 +26,15 @@ ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char *attr_name,
>  		sysfs_attr_value_ok(__rc, __l); \
>  	})
>  
> +#define log_sysfs_attr_set_value(prio, rc, fmt, __args...)		\
> +do {									\
> +	STRBUF_ON_STACK(__buf);						\
> +	if (print_strbuf(&__buf, fmt, ##__args) >= 0 &&			\
> +	    print_strbuf(&__buf, ": %s", rc < 0 ? strerror(-rc) :	\
> +					"write underflow") >= 0)	\
> +		condlog(prio, "%s", get_strbuf_str(&__buf));		\
> +} while(0)
> +
>  int sysfs_get_size (struct path *pp, unsigned long long * size);
>  int sysfs_check_holders(char * check_devt, char * new_devt);
>  bool sysfs_is_multipathed(struct path *pp, bool set_wwid);
> -- 
> 2.36.1
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 09/14] libmultipath: sysfs: cleanup file descriptors on pthread_cancel()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/sysfs.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index 9c84af7..6494638 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -49,7 +49,7 @@ static ssize_t __sysfs_attr_get_value(struct udev_device *dev, const char *attr_
 {
 	const char *syspath;
 	char devpath[PATH_MAX];
-	int fd;
+	long fd;
 	ssize_t size = -1;
 
 	if (!dev || !attr_name || !value || !value_len) {
@@ -74,6 +74,8 @@ static ssize_t __sysfs_attr_get_value(struct udev_device *dev, const char *attr_
 			__func__, devpath, strerror(errno));
 		return -errno;
 	}
+	pthread_cleanup_push(close_fd, (void *)fd);
+
 	size = read(fd, value, value_len);
 	if (size < 0) {
 		size = -errno;
@@ -90,7 +92,7 @@ static ssize_t __sysfs_attr_get_value(struct udev_device *dev, const char *attr_
 		size = strchop(value);
 	}
 
-	close(fd);
+	pthread_cleanup_pop(1);
 	return size;
 }
 
@@ -112,7 +114,7 @@ ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
 {
 	const char *syspath;
 	char devpath[PATH_MAX];
-	int fd;
+	long fd;
 	ssize_t size = -1;
 
 	if (!dev || !attr_name || !value || !value_len) {
@@ -138,6 +140,7 @@ ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
 			__func__, devpath, strerror(errno));
 		return -errno;
 	}
+	pthread_cleanup_push(close_fd, (void *)fd);
 
 	size = write(fd, value, value_len);
 	if (size < 0) {
@@ -148,7 +151,7 @@ ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
 		condlog(3, "%s: underflow writing %zu bytes to %s. Wrote %zd bytes",
 			__func__, value_len, devpath, size);
 
-	close(fd);
+	pthread_cleanup_pop(1);
 	return size;
 }
 
-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 10/14] libmultipath, multipathd: log failure setting sysfs attributes

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Failure to set a sysfs attribute is worth noting, normally.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c   | 18 +++++++++++++++---
 multipathd/fpin_handlers.c | 11 +++++++++--
 multipathd/main.c          | 38 +++++++++++++++++++++++++++++++-------
 3 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 0607dba..4427f91 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -489,9 +489,15 @@ void trigger_partitions_udev_change(struct udev_device *dev,
 
 		devtype = udev_device_get_devtype(part);
 		if (devtype && !strcmp("partition", devtype)) {
+			ssize_t ret;
+
 			condlog(4, "%s: triggering %s event for %s", __func__,
 				action, syspath);
-			sysfs_attr_set_value(part, "uevent", action, len);
+			ret = sysfs_attr_set_value(part, "uevent", action, len);
+			if (ret != len)
+				log_sysfs_attr_set_value(2, ret,
+					"%s: failed to trigger %s uevent",
+					syspath, action);
 		}
 		udev_device_unref(part);
 	}
@@ -510,6 +516,7 @@ trigger_path_udev_change(struct path *pp, bool is_mpath)
 	 */
 	const char *action = is_mpath ? "change" : "add";
 	const char *env;
+	ssize_t len, ret;
 
 	if (!pp->udev)
 		return;
@@ -536,8 +543,13 @@ trigger_path_udev_change(struct path *pp, bool is_mpath)
 
 	condlog(3, "triggering %s uevent for %s (is %smultipath member)",
 		action, pp->dev, is_mpath ? "" : "no ");
-	sysfs_attr_set_value(pp->udev, "uevent",
-			     action, strlen(action));
+
+	len = strlen(action);
+	ret = sysfs_attr_set_value(pp->udev, "uevent", action, len);
+	if (ret != len)
+		log_sysfs_attr_set_value(2, ret,
+					 "%s: failed to trigger %s uevent",
+					 pp->dev, action);
 	trigger_partitions_udev_change(pp->udev, action,
 				       strlen(action));
 }
diff --git a/multipathd/fpin_handlers.c b/multipathd/fpin_handlers.c
index 384ae31..0019572 100644
--- a/multipathd/fpin_handlers.c
+++ b/multipathd/fpin_handlers.c
@@ -172,8 +172,15 @@ fpin_els_add_li_frame(struct fc_nl_event *fc_event)
 /*Sets the rport port_state to marginal*/
 static void fpin_set_rport_marginal(struct udev_device *rport_dev)
 {
-	sysfs_attr_set_value(rport_dev, "port_state",
-				"Marginal", strlen("Marginal"));
+	static const char marginal[] = "Marginal";
+	ssize_t ret;
+
+	ret = sysfs_attr_set_value(rport_dev, "port_state",
+				   marginal, sizeof(marginal) - 1);
+	if (ret != sizeof(marginal) - 1)
+		log_sysfs_attr_set_value(2, ret,
+					 "%s: failed to set port_state to marginal",
+					 udev_device_get_syspath(rport_dev));
 }
 
 /*Add the marginal devices info into the list*/
diff --git a/multipathd/main.c b/multipathd/main.c
index 68eca92..a160c82 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -911,14 +911,22 @@ rescan_path(struct udev_device *ud)
 {
 	ud = udev_device_get_parent_with_subsystem_devtype(ud, "scsi",
 							   "scsi_device");
-	if (ud)
-		sysfs_attr_set_value(ud, "rescan", "1", strlen("1"));
+	if (ud) {
+		ssize_t ret =
+			sysfs_attr_set_value(ud, "rescan", "1", strlen("1"));
+		if (ret != strlen("1"))
+			log_sysfs_attr_set_value(1, ret,
+						 "%s: failed to trigger rescan",
+						 udev_device_get_syspath(ud));
+	}
 }
 
 void
 handle_path_wwid_change(struct path *pp, struct vectors *vecs)
 {
 	struct udev_device *udd;
+	static const char add[] = "add";
+	ssize_t ret;
 
 	if (!pp || !pp->udev)
 		return;
@@ -929,8 +937,12 @@ handle_path_wwid_change(struct path *pp, struct vectors *vecs)
 		dm_fail_path(pp->mpp->alias, pp->dev_t);
 	}
 	rescan_path(udd);
-	sysfs_attr_set_value(udd, "uevent", "add", strlen("add"));
+	ret = sysfs_attr_set_value(udd, "uevent", add, sizeof(add) - 1);
 	udev_device_unref(udd);
+	if (ret != sizeof(add) - 1)
+		log_sysfs_attr_set_value(1, ret,
+					 "%s: failed to trigger add event",
+					 pp->dev);
 }
 
 bool
@@ -2003,9 +2015,14 @@ partial_retrigger_tick(vector pathvec)
 		    --pp->partial_retrigger_delay == 0) {
 			const char *msg = udev_device_get_is_initialized(pp->udev) ?
 					  "change" : "add";
+			ssize_t len = strlen(msg);
+			ssize_t ret = sysfs_attr_set_value(pp->udev, "uevent", msg,
+							   len);
 
-			sysfs_attr_set_value(pp->udev, "uevent", msg,
-					     strlen(msg));
+			if (len != ret)
+				log_sysfs_attr_set_value(2, ret,
+					"%s: failed to trigger %s event",
+					pp->dev, msg);
 		}
 	}
 }
@@ -2245,12 +2262,19 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 
 	if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {
 		if (pp->retriggers < retrigger_tries) {
+			static const char change[] = "change";
+			ssize_t ret;
+
 			condlog(2, "%s: triggering change event to reinitialize",
 				pp->dev);
 			pp->initialized = INIT_REQUESTED_UDEV;
 			pp->retriggers++;
-			sysfs_attr_set_value(pp->udev, "uevent", "change",
-					     strlen("change"));
+			ret = sysfs_attr_set_value(pp->udev, "uevent", change,
+						   sizeof(change) - 1);
+			if (ret != sizeof(change) - 1)
+				log_sysfs_attr_set_value(1, ret,
+							 "%s: failed to trigger change event",
+							 pp->dev);
 			return 0;
 		} else {
 			condlog(1, "%s: not initialized after %d udev retriggers",
-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 11/14] multipath tests: expect_condlog: skip depending on verbosity

[mwilck]

From: Martin Wilck <mwilck@suse.com>

otherwise we'll get failures if verbosity level is low.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/test-log.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/test-log.c b/tests/test-log.c
index 14f25b9..0c17cd9 100644
--- a/tests/test-log.c
+++ b/tests/test-log.c
@@ -6,6 +6,8 @@
 #include <cmocka.h>
 #include "log.h"
 #include "test-log.h"
+#include "debug.h"
+
 
 __attribute__((format(printf, 2, 0)))
 void __wrap_dlog (int prio, const char * fmt, ...)
@@ -24,6 +26,8 @@ void __wrap_dlog (int prio, const char * fmt, ...)
 
 void expect_condlog(int prio, char *string)
 {
+	if (prio > MAX_VERBOSITY || prio > libmp_verbosity)
+		return;
 	expect_value(__wrap_dlog, prio, prio);
 	will_return(__wrap_dlog, string);
 }
-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 12/14] multipath tests: __wrap_dlog: print log message

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This makes it easier to analyze errors from __wrap_dlog().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/test-log.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/test-log.c b/tests/test-log.c
index 0c17cd9..c174587 100644
--- a/tests/test-log.c
+++ b/tests/test-log.c
@@ -20,6 +20,7 @@ void __wrap_dlog (int prio, const char * fmt, ...)
 	va_start(ap, fmt);
 	vsnprintf(buff, MAX_MSG_SIZE, fmt, ap);
 	va_end(ap);
+	fprintf(stderr, "%s(%d): %s", __func__, prio, buff);
 	expected = mock_ptr_type(char *);
 	assert_memory_equal(buff, expected, strlen(expected));
 }
-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 13/14] multipath tests: add sysfs test

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/Makefile |   5 +-
 tests/sysfs.c  | 494 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 498 insertions(+), 1 deletion(-)
 create mode 100644 tests/sysfs.c

diff --git a/tests/Makefile b/tests/Makefile
index d20ef23..95a9990 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -16,7 +16,7 @@ CFLAGS += $(BIN_CFLAGS) -Wno-unused-parameter $(W_MISSING_INITIALIZERS)
 LIBDEPS += -L. -L$(mpathcmddir) -lmultipath -lmpathcmd -lcmocka
 
 TESTS := uevent parser util dmevents hwtable blacklist unaligned vpd pgpolicy \
-	 alias directio valid devt mpathvalid strbuf
+	 alias directio valid devt mpathvalid strbuf sysfs
 HELPERS := test-lib.o test-log.o
 
 .SILENT: $(TESTS:%=%.o)
@@ -70,6 +70,9 @@ ifneq ($(DIO_TEST_DEV),)
 directio-test_LIBDEPS := -laio
 endif
 strbuf-test_OBJDEPS := ../libmultipath/strbuf.o
+sysfs-test_TESTDEPS := test-log.o
+sysfs-test_OBJDEPS := ../libmultipath/sysfs.o ../libmultipath/util.o
+sysfs-test_LIBDEPS := -ludev -lpthread -ldl
 
 %.o: %.c
 	$(CC) $(CPPFLAGS) $(CFLAGS) $($*-test_FLAGS) -c -o $@ $<
diff --git a/tests/sysfs.c b/tests/sysfs.c
new file mode 100644
index 0000000..0ec135b
--- /dev/null
+++ b/tests/sysfs.c
@@ -0,0 +1,494 @@
+/*
+ * Copyright (c) 2021 SUSE LLC
+ * SPDX-License-Identifier: GPL-2.0-only
+ */
+
+#define _GNU_SOURCE
+#include <stdbool.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <stdlib.h>
+#include <cmocka.h>
+#include <libudev.h>
+#include <fcntl.h>
+#include <errno.h>
+#include "debug.h"
+#include "globals.c"
+#include "test-log.h"
+#include "sysfs.h"
+#include "util.h"
+
+#define TEST_FD 123
+
+char *__wrap_udev_device_get_syspath(struct udev_device *ud)
+{
+	char *val  = mock_ptr_type(char *);
+
+	return val;
+}
+
+int __wrap_open(const char *pathname, int flags)
+{
+	int ret;
+
+	check_expected(pathname);
+	check_expected(flags);
+	ret = mock_type(int);
+	return ret;
+}
+
+ssize_t __wrap_read(int fd, void *buf, size_t count)
+{
+	ssize_t ret;
+	char *val;
+
+	check_expected(fd);
+	check_expected(count);
+	ret = mock_type(int);
+	val = mock_ptr_type(char *);
+	if (ret >= (ssize_t)count)
+		ret = count;
+	if (ret >= 0 && val) {
+		fprintf(stderr, "%s: '%s' -> %zd\n", __func__, val, ret);
+		memcpy(buf, val, ret);
+	}
+	return ret;
+}
+
+ssize_t __wrap_write(int fd, void *buf, size_t count)
+{
+	ssize_t ret;
+
+	check_expected(fd);
+	check_expected(count);
+	ret = mock_type(int);
+	if (ret >= (ssize_t)count)
+		ret = count;
+	return ret;
+}
+
+int __real_close(int fd);
+int __wrap_close(int fd) {
+	if (fd != TEST_FD)
+		return __real_close(fd);
+	return mock_type(int);
+}
+
+static int setup(void **state)
+{
+	udev = udev_new();
+	return 0;
+}
+
+static int teardown(void **state)
+{
+	udev_unref(udev);
+	return 0;
+}
+
+static void expect_sagv_invalid(void)
+{
+	expect_condlog(1, "__sysfs_attr_get_value: invalid parameters");
+}
+
+static void test_sagv_invalid(void **state)
+{
+	expect_sagv_invalid();
+	assert_int_equal(sysfs_attr_get_value(NULL, NULL, NULL, 0), -EINVAL);
+	expect_sagv_invalid();
+	assert_int_equal(sysfs_bin_attr_get_value(NULL, NULL, NULL, 0), -EINVAL);
+
+	expect_sagv_invalid();
+	assert_int_equal(sysfs_attr_get_value(NULL, (void *)state, (void *)state, 1),
+			 -EINVAL);
+	expect_sagv_invalid();
+	assert_int_equal(sysfs_bin_attr_get_value(NULL, (void *)state, (void *)state, 1),
+			 -EINVAL);
+
+	expect_sagv_invalid();
+	assert_int_equal(sysfs_attr_get_value((void *)state, NULL, (void *)state, 1),
+			 -EINVAL);
+	expect_sagv_invalid();
+	assert_int_equal(sysfs_bin_attr_get_value((void *)state, NULL, (void *)state, 1),
+			 -EINVAL);
+
+	expect_sagv_invalid();
+	assert_int_equal(sysfs_attr_get_value((void *)state, (void *)state, NULL, 1),
+			 -EINVAL);
+	expect_sagv_invalid();
+	assert_int_equal(sysfs_bin_attr_get_value((void *)state, (void *)state, NULL, 1),
+			 -EINVAL);
+
+	expect_sagv_invalid();
+	assert_int_equal(sysfs_attr_get_value((void *)state, (void *)state,
+					      (void *)state, 0), -EINVAL);
+	expect_sagv_invalid();
+	assert_int_equal(sysfs_bin_attr_get_value((void *)state, (void *)state,
+						  (void *)state, 0), -EINVAL);
+}
+
+static void test_sagv_bad_udev(void **state)
+{
+	will_return(__wrap_udev_device_get_syspath, NULL);
+	expect_condlog(3, "__sysfs_attr_get_value: invalid udevice");
+	assert_int_equal(sysfs_attr_get_value((void *)state, (void *)state,
+					      (void *)state, 1), -EINVAL);
+
+	will_return(__wrap_udev_device_get_syspath, NULL);
+	expect_condlog(3, "__sysfs_attr_get_value: invalid udevice");
+	assert_int_equal(sysfs_bin_attr_get_value((void *)state, (void *)state,
+						  (void *)state, 1), -EINVAL);
+}
+
+static void test_sagv_bad_snprintf(void **state)
+{
+	char longstr[PATH_MAX + 1];
+	char buf[1];
+
+	memset(longstr, 'a', sizeof(longstr) - 1);
+	longstr[sizeof(longstr) - 1] = '\0';
+
+	will_return(__wrap_udev_device_get_syspath, "/foo");
+	expect_condlog(3, "__sysfs_attr_get_value: devpath overflow");
+	assert_int_equal(sysfs_attr_get_value((void *)state, longstr,
+					      buf, sizeof(buf)), -EOVERFLOW);
+	will_return(__wrap_udev_device_get_syspath, "/foo");
+	expect_condlog(3, "__sysfs_attr_get_value: devpath overflow");
+	assert_int_equal(sysfs_bin_attr_get_value((void *)state, longstr,
+						  (unsigned char *)buf, sizeof(buf)),
+			 -EOVERFLOW);
+}
+
+static void test_sagv_open_fail(void **state)
+{
+	char buf[1];
+
+	will_return(__wrap_udev_device_get_syspath, "/foo");
+	expect_condlog(4, "open '/foo/bar'");
+	expect_string(__wrap_open, pathname, "/foo/bar");
+	expect_value(__wrap_open, flags, O_RDONLY);
+	errno = ENOENT;
+	will_return(__wrap_open, -1);
+	expect_condlog(3, "__sysfs_attr_get_value: attribute '/foo/bar' can not be opened");
+	assert_int_equal(sysfs_attr_get_value((void *)state, "bar",
+					      buf, sizeof(buf)), -ENOENT);
+}
+
+static void test_sagv_read_fail(void **state)
+{
+	char buf[1];
+
+	will_return(__wrap_udev_device_get_syspath, "/foo");
+	expect_condlog(4, "open '/foo/bar'");
+	expect_string(__wrap_open, pathname, "/foo/bar");
+	expect_value(__wrap_open, flags, O_RDONLY);
+	will_return(__wrap_open, TEST_FD);
+	expect_value(__wrap_read, fd, TEST_FD);
+	expect_value(__wrap_read, count, sizeof(buf));
+	errno = EISDIR;
+	will_return(__wrap_read, -1);
+	will_return(__wrap_read, NULL);
+	expect_condlog(3, "__sysfs_attr_get_value: read from /foo/bar failed:");
+	will_return(__wrap_close, 0);
+	assert_int_equal(sysfs_attr_get_value((void *)state, "bar",
+					      buf, sizeof(buf)), -EISDIR);
+
+	will_return(__wrap_udev_device_get_syspath, "/foo");
+	expect_condlog(4, "open '/foo/baz'");
+	expect_string(__wrap_open, pathname, "/foo/baz");
+	expect_value(__wrap_open, flags, O_RDONLY);
+	will_return(__wrap_open, TEST_FD);
+	expect_value(__wrap_read, fd, TEST_FD);
+	expect_value(__wrap_read, count, sizeof(buf));
+	errno = EPERM;
+	will_return(__wrap_read, -1);
+	will_return(__wrap_read, NULL);
+	expect_condlog(3, "__sysfs_attr_get_value: read from /foo/baz failed:");
+	will_return(__wrap_close, 0);
+	assert_int_equal(sysfs_bin_attr_get_value((void *)state, "baz",
+						  (unsigned char *)buf, sizeof(buf)),
+			 -EPERM);
+
+}
+
+static void _test_sagv_read(void **state, unsigned int bufsz)
+{
+	char buf[16];
+	char input[] = "01234567";
+	unsigned int n, trunc;
+
+	assert_in_range(bufsz, 1, sizeof(buf));
+	memset(buf, '.', sizeof(buf));
+	will_return(__wrap_udev_device_get_syspath, "/foo");
+	expect_condlog(4, "open '/foo/bar'");
+	expect_string(__wrap_open, pathname, "/foo/bar");
+	expect_value(__wrap_open, flags, O_RDONLY);
+	will_return(__wrap_open, TEST_FD);
+	expect_value(__wrap_read, fd, TEST_FD);
+	expect_value(__wrap_read, count, bufsz);
+	will_return(__wrap_read, sizeof(input) - 1);
+	will_return(__wrap_read, input);
+
+	/* If the buffer is too small, input will be truncated by a 0 byte */
+	if (bufsz <= sizeof(input) - 1) {
+		n = bufsz;
+		trunc = 1;
+		expect_condlog(3, "__sysfs_attr_get_value: overflow reading from /foo/bar");
+	} else {
+		n = sizeof(input) - 1;
+		trunc = 0;
+	}
+	will_return(__wrap_close, 0);
+	assert_int_equal(sysfs_attr_get_value((void *)state, "bar",
+					      buf, bufsz), n);
+	assert_memory_equal(buf, input, n - trunc);
+	assert_int_equal(buf[n - trunc], '\0');
+
+	/* Binary input is not truncated */
+	memset(buf, '.', sizeof(buf));
+	will_return(__wrap_udev_device_get_syspath, "/foo");
+	expect_condlog(4, "open '/foo/baz'");
+	expect_string(__wrap_open, pathname, "/foo/baz");
+	expect_value(__wrap_open, flags, O_RDONLY);
+	will_return(__wrap_open, TEST_FD);
+	expect_value(__wrap_read, fd, TEST_FD);
+	expect_value(__wrap_read, count, bufsz);
+	will_return(__wrap_read, sizeof(input) - 1);
+	will_return(__wrap_read, input);
+	will_return(__wrap_close, 0);
+	n = bufsz < sizeof(input) - 1 ? bufsz : sizeof(input) - 1;
+	assert_int_equal(sysfs_bin_attr_get_value((void *)state, "baz",
+						  (unsigned char *)buf,
+						  bufsz),
+			 n);
+	assert_memory_equal(buf, input, n);
+}
+
+static void test_sagv_read_overflow_8(void **state)
+{
+	_test_sagv_read(state, 8);
+}
+
+static void test_sagv_read_overflow_4(void **state)
+{
+	_test_sagv_read(state, 4);
+}
+
+static void test_sagv_read_overflow_1(void **state)
+{
+	_test_sagv_read(state, 1);
+}
+
+static void test_sagv_read_good_9(void **state)
+{
+	_test_sagv_read(state, 9);
+}
+
+static void test_sagv_read_good_15(void **state)
+{
+	_test_sagv_read(state, 15);
+}
+
+static void _test_sagv_read_zeroes(void **state, unsigned int bufsz)
+{
+	char buf[16];
+	char input[] = { '\0','\0','\0','\0','\0','\0','\0','\0' };
+	unsigned int n;
+
+	assert_in_range(bufsz, 1, sizeof(buf));
+	memset(buf, '.', sizeof(buf));
+	will_return(__wrap_udev_device_get_syspath, "/foo");
+	expect_condlog(4, "open '/foo/bar'");
+	expect_string(__wrap_open, pathname, "/foo/bar");
+	expect_value(__wrap_open, flags, O_RDONLY);
+	will_return(__wrap_open, TEST_FD);
+	expect_value(__wrap_read, fd, TEST_FD);
+	expect_value(__wrap_read, count, bufsz);
+	will_return(__wrap_read, sizeof(input) - 1);
+	will_return(__wrap_read, input);
+
+	if (bufsz <= sizeof(input) - 1) {
+		n = bufsz;
+		expect_condlog(3, "__sysfs_attr_get_value: overflow reading from /foo/bar");
+	} else
+		n = 0;
+
+	will_return(__wrap_close, 0);
+	assert_int_equal(sysfs_attr_get_value((void *)state, "bar",
+					      buf, bufsz), n);
+
+	/*
+	 * The return value of sysfs_attr_get_value ignores zero bytes,
+	 * but the read data should have been copied to the buffer
+	 */
+	assert_memory_equal(buf, input, n == 0 ? bufsz : n);
+}
+
+static void test_sagv_read_zeroes_4(void **state)
+{
+	_test_sagv_read_zeroes(state, 4);
+}
+
+static void expect_sasv_invalid(void)
+{
+	expect_condlog(1, "sysfs_attr_set_value: invalid parameters");
+}
+
+static void test_sasv_invalid(void **state)
+{
+	expect_sasv_invalid();
+	assert_int_equal(sysfs_attr_set_value(NULL, NULL, NULL, 0), -EINVAL);
+
+	expect_sasv_invalid();
+	assert_int_equal(sysfs_attr_set_value(NULL, (void *)state, (void *)state, 1),
+			 -EINVAL);
+
+	expect_sasv_invalid();
+	assert_int_equal(sysfs_attr_set_value((void *)state, NULL, (void *)state, 1),
+			 -EINVAL);
+
+	expect_sasv_invalid();
+	assert_int_equal(sysfs_attr_set_value((void *)state, (void *)state, NULL, 1),
+			 -EINVAL);
+
+	expect_sasv_invalid();
+	assert_int_equal(sysfs_attr_set_value((void *)state, (void *)state,
+					      (void *)state, 0), -EINVAL);
+}
+
+static void test_sasv_bad_udev(void **state)
+{
+	will_return(__wrap_udev_device_get_syspath, NULL);
+	expect_condlog(3, "sysfs_attr_set_value: invalid udevice");
+	assert_int_equal(sysfs_attr_set_value((void *)state, (void *)state,
+					      (void *)state, 1), -EINVAL);
+}
+
+static void test_sasv_bad_snprintf(void **state)
+{
+	char longstr[PATH_MAX + 1];
+	char buf[1];
+
+	memset(longstr, 'a', sizeof(longstr) - 1);
+	longstr[sizeof(longstr) - 1] = '\0';
+
+	will_return(__wrap_udev_device_get_syspath, "/foo");
+	expect_condlog(3, "sysfs_attr_set_value: devpath overflow");
+	assert_int_equal(sysfs_attr_set_value((void *)state, longstr,
+					      buf, sizeof(buf)), -EOVERFLOW);
+}
+
+static void test_sasv_open_fail(void **state)
+{
+	char buf[1];
+
+	will_return(__wrap_udev_device_get_syspath, "/foo");
+	expect_condlog(4, "open '/foo/bar'");
+	expect_string(__wrap_open, pathname, "/foo/bar");
+	expect_value(__wrap_open, flags, O_WRONLY);
+	errno = EPERM;
+	will_return(__wrap_open, -1);
+	expect_condlog(3, "sysfs_attr_set_value: attribute '/foo/bar' can not be opened");
+	assert_int_equal(sysfs_attr_set_value((void *)state, "bar",
+					      buf, sizeof(buf)), -EPERM);
+}
+
+static void test_sasv_write_fail(void **state)
+{
+	char buf[1];
+
+	will_return(__wrap_udev_device_get_syspath, "/foo");
+	expect_condlog(4, "open '/foo/bar'");
+	expect_string(__wrap_open, pathname, "/foo/bar");
+	expect_value(__wrap_open, flags, O_WRONLY);
+	will_return(__wrap_open, TEST_FD);
+	expect_value(__wrap_write, fd, TEST_FD);
+	expect_value(__wrap_write, count, sizeof(buf));
+	errno = EISDIR;
+	will_return(__wrap_write, -1);
+	expect_condlog(3, "sysfs_attr_set_value: write to /foo/bar failed:");
+	will_return(__wrap_close, 0);
+	assert_int_equal(sysfs_attr_set_value((void *)state, "bar",
+					      buf, sizeof(buf)), -EISDIR);
+
+}
+
+static void _test_sasv_write(void **state, unsigned int n_written)
+{
+	char buf[8];
+
+	assert_in_range(n_written, 0, sizeof(buf));
+	will_return(__wrap_udev_device_get_syspath, "/foo");
+	expect_condlog(4, "open '/foo/bar'");
+	expect_string(__wrap_open, pathname, "/foo/bar");
+	expect_value(__wrap_open, flags, O_WRONLY);
+	will_return(__wrap_open, TEST_FD);
+	expect_value(__wrap_write, fd, TEST_FD);
+	expect_value(__wrap_write, count, sizeof(buf));
+	will_return(__wrap_write, n_written);
+
+	if (n_written < sizeof(buf))
+		expect_condlog(3, "sysfs_attr_set_value: underflow writing");
+	will_return(__wrap_close, 0);
+	assert_int_equal(sysfs_attr_set_value((void *)state, "bar",
+					      buf, sizeof(buf)),
+			 n_written);
+}
+
+static void test_sasv_write_0(void **state)
+{
+	_test_sasv_write(state, 0);
+}
+
+static void test_sasv_write_4(void **state)
+{
+	_test_sasv_write(state, 4);
+}
+
+static void test_sasv_write_7(void **state)
+{
+	_test_sasv_write(state, 7);
+}
+
+static void test_sasv_write_8(void **state)
+{
+	_test_sasv_write(state, 8);
+}
+
+static int test_sysfs(void)
+{
+	const struct CMUnitTest tests[] = {
+		cmocka_unit_test(test_sagv_invalid),
+		cmocka_unit_test(test_sagv_bad_udev),
+		cmocka_unit_test(test_sagv_bad_snprintf),
+		cmocka_unit_test(test_sagv_open_fail),
+		cmocka_unit_test(test_sagv_read_fail),
+		cmocka_unit_test(test_sagv_read_overflow_1),
+		cmocka_unit_test(test_sagv_read_overflow_4),
+		cmocka_unit_test(test_sagv_read_overflow_8),
+		cmocka_unit_test(test_sagv_read_good_9),
+		cmocka_unit_test(test_sagv_read_good_15),
+		cmocka_unit_test(test_sagv_read_zeroes_4),
+		cmocka_unit_test(test_sasv_invalid),
+		cmocka_unit_test(test_sasv_bad_udev),
+		cmocka_unit_test(test_sasv_bad_snprintf),
+		cmocka_unit_test(test_sasv_open_fail),
+		cmocka_unit_test(test_sasv_write_fail),
+		cmocka_unit_test(test_sasv_write_0),
+		cmocka_unit_test(test_sasv_write_4),
+		cmocka_unit_test(test_sasv_write_7),
+		cmocka_unit_test(test_sasv_write_8),
+	};
+
+	return cmocka_run_group_tests(tests, setup, teardown);
+}
+
+int main(void)
+{
+	int ret = 0;
+
+	init_test_verbosity(4);
+	ret += test_sysfs();
+	return ret;
+}
-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 14/14] libmultipath.version: bump version for sysfs accessors

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Formally, the ABI is still the same, but the semantics of the
return value have changed.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/libmultipath.version | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index b3690ac..c1d9b15 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -207,7 +207,6 @@ global:
 	strchop;
 	strlcpy;
 	sync_map_state;
-	sysfs_attr_set_value;
 	sysfs_get_size;
 	sysfs_is_multipathed;
 	timespeccmp;
@@ -264,8 +263,13 @@ global:
 
 	/* foreign */
 	free_scandir_result;
-	sysfs_attr_get_value;
 
 local:
 	*;
 };
+
+LIBMULTIPATH_16.0.0 {
+global:
+	sysfs_attr_set_value;
+	sysfs_attr_get_value;
+};
-- 
2.36.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 00/14] multipath: fixes for sysfs accessors

[Benjamin Marzinski]

On Wed, Jul 06, 2022 at 04:38:08PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> This set fixes some strangeness in our sysfs accessors which I
> found while looking at
> https://github.com/opensvc/multipath-tools/issues/35#issuecomment-1175901745.
> (The patches don't fix this issue, which seems to be related to
> Debian's initramfs setup).
> 
> Most importantly, sysfs_attr_get_value() and sysfs_attr_set_value()
> would return 0 if the number of bytes read/written was different from
> the expected value, which is non-standard and unexpected. This
> series changes the return value semantics of these functions:
> 
>  - in sysfs_attr_get_value(), if a read buffer is too small to hold
>    the string read plus a terminating 0 byte, the return value
>    equals the buffer size.
>  - in sysfs_bin_attr_get_value(), no 0 bytes are appended. It's not
>    an error if the read buffer is completely filled, and no warning
>    is printed in this case.
>  - sysfs_attr_set_value() always returns the number of bytes written
>    unless an error occured in open() or write().
> 
> Tests for the new semantics are added. Moreover, the sysfs.c code
> is slightly refactored to avoid code duplication.

For all except 8/14:
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> 
> Martin Wilck (14):
>   libmultipath: alua: remove get_sysfs_pg83()
>   libmultipath: remove sysfs_get_binary()
>   libmultipath: sysfs_bin_attr_get_value(): no error if buffer is filled
>   libmultipath: common code path for sysfs_attr_get_value()
>   libmultipath: sanitize error checking in sysfs accessors
>   libmultipath: get rid of PATH_SIZE
>   libmultipath: sysfs_attr_get_value(): don't return 0 if buffer too
>     small
>   libmultipath: sysfs_attr_set_value(): don't return 0 on partial write
>   libmultipath: sysfs: cleanup file descriptors on pthread_cancel()
>   libmultipath, multipathd: log failure setting sysfs attributes
>   multipath tests: expect_condlog: skip depending on verbosity
>   multipath tests: __wrap_dlog: print log message
>   multipath tests: add sysfs test
>   libmultipath.version: bump version for sysfs accessors
> 
>  libmultipath/configure.c              |  30 +-
>  libmultipath/discovery.c              | 120 +++----
>  libmultipath/libmultipath.version     |   8 +-
>  libmultipath/prioritizers/alua_rtpg.c |  57 +--
>  libmultipath/propsel.c                |   6 +-
>  libmultipath/structs.h                |   3 -
>  libmultipath/sysfs.c                  | 191 ++++------
>  libmultipath/sysfs.h                  |  23 ++
>  libmultipath/util.c                   |   8 +-
>  multipathd/cli_handlers.c             |   2 +-
>  multipathd/fpin_handlers.c            |  11 +-
>  multipathd/main.c                     |  40 ++-
>  tests/Makefile                        |   5 +-
>  tests/sysfs.c                         | 494 ++++++++++++++++++++++++++
>  tests/test-lib.c                      |   1 -
>  tests/test-log.c                      |   5 +
>  16 files changed, 751 insertions(+), 253 deletions(-)
>  create mode 100644 tests/sysfs.c
> 
> -- 
> 2.36.1
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel