[dm-devel] [PATCH v10 2/2] init: add support to directly boot to a mapped device
From: Will Drewry Add a dm= kernel parameter. It allows device-mapper targets to be configured at boot time for use early in the boot process (as the root device or otherwise). Signed-off-by: Will Drewry Signed-off-by: Kees Cook [rework to use dm_ioctl calls] Signed-off-by: Enric Balletbo i Serra [rework to use concise format | rework for upstream] Signed-off-by: Helen Koike --- Hello, In this patch, I constrained the targets allowed to be used by dm=, but I am not entirely familiar with all the targets. I blacklisted the ones mentioned previously and some other that I think doesn't make much sense, but please let me know if you think there are others I should blacklist. Changes since v9: - https://www.redhat.com/archives/linux-lvm/2018-September/msg00016.html - new file: drivers/md/dm-boot.c - most of the parsing code was moved from init/do_mounts_dm.c to drivers/md/dm-boot.c - parsing code was in essence replaced by the concise parser from dmsetup _create_concise function: https://sourceware.org/git/?p=lvm2.git;a=blob;f=libdm/dm-tools/dmsetup.c;h=835fdcdc75e8f0f0f7c4ed46cc9788a6616f58b8;hb=7498f8383397a93db95655ca227257836cbcac82#l1265 the main reason is that this code is already being used/tested by dmsetup, so we can have some level of confidence that it works as expected. Besides this, it also looks more efficient. - Not all targets are allowed to be used by dm=, as pointed previously, there are some risks in creating a mapped device without some validation from userspace (see documentation from the patch listing which targets are allowed). - Instead of using a simple singly linked list (for devices and tables), use the struct list_head. This occupies unnecessary space in the code, but it makes the code cleaner and easier to read and less prone to silly errors. - Documentation and comments were reviewed and refactored, e.g.: * "is to possible" was removed * s/specified as a simple string/specified as a string/ - Added docs above __align function, make it clear that the second parameter @a must be a power of two. - Clean ups: removal of unnecessary includes, macros, variables, some redundant checks and warnings. - when calling ioctls, the code was allocating and freeing the same structure a couple of times. So instead of executing kzalloc/kfree 3 times, execute kmalloc once and reuse the structure after a memset, then finally kfree it once. - update commit message Thanks --- .../admin-guide/kernel-parameters.rst | 1 + .../admin-guide/kernel-parameters.txt | 3 + Documentation/device-mapper/dm-boot.txt | 87 drivers/md/Makefile | 2 +- drivers/md/dm-boot.c | 433 ++ include/linux/device-mapper.h | 6 + init/Makefile | 1 + init/do_mounts.c | 1 + init/do_mounts.h | 10 + init/do_mounts_dm.c | 46 ++ 10 files changed, 589 insertions(+), 1 deletion(-) create mode 100644 Documentation/device-mapper/dm-boot.txt create mode 100644 drivers/md/dm-boot.c create mode 100644 init/do_mounts_dm.c diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst index b8d0bc07ed0a..bd628865f66f 100644 --- a/Documentation/admin-guide/kernel-parameters.rst +++ b/Documentation/admin-guide/kernel-parameters.rst @@ -91,6 +91,7 @@ parameter is applicable:: AX25Appropriate AX.25 support is enabled. CLK Common clock infrastructure is enabled. CMA Contiguous Memory Area support is enabled. + DM Device mapper support is enabled. DRM Direct Rendering Management support is enabled. DYNAMIC_DEBUG Build in debug messages and enable them at runtime EDD BIOS Enhanced Disk Drive Services (EDD) is enabled diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 92eb1f42240d..a3ff7192b980 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -880,6 +880,9 @@ dis_ucode_ldr [X86] Disable the microcode loader. + dm= [DM] Allows early creation of a device-mapper device. + See Documentation/device-mapper/dm-boot.txt. + dma_debug=off If the kernel is compiled with DMA_API_DEBUG support, this option disables the debugging code at boot. diff --git a/Documentation/device-mapper/dm-boot.txt b/Documentation/device-mapper/dm-boot.txt new file mode 100644 index ..14f756b34328 --- /dev/null +++ b/Documentation/device-mapper/dm-boot.txt @@ -0,0 +1,87 @@ +Boot time creation of mapped devices + + +It is possible to configure a device-mapper device to act as the root device
[dm-devel] [PATCH v10 0/2] dm: boot a mapped device without an initramfs
As mentioned in the discussion from the previous version of this patch, Android and Chrome OS do not use initramfs mostly due to boot time and size liability. A practical example as mentioned by Kees is that Chrome OS has a limited amount of storage available for the boot image as it is covered by the static root of trust signature. So instead of bringing up userspace to perform the required configuration for mapped devices, this patchset allows them to be configured in the kernel command line parameter for use early in the boot process, allowing booting from a mapped device without an initramfs. The syntax used in the boot param is based on the concise format from the dmsetup tool as described in its man page http://man7.org/linux/man-pages/man8/dmsetup.8.html#CONCISE_FORMAT Which is: dm=[,+][;[,+]+] Where, ::= The device name. ::= ---- | "" ::= The device minor number | "" ::= "ro" | "rw" ::= ::= "verity" | "linear" | ... Example, the following could be added in the boot parameters. dm="lroot,,,rw, 0 4096 linear 98:16 0, 4096 4096 linear 98:32 0" root=/dev/dm-0 Please check patch 2/2 with the documentation on the format. The idea to make it compatible with the dmsetup concise format is to make it easier for users, allowing just copy & paste from the output of the command: sudo dmsetup table --concise /dev/mapper/lroot The implementation consists basically in parsing the command line argument and performing the ioctls that would be performed by userspace otherwise, i.e. DM_DEV_CREATE, followed by DM_TABLE_LOAD then DM_DEV_SUSPEND. Instead of performing the ioctls, we could by-pass it, calling the corresponding functions directly, but the ioctls calls perform some checks and also the implementation stays less invasive. Please let me know if you would prefer a directly call instead of going thought the ioctls. Changes since v9: - https://www.redhat.com/archives/linux-lvm/2018-September/msg00016.html - new file: drivers/md/dm-boot.c - most of the parsing code was moved from init/do_mounts_dm.c to drivers/md/dm-boot.c - parsing code was in essence replaced by the concise parser from dmsetup _create_concise function: https://sourceware.org/git/?p=lvm2.git;a=blob;f=libdm/dm-tools/dmsetup.c;h=835fdcdc75e8f0f0f7c4ed46cc9788a6616f58b8;hb=7498f8383397a93db95655ca227257836cbcac82#l1265 the main reason is that this code is already being used/tested by dmsetup, so we can have some level of confidence that it works as expected. Besides this, it also looks more efficient. - Not all targets are allowed to be used by dm=, as pointed previously, there are some risks in creating a mapped device without some validation from userspace (see documentation from the patch listing which targets are allowed). - Instead of using a simple singly linked list (for devices and tables), use the struct list_head. This occupies unnecessary space in the code, but it makes the code cleaner and easier to read and less prone to silly errors. - Documentation and comments were reviewed and refactored, e.g.: * "is to possible" was removed * s/specified as a simple string/specified as a string/ - Added docs above __align function, make it clear that the second parameter @a must be a power of two. - Clean ups: removal of unnecessary includes, macros, variables, some redundant checks and warnings. - when calling ioctls, the code was allocating and freeing the same structure a couple of times. So instead of executing kzalloc/kfree 3 times, execute kmalloc once and reuse the structure after a memset, then finally kfree it once. - update commit message Changes since v8: - https://www.redhat.com/archives/linux-lvm/2017-May/msg00055.html - Add minor number to make it compatible with dmsetup concise format Changes since v7: - http://lkml.iu.edu/hypermail/linux/kernel/1705.2/02657.html - Fix build error due commit e516db4f67 (dm ioctl: add a new DM_DEV_ARM_POLL ioctl) Changes since v6: - https://www.redhat.com/archives/dm-devel/2017-April/msg00316.html - Add a new function to issue the equivalent of a DM ioctl programatically. - Use the new ioctl interface to create the devices. - Use a comma-delimited and semi-colon delimited dmsetup-like commands. Changes since v5: - https://www.redhat.com/archives/dm-devel/2016-February/msg00112.html Enric Balletbo i Serra (1): dm ioctl: add a device mapper ioctl function. Will Drewry (1): init: add support to directly boot to a mapped device .../admin-guide/kernel-parameters.rst | 1 + .../admin-guide/kernel-parameters.txt | 3 + Documentation/device-mapper/dm-boot.txt | 87 drivers/md/Makefile | 2 +- drivers/md/dm-boot.c | 433 ++ drivers/md/dm-ioctl.c | 49
[dm-devel] [PATCH v10 1/2] dm ioctl: add a device mapper ioctl function.
From: Enric Balletbo i Serra
Add a dm_ioctl_cmd to issue the equivalent of a DM ioctl call in kernel.
Signed-off-by: Enric Balletbo i Serra
---
Changes since v9:
- https://www.redhat.com/archives/linux-lvm/2018-September/msg00016.html
- Reorganize variables
---
drivers/md/dm-ioctl.c | 49 +++
include/linux/device-mapper.h | 6 +
2 files changed, 55 insertions(+)
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index f666778ad237..ae34c2030a9c 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -2018,3 +2018,52 @@ int dm_copy_name_and_uuid(struct mapped_device *md, char
*name, char *uuid)
return r;
}
+
+/**
+ * dm_ioctl_cmd - Device mapper ioctl's
+ * @command: ioctl command
+ * @param: Pointer to device mapped ioctl struct
+ */
+int dm_ioctl_cmd(uint command, struct dm_ioctl *param)
+{
+ struct file *filp = NULL;
+ size_t input_param_size;
+ int ioctl_flags, r;
+ unsigned int cmd;
+ ioctl_fn fn;
+
+ if (_IOC_TYPE(command) != DM_IOCTL)
+ return -ENOTTY;
+
+ /* DM_DEV_ARM_POLL is not supported */
+ if (command == DM_DEV_ARM_POLL)
+ return -EINVAL;
+
+ cmd = _IOC_NR(command);
+
+ /*
+* Nothing more to do for the version command.
+*/
+ if (cmd == DM_VERSION_CMD)
+ return 0;
+
+ fn = lookup_ioctl(cmd, _flags);
+ if (!fn) {
+ DMWARN("dm_ioctl: unknown command 0x%x", command);
+ return -ENOTTY;
+ }
+
+ input_param_size = param->data_size;
+ r = validate_params(cmd, param);
+ if (r)
+ return r;
+
+ param->data_size = sizeof(*param);
+ r = fn(filp, param, input_param_size);
+
+ if (unlikely(param->flags & DM_BUFFER_FULL_FLAG) &&
+ unlikely(ioctl_flags & IOCTL_FLAGS_NO_PARAMS))
+ DMERR("ioctl %d tried to output some data but has
IOCTL_FLAGS_NO_PARAMS set", cmd);
+
+ return r;
+}
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index d7bee8669f10..8b2e4ae6a498 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
struct dm_dev;
struct dm_target;
@@ -423,6 +424,11 @@ void dm_remap_zone_report(struct dm_target *ti, struct bio
*bio,
sector_t start);
union map_info *dm_get_rq_mapinfo(struct request *rq);
+/*
+ * Device mapper ioctl function.
+ */
+int dm_ioctl_cmd(unsigned int command, struct dm_ioctl *param);
+
struct queue_limits *dm_get_queue_limits(struct mapped_device *md);
/*
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 13/22] libmultipath: clariion checker: leave unsupported paths alone
On Tue, Oct 30, 2018 at 10:06:44PM +0100, Martin Wilck wrote:
> A checker shouldn't set the path state to PATH_DOWN if it fails
> to obtain information about the path in the first place. Add logic
> to the checker to distinguish a failed path from an unsupported path.
>
Reviewed-by: Benjamin Marzinski
> Signed-off-by: Martin Wilck
> ---
> libmultipath/checkers/emc_clariion.c | 54
> 1 file changed, 54 insertions(+)
>
> diff --git a/libmultipath/checkers/emc_clariion.c
> b/libmultipath/checkers/emc_clariion.c
> index f8e55b93..6fc89113 100644
> --- a/libmultipath/checkers/emc_clariion.c
> +++ b/libmultipath/checkers/emc_clariion.c
> @@ -20,6 +20,11 @@
> #define INQUIRY_CMD 0x12
> #define INQUIRY_CMDLEN 6
> #define HEAVY_CHECK_COUNT 10
> +#define SCSI_COMMAND_TERMINATED 0x22
> +#define SCSI_CHECK_CONDITION 0x2
> +#define RECOVERED_ERROR 0x01
> +#define ILLEGAL_REQUEST 0x05
> +#define SG_ERR_DRIVER_SENSE 0x08
>
> /*
> * Mechanism to track CLARiiON inactive snapshot LUs.
> @@ -130,7 +135,9 @@ int libcheck_check (struct checker * c)
> (struct emc_clariion_checker_path_context *)c->context;
> char wwnstr[33];
> int ret;
> + int retry_emc = 5;
>
> +retry:
> memset(_hdr, 0, sizeof (struct sg_io_hdr));
> memset(sense_buffer, 0, 128);
> memset(sb, 0, SENSE_BUFF_LEN);
> @@ -145,13 +152,60 @@ int libcheck_check (struct checker * c)
> io_hdr.timeout = c->timeout * 1000;
> io_hdr.pack_id = 0;
> if (ioctl(c->fd, SG_IO, _hdr) < 0) {
> + if (errno == ENOTTY) {
> + c->msgid = CHECKER_MSGID_UNSUPPORTED;
> + return PATH_WILD;
> + }
> c->msgid = MSG_CLARIION_QUERY_FAILED;
> return PATH_DOWN;
> }
> +
> if (io_hdr.info & SG_INFO_OK_MASK) {
> + switch (io_hdr.host_status) {
> + case DID_BUS_BUSY:
> + case DID_ERROR:
> + case DID_SOFT_ERROR:
> + case DID_TRANSPORT_DISRUPTED:
> + /* Transport error, retry */
> + if (--retry_emc)
> + goto retry;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + if (SCSI_CHECK_CONDITION == io_hdr.status ||
> + SCSI_COMMAND_TERMINATED == io_hdr.status ||
> + SG_ERR_DRIVER_SENSE == (0xf & io_hdr.driver_status)) {
> + if (io_hdr.sbp && (io_hdr.sb_len_wr > 2)) {
> + unsigned char *sbp = io_hdr.sbp;
> + int sense_key;
> +
> + if (sbp[0] & 0x2)
> + sense_key = sbp[1] & 0xf;
> + else
> + sense_key = sbp[2] & 0xf;
> +
> + if (sense_key == ILLEGAL_REQUEST) {
> + c->msgid = CHECKER_MSGID_UNSUPPORTED;
> + return PATH_WILD;
> + } else if (sense_key != RECOVERED_ERROR) {
> + condlog(1, "emc_clariion_checker: INQUIRY
> failed with sense key %02x",
> + sense_key);
> + c->msgid = MSG_CLARIION_QUERY_ERROR;
> + return PATH_DOWN;
> + }
> + }
> + }
> +
> + if (io_hdr.info & SG_INFO_OK_MASK) {
> + condlog(1, "emc_clariion_checker: INQUIRY failed without sense,
> status %02x",
> + io_hdr.status);
> c->msgid = MSG_CLARIION_QUERY_ERROR;
> return PATH_DOWN;
> }
> +
> if (/* Verify the code page - right page & revision */
> sense_buffer[1] != 0xc0 || sense_buffer[9] != 0x00) {
> c->msgid = MSG_CLARIION_UNIT_REPORT;
> --
> 2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] dm crypt: use unsigned long long instead of sector_t to store iv_offset
On 01/11/2018 21:06, Mike Snitzer wrote: > On Thu, Nov 01 2018 at 4:53am -0400, > AliOS system security wrote: > >> The iv_offset in the mapping table of crypt target is a 64bit number >> when iv mode is plain64 or plain64be. It will be assigned to iv_offset of >> struct crypt_config, cc_sector of struct convert_context and iv_sector of >> struct dm_crypt_request. These structures members are defined as a sector_t. >> But sector_t is 32bit when CONFIG_LBDAF is not set in 32bit kernel. In this >> situation sector_t is not big enough to store the 64bit iv_offset. > > I really don't think this is needed. > > cc->iv_offset can only address a the address space used to access the > device. Which is expressed in terms of sectors. Therefore if > CONFIG_LBDAF is not set in 32bit kernel then there is no need to address > beyond that which 'sector_t' addresses. Hi, The iv_offset is Initialization Vector offset (for symmetric block cipher) that is not in principle limited by the device size sector offset. It is just an additional tweak to the cipher mode IV. If we use 32bit only for 64bit IV, it is wrong and a patch is needed. > Please show proof to the contrary if you still think this change is > needed. It is easy to reproduce, even it shows this causes data corruption: # dd if=/dev/zero of=tst.img bs=1M count=1 On 32bit system (use IV offset value that overflows to 64bit; CONFIG_LBDAF if off) (and note IV value overflow in table!!) # echo "tst"|cryptsetup open --type plain -c aes-xts-plain64 --skip 50 tst.img test # sha256sum /dev/mapper/test 533e25c09176632b3794f35303488c4a8f3f965da6ec2df347c168cb6c19 /dev/mapper/test # dmsetup table test 0 2048 crypt aes-xts-plain64 3551657984 7:0 0 On 64bit system (the same image): # echo "tst"|cryptsetup open --type plain -c aes-xts-plain64 --skip 50 tst.img test # sha256sum /dev/mapper/test 5d16160f9d5f8c33d8051e65fdb4f003cc31cd652b5abb08f03aa6fce0df75fc /dev/mapper/test # dmsetup table test 0 2048 crypt aes-xts-plain64 50 7:0 0 This must be fixed, thanks for reporting it! Unfortunately I still do not see the original patch in the list you are replying to. Once it is on the list, I'll ack it. Thanks, Milan -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] dm crypt: use unsigned long long instead of sector_t to store iv_offset
The iv_offset in the mapping table of crypt target is a 64bit number
when iv mode is plain64 or plain64be. It will be assigned to iv_offset of
struct crypt_config, cc_sector of struct convert_context and iv_sector of
struct dm_crypt_request. These structures members are defined as a sector_t.
But sector_t is 32bit when CONFIG_LBDAF is not set in 32bit kernel. In this
situation sector_t is not big enough to store the 64bit iv_offset.
Signed-off-by: AliOS system security
---
drivers/md/dm-crypt.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index b8eec51..49be7a6 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -49,7 +49,7 @@ struct convert_context {
struct bio *bio_out;
struct bvec_iter iter_in;
struct bvec_iter iter_out;
- sector_t cc_sector;
+ unsigned long long cc_sector;
atomic_t cc_pending;
union {
struct skcipher_request *req;
@@ -81,7 +81,7 @@ struct dm_crypt_request {
struct convert_context *ctx;
struct scatterlist sg_in[4];
struct scatterlist sg_out[4];
- sector_t iv_sector;
+ unsigned long long iv_sector;
};
struct crypt_config;
@@ -160,7 +160,7 @@ struct crypt_config {
struct iv_lmk_private lmk;
struct iv_tcw_private tcw;
} iv_gen_private;
- sector_t iv_offset;
+ unsigned long long iv_offset;
unsigned int iv_size;
unsigned short int sector_size;
unsigned char sector_shift;
--
2.7.4
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] dm crypt: use unsigned long long instead of sector_t to store iv_offset
On 2018/11/2 4:06, Mike Snitzer wrote: On Thu, Nov 01 2018 at 4:53am -0400, AliOS system security wrote: The iv_offset in the mapping table of crypt target is a 64bit number when iv mode is plain64 or plain64be. It will be assigned to iv_offset of struct crypt_config, cc_sector of struct convert_context and iv_sector of struct dm_crypt_request. These structures members are defined as a sector_t. But sector_t is 32bit when CONFIG_LBDAF is not set in 32bit kernel. In this situation sector_t is not big enough to store the 64bit iv_offset. I really don't think this is needed. cc->iv_offset can only address a the address space used to access the device. Which is expressed in terms of sectors. Therefore if CONFIG_LBDAF is not set in 32bit kernel then there is no need to address beyond that which 'sector_t' addresses. Please show proof to the contrary if you still think this change is needed. Mike Sorry I made a mistake. I read Documentation/device-mapper/dm-crypt.txt again and found that the IV offset is a sector count. So it make sense to store the iv_offset as a sector_t. In addition, I want to describe what problem I met in the beginning. I made a crypt.img with the crypt param "aes-cbc-plain64 0x1234...5678 1311768465173141112 /dev/loop0 0" in a 32bit kernel with CONFIG_LBDAF=y. The iv_offset is set to a 64bit number and the iv mode is set to plain64. Someday I recompiled my kernel but CONFIG_LBDAF is not set this time. When I reload the crypt.img with the same crypt param in new kernel, I got ioctl(..., DM_TABLE_LOAD, ...) return 0 but the content of /dev/dm-0 is incorrect. So, is this situation, set the iv mode to plain64 or plain64be in a 32bit kernel with CONFIG_LBDAF is not set, a problem? Should the crypt_ctr() return an error code when this happned? Or we just support 64bit iv mode all the time regardless of CONFIG_LBDAF? -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 14/21] libmultipath: hp_sw checker: leave unsupported paths alone
A checker shouldn't set the path state to PATH_DOWN if it fails
to obtain information about the path in the first place. Add logic
to the checker to distinguish a failed path from an unsupported path.
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/checkers/hp_sw.c | 29 +
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/libmultipath/checkers/hp_sw.c b/libmultipath/checkers/hp_sw.c
index d7f1018c..1a820223 100644
--- a/libmultipath/checkers/hp_sw.c
+++ b/libmultipath/checkers/hp_sw.c
@@ -24,6 +24,7 @@
#define SCSI_COMMAND_TERMINATED0x22
#define SG_ERR_DRIVER_SENSE0x08
#define RECOVERED_ERROR0x01
+#define ILLEGAL_REQUEST0x05
#define MX_ALLOC_LEN 255
#define HEAVY_CHECK_COUNT 10
@@ -68,14 +69,19 @@ do_inq(int sg_fd, int cmddt, int evpd, unsigned int pg_op,
io_hdr.sbp = sense_b;
io_hdr.timeout = timeout * 1000;
- if (ioctl(sg_fd, SG_IO, _hdr) < 0)
- return 1;
+ if (ioctl(sg_fd, SG_IO, _hdr) < 0) {
+ if (errno == ENOTTY)
+ return PATH_WILD;
+ else
+ return PATH_DOWN;
+ }
/* treat SG_ERR here to get rid of sg_err.[ch] */
io_hdr.status &= 0x7e;
if ((0 == io_hdr.status) && (0 == io_hdr.host_status) &&
(0 == io_hdr.driver_status))
- return 0;
+ return PATH_UP;
+
if ((SCSI_CHECK_CONDITION == io_hdr.status) ||
(SCSI_COMMAND_TERMINATED == io_hdr.status) ||
(SG_ERR_DRIVER_SENSE == (0xf & io_hdr.driver_status))) {
@@ -86,11 +92,13 @@ do_inq(int sg_fd, int cmddt, int evpd, unsigned int pg_op,
sense_key = sense_buffer[1] & 0xf;
else
sense_key = sense_buffer[2] & 0xf;
- if(RECOVERED_ERROR == sense_key)
- return 0;
+ if (RECOVERED_ERROR == sense_key)
+ return PATH_UP;
+ else if (ILLEGAL_REQUEST == sense_key)
+ return PATH_WILD;
}
}
- return 1;
+ return PATH_DOWN;
}
static int
@@ -122,10 +130,15 @@ do_tur (int fd, unsigned int timeout)
int libcheck_check(struct checker * c)
{
char buff[MX_ALLOC_LEN];
+ int ret = do_inq(c->fd, 0, 1, 0x80, buff, MX_ALLOC_LEN, 0, c->timeout);
- if (0 != do_inq(c->fd, 0, 1, 0x80, buff, MX_ALLOC_LEN, 0, c->timeout)) {
+ if (ret == PATH_WILD) {
+ c->msgid = CHECKER_MSGID_UNSUPPORTED;
+ return ret;
+ }
+ if (ret != PATH_UP) {
c->msgid = CHECKER_MSGID_DOWN;
- return PATH_DOWN;
+ return ret;
};
if (do_tur(c->fd, c->timeout)) {
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 16/21] libmultipath: tur checker: leave unsupported paths alone
A checker shouldn't set the path state to PATH_DOWN if it fails
to obtain information about the path in the first place. Add logic
to the checker to distinguish a failed path from an unsupported path.
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/checkers/tur.c | 4
1 file changed, 4 insertions(+)
diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 22734be2..a27474f9 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -125,6 +125,10 @@ retry:
io_hdr.timeout = timeout * 1000;
io_hdr.pack_id = 0;
if (ioctl(fd, SG_IO, _hdr) < 0) {
+ if (errno == ENOTTY) {
+ *msgid = CHECKER_MSGID_UNSUPPORTED;
+ return PATH_WILD;
+ }
*msgid = CHECKER_MSGID_DOWN;
return PATH_DOWN;
}
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 21/21] libmultipath/checkers: cleanup class/instance model
The checkers code implicitly uses a sort-of OOP class/instance model,
but very clumsily. Separate the checker "class" and "instance" cleanly,
and do a few further cleanups (constifications etc) on the way.
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/checkers.c | 131 ---
libmultipath/checkers.h | 23 ++
libmultipath/checkers/directio.c | 2 +-
libmultipath/checkers/tur.c | 2 +-
libmultipath/print.c | 2 +-
libmultipath/propsel.c | 19 ++---
6 files changed, 89 insertions(+), 90 deletions(-)
diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 90313c35..848c4c34 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -8,6 +8,19 @@
#include "checkers.h"
#include "vector.h"
+struct checker_class {
+ struct list_head node;
+ void *handle;
+ int refcount;
+ int sync;
+ char name[CHECKER_NAME_LEN];
+ int (*check)(struct checker *);
+ int (*init)(struct checker *); /* to allocate the context */
+ void (*free)(struct checker *); /* to free the context */
+ const char **msgtable;
+ short msgtable_size;
+};
+
char *checker_state_names[] = {
"wild",
"unchecked",
@@ -23,38 +36,30 @@ char *checker_state_names[] = {
static LIST_HEAD(checkers);
-char * checker_state_name (int i)
+const char *checker_state_name(int i)
{
return checker_state_names[i];
}
-int init_checkers (char *multipath_dir)
-{
- if (!add_checker(multipath_dir, DEFAULT_CHECKER))
- return 1;
- return 0;
-}
-
-struct checker * alloc_checker (void)
+static struct checker_class *alloc_checker_class(void)
{
- struct checker *c;
+ struct checker_class *c;
- c = MALLOC(sizeof(struct checker));
+ c = MALLOC(sizeof(struct checker_class));
if (c) {
INIT_LIST_HEAD(>node);
c->refcount = 1;
- c->fd = -1;
}
return c;
}
-void free_checker (struct checker * c)
+void free_checker_class(struct checker_class *c)
{
if (!c)
return;
c->refcount--;
if (c->refcount) {
- condlog(3, "%s checker refcount %d",
+ condlog(4, "%s checker refcount %d",
c->name, c->refcount);
return;
}
@@ -71,17 +76,17 @@ void free_checker (struct checker * c)
void cleanup_checkers (void)
{
- struct checker * checker_loop;
- struct checker * checker_temp;
+ struct checker_class *checker_loop;
+ struct checker_class *checker_temp;
list_for_each_entry_safe(checker_loop, checker_temp, , node) {
- free_checker(checker_loop);
+ free_checker_class(checker_loop);
}
}
-struct checker * checker_lookup (char * name)
+static struct checker_class *checker_class_lookup(const char *name)
{
- struct checker * c;
+ struct checker_class *c;
if (!name || !strlen(name))
return NULL;
@@ -92,14 +97,15 @@ struct checker * checker_lookup (char * name)
return NULL;
}
-struct checker * add_checker (char *multipath_dir, char * name)
+static struct checker_class *add_checker_class(const char *multipath_dir,
+ const char *name)
{
char libname[LIB_CHECKER_NAMELEN];
struct stat stbuf;
- struct checker * c;
+ struct checker_class *c;
char *errstr;
- c = alloc_checker();
+ c = alloc_checker_class();
if (!c)
return NULL;
snprintf(c->name, CHECKER_NAME_LEN, "%s", name);
@@ -158,12 +164,11 @@ struct checker * add_checker (char *multipath_dir, char *
name)
c->name, c->msgtable_size);
done:
- c->fd = -1;
c->sync = 1;
list_add(>node, );
return c;
out:
- free_checker(c);
+ free_checker_class(c);
return NULL;
}
@@ -176,16 +181,16 @@ void checker_set_fd (struct checker * c, int fd)
void checker_set_sync (struct checker * c)
{
- if (!c)
+ if (!c || !c->cls)
return;
- c->sync = 1;
+ c->cls->sync = 1;
}
void checker_set_async (struct checker * c)
{
- if (!c)
+ if (!c || !c->cls)
return;
- c->sync = 0;
+ c->cls->sync = 0;
}
void checker_enable (struct checker * c)
@@ -204,11 +209,11 @@ void checker_disable (struct checker * c)
int checker_init (struct checker * c, void ** mpctxt_addr)
{
- if (!c)
+ if (!c || !c->cls)
return 1;
c->mpcontext = mpctxt_addr;
- if (c->init)
- return c->init(c);
+ if (c->cls->init)
+ return c->cls->init(c);
return 0;
}
@@ -220,15 +225,16 @@ void checker_clear (struct checker *c)
void checker_put (struct checker *
[dm-devel] [PATCH v5 06/21] libmultipath/checkers: emc_clariion: use message id
emc_clariion is the only path checker that was using a non-constant
message ("read error" case). This isn't possible with the msgid
approach any more. Use condlog() for the dynamic log message and
simply report "read error" as checker message.
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/checkers/emc_clariion.c | 60 +++-
1 file changed, 42 insertions(+), 18 deletions(-)
diff --git a/libmultipath/checkers/emc_clariion.c
b/libmultipath/checkers/emc_clariion.c
index 9115b1b9..f8e55b93 100644
--- a/libmultipath/checkers/emc_clariion.c
+++ b/libmultipath/checkers/emc_clariion.c
@@ -41,6 +41,35 @@
((struct emc_clariion_checker_LU_context *)\
(*c->mpcontext))->inactive_snap = 0
+enum {
+ MSG_CLARIION_QUERY_FAILED = CHECKER_FIRST_MSGID,
+ MSG_CLARIION_QUERY_ERROR,
+ MSG_CLARIION_PATH_CONFIG,
+ MSG_CLARIION_UNIT_REPORT,
+ MSG_CLARIION_PATH_NOT_AVAIL,
+ MSG_CLARIION_LUN_UNBOUND,
+ MSG_CLARIION_WWN_CHANGED,
+ MSG_CLARIION_READ_ERROR,
+ MSG_CLARIION_PASSIVE_GOOD,
+};
+
+#define _IDX(x) (MSG_CLARIION_ ## x - CHECKER_FIRST_MSGID)
+const char *libcheck_msgtable[] = {
+ [_IDX(QUERY_FAILED)] = ": sending query command failed",
+ [_IDX(QUERY_ERROR)] = ": query command indicates error",
+ [_IDX(PATH_CONFIG)] =
+ ": Path not correctly configured for failover",
+ [_IDX(UNIT_REPORT)] =
+ ": Path unit report page in unknown format",
+ [_IDX(PATH_NOT_AVAIL)] =
+ ": Path not available for normal operations",
+ [_IDX(LUN_UNBOUND)] = ": Logical Unit is unbound or LUNZ",
+ [_IDX(WWN_CHANGED)] = ": Logical Unit WWN has changed",
+ [_IDX(READ_ERROR)] = ": Read error",
+ [_IDX(PASSIVE_GOOD)] = ": Active path is healthy",
+ NULL,
+};
+
struct emc_clariion_checker_path_context {
char wwn[16];
unsigned wwn_set;
@@ -116,17 +145,16 @@ int libcheck_check (struct checker * c)
io_hdr.timeout = c->timeout * 1000;
io_hdr.pack_id = 0;
if (ioctl(c->fd, SG_IO, _hdr) < 0) {
- MSG(c, "emc_clariion_checker: sending query command failed");
+ c->msgid = MSG_CLARIION_QUERY_FAILED;
return PATH_DOWN;
}
if (io_hdr.info & SG_INFO_OK_MASK) {
- MSG(c, "emc_clariion_checker: query command indicates error");
+ c->msgid = MSG_CLARIION_QUERY_ERROR;
return PATH_DOWN;
}
if (/* Verify the code page - right page & revision */
sense_buffer[1] != 0xc0 || sense_buffer[9] != 0x00) {
- MSG(c, "emc_clariion_checker: Path unit report page in "
- "unknown format");
+ c->msgid = MSG_CLARIION_UNIT_REPORT;
return PATH_DOWN;
}
@@ -140,22 +168,19 @@ int libcheck_check (struct checker * c)
((sense_buffer[28] & 0x07) != 0x06))
/* Arraycommpath should be set to 1 */
|| (sense_buffer[30] & 0x04) != 0x04) {
- MSG(c, "emc_clariion_checker: Path not correctly configured "
- "for failover");
+ c->msgid = MSG_CLARIION_PATH_CONFIG;
return PATH_DOWN;
}
if ( /* LUN operations should indicate normal operations */
sense_buffer[48] != 0x00) {
- MSG(c, "emc_clariion_checker: Path not available for normal "
- "operations");
+ c->msgid = MSG_CLARIION_PATH_NOT_AVAIL;
return PATH_SHAKY;
}
if ( /* LUN should at least be bound somewhere and not be LUNZ */
sense_buffer[4] == 0x00) {
- MSG(c, "emc_clariion_checker: Logical Unit is unbound "
- "or LUNZ");
+ c->msgid = MSG_CLARIION_LUN_UNBOUND;
return PATH_DOWN;
}
@@ -166,8 +191,7 @@ int libcheck_check (struct checker * c)
*/
if (ct->wwn_set) {
if (memcmp(ct->wwn, _buffer[10], 16) != 0) {
- MSG(c, "emc_clariion_checker: Logical Unit WWN "
- "has changed!");
+ c->msgid = MSG_CLARIION_WWN_CHANGED;
return PATH_DOWN;
}
} else {
@@ -202,14 +226,15 @@ int libcheck_check (struct checker * c)
condlog(3, "emc_clariion_checker: Active "
"path to inactive snapshot WWN %s.",
wwnstr);
- } else
- MSG(c, "emc_clariion_checker: Read "
+ } else {
+ condlog(3, "emc_clariion_checker: Read "
"error for WWN %s. Sense data are "
[dm-devel] [PATCH v5 15/21] libmultipath: rdac checker: leave unsupported paths alone
A checker shouldn't set the path state to PATH_DOWN if it fails
to obtain information about the path in the first place. Add logic
to the checker to distinguish a failed path from an unsupported path.
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/checkers/rdac.c | 28 +++-
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/libmultipath/checkers/rdac.c b/libmultipath/checkers/rdac.c
index 266f8e10..8a3b73ec 100644
--- a/libmultipath/checkers/rdac.c
+++ b/libmultipath/checkers/rdac.c
@@ -26,6 +26,7 @@
#define SCSI_COMMAND_TERMINATED0x22
#define SG_ERR_DRIVER_SENSE0x08
#define RECOVERED_ERROR0x01
+#define ILLEGAL_REQUEST0x05
#define CURRENT_PAGE_CODE_VALUES 0
@@ -162,14 +163,14 @@ retry:
io_hdr.sbp = sense_b;
io_hdr.timeout = timeout * 1000;
- if (ioctl(sg_fd, SG_IO, _hdr) < 0)
- return 1;
+ if (ioctl(sg_fd, SG_IO, _hdr) < 0 && errno == ENOTTY)
+ return PATH_WILD;
/* treat SG_ERR here to get rid of sg_err.[ch] */
io_hdr.status &= 0x7e;
if ((0 == io_hdr.status) && (0 == io_hdr.host_status) &&
(0 == io_hdr.driver_status))
- return 0;
+ return PATH_UP;
/* check if we need to retry this error */
if (io_hdr.info & SG_INFO_OK_MASK) {
@@ -198,10 +199,14 @@ retry:
else
sense_key = sense_buffer[2] & 0xf;
if (RECOVERED_ERROR == sense_key)
- return 0;
+ return PATH_UP;
+ else if (ILLEGAL_REQUEST == sense_key)
+ return PATH_WILD;
+ condlog(1, "rdac checker: INQUIRY failed with sense key
%02x",
+ sense_key);
}
}
- return 1;
+ return PATH_DOWN;
}
struct volume_access_inq
@@ -290,12 +295,14 @@ int libcheck_check(struct checker * c)
inqfail = 0;
memset(, 0, sizeof(struct volume_access_inq));
- if (0 != do_inq(c->fd, 0xC9, , sizeof(struct volume_access_inq),
- c->timeout)) {
- ret = PATH_DOWN;
+ ret = do_inq(c->fd, 0xC9, , sizeof(struct volume_access_inq),
+c->timeout);
+ if (ret != PATH_UP) {
inqfail = 1;
goto done;
- } else if (((inq.PQ_PDT & 0xE0) == 0x20) || (inq.PQ_PDT & 0x7f)) {
+ }
+
+ if (((inq.PQ_PDT & 0xE0) == 0x20) || (inq.PQ_PDT & 0x7f)) {
/* LUN not connected*/
ret = PATH_DOWN;
goto done;
@@ -332,6 +339,9 @@ int libcheck_check(struct checker * c)
done:
switch (ret) {
+ case PATH_WILD:
+ c->msgid = CHECKER_MSGID_UNSUPPORTED;
+ break;
case PATH_DOWN:
c->msgid = (inqfail ? RDAC_MSGID_INQUIRY_FAILED :
checker_msg_string());
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 04/21] libmultipath/checkers: cciss_tur: use message id
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/checkers/cciss_tur.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/libmultipath/checkers/cciss_tur.c
b/libmultipath/checkers/cciss_tur.c
index 1cab2015..ea843742 100644
--- a/libmultipath/checkers/cciss_tur.c
+++ b/libmultipath/checkers/cciss_tur.c
@@ -42,9 +42,6 @@
#define TUR_CMD_LEN 6
#define HEAVY_CHECK_COUNT 10
-#define MSG_CCISS_TUR_UP "cciss_tur checker reports path is up"
-#define MSG_CCISS_TUR_DOWN "cciss_tur checker reports path is down"
-
struct cciss_tur_checker_context {
void * dummy;
};
@@ -69,7 +66,7 @@ int libcheck_check(struct checker * c)
IOCTL_Command_struct cic; // cciss ioctl command
if ((c->fd) < 0) {
- MSG(c,"no usable fd");
+ c->msgid = CHECKER_MSGID_NO_FD;
ret = -1;
goto out;
}
@@ -79,7 +76,7 @@ int libcheck_check(struct checker * c)
perror("Error: ");
fprintf(stderr, "cciss TUR failed in CCISS_GETLUNINFO: %s\n",
strerror(errno));
- MSG(c,MSG_CCISS_TUR_DOWN);
+ c->msgid = CHECKER_MSGID_DOWN;
ret = PATH_DOWN;
goto out;
} else {
@@ -106,18 +103,18 @@ int libcheck_check(struct checker * c)
if (rc < 0) {
fprintf(stderr, "cciss TUR failed: %s\n",
strerror(errno));
- MSG(c,MSG_CCISS_TUR_DOWN);
+ c->msgid = CHECKER_MSGID_DOWN;
ret = PATH_DOWN;
goto out;
}
if ((cic.error_info.CommandStatus | cic.error_info.ScsiStatus )) {
- MSG(c,MSG_CCISS_TUR_DOWN);
+ c->msgid = CHECKER_MSGID_DOWN;
ret = PATH_DOWN;
goto out;
}
- MSG(c,MSG_CCISS_TUR_UP);
+ c->msgid = CHECKER_MSGID_UP;
ret = PATH_UP;
out:
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 18/21] multipathd: check_path: improve logging for "unusable path" case
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
multipathd/main.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index 2f922db7..c57aa392 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1897,7 +1897,8 @@ check_path (struct vectors * vecs, struct path * pp, int
ticks)
}
if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
- condlog(2, "%s: unusable path", pp->dev);
+ condlog(2, "%s: unusable path - checker failed", pp->dev);
+ LOG_MSG(2, verbosity, pp);
conf = get_multipath_config();
pthread_cleanup_push(put_multipath_config, conf);
pathinfo(pp, conf, 0);
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 19/21] libmultipath: coalesce_paths: improve logging of orphaned paths
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/configure.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 09c3dcf2..ed3e30f5 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1020,14 +1020,18 @@ int coalesce_paths (struct vectors * vecs, vector
newmp, char * refwwid,
}
}
vector_foreach_slot (pathvec, pp1, k) {
- int invalid = 0;
+ int invalid;
/* skip this path for some reason */
/* 1. if path has no unique id or wwid blacklisted */
+ if (strlen(pp1->wwid) == 0) {
+ orphan_path(pp1, "no WWID");
+ continue;
+ }
+
conf = get_multipath_config();
pthread_cleanup_push(put_multipath_config, conf);
- if (strlen(pp1->wwid) == 0 || filter_path(conf, pp1) > 0)
- invalid = 1;
+ invalid = (filter_path(conf, pp1) > 0);
pthread_cleanup_pop(1);
if (invalid) {
orphan_path(pp1, "blacklisted");
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 2/7] multipathd: remove init_path_check_interval()
After "libmultipath: set pp->checkint in store_pathinfo()",
pp-checkint should always be properly initialized, so this code
is not needed any more.
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
multipathd/main.c | 14 --
1 file changed, 14 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index 958545a4..dfc10e54 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2133,19 +2133,6 @@ check_path (struct vectors * vecs, struct path * pp, int
ticks)
return 1;
}
-static void init_path_check_interval(struct vectors *vecs)
-{
- struct config *conf;
- struct path *pp;
- unsigned int i;
-
- vector_foreach_slot (vecs->pathvec, pp, i) {
- conf = get_multipath_config();
- pp->checkint = conf->checkint;
- put_multipath_config(conf);
- }
-}
-
static void *
checkerloop (void *ap)
{
@@ -2729,7 +2716,6 @@ child (void * param)
*/
post_config_state(DAEMON_CONFIGURE);
- init_path_check_interval(vecs);
if (poll_dmevents) {
if (init_dmevent_waiter(vecs)) {
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 20/21] libmultipath: sync_map_state: log failing paths
Emit a log message when force-failing exisiting paths.
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/structs_vec.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index f87d69d4..c85823a0 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -318,8 +318,11 @@ sync_map_state(struct multipath *mpp)
else if ((pp->dmstate == PSTATE_ACTIVE ||
pp->dmstate == PSTATE_UNDEF) &&
(pp->state == PATH_DOWN ||
- pp->state == PATH_SHAKY))
+ pp->state == PATH_SHAKY)) {
+ condlog(2, "sync_map_state: failing %s state %d
dmstate %d",
+ pp->dev, pp->state, pp->dmstate);
dm_fail_path(mpp->alias, pp->dev_t);
+ }
}
}
}
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 5/7] multipathd: set DAEMON_CONFIGURE from uxlsnr thread
Commit ee01e841 had the intention to make multipathd quit if
the client socket couldn't be set up, because the unix socket
listener is vital for signal handling in multipathd.
But during startup, this condition might be lost if the main
thread doesn't wait for the unix listener to initialize.
Fixes: ee01e841 "multipathd: handle errors in uxlsnr as fatal"
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
multipathd/main.c | 43 ---
1 file changed, 32 insertions(+), 11 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index a91a14c6..9052b56d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -207,9 +207,8 @@ static void config_cleanup(void *arg)
pthread_mutex_unlock(_lock);
}
-void post_config_state(enum daemon_status state)
+static void __post_config_state(enum daemon_status state)
{
- pthread_mutex_lock(_lock);
if (state != running_state) {
enum daemon_status old_state = running_state;
@@ -219,7 +218,14 @@ void post_config_state(enum daemon_status state)
do_sd_notify(old_state);
#endif
}
- pthread_mutex_unlock(_lock);
+}
+
+void post_config_state(enum daemon_status state)
+{
+ pthread_mutex_lock(_lock);
+ pthread_cleanup_push(config_cleanup, NULL);
+ __post_config_state(state);
+ pthread_cleanup_pop(1);
}
int set_config_state(enum daemon_status state)
@@ -1515,6 +1521,10 @@ uxlsnrloop (void * ap)
exit_daemon();
goto out_sock;
}
+
+ /* Tell main thread that thread has started */
+ post_config_state(DAEMON_CONFIGURE);
+
set_handler_callback(LIST+PATHS, cli_list_paths);
set_handler_callback(LIST+PATHS+FMT, cli_list_paths_fmt);
set_handler_callback(LIST+PATHS+RAW+FMT, cli_list_paths_raw);
@@ -2734,11 +2744,26 @@ child (void * param)
*/
conf = NULL;
- /*
-* Signal start of configuration
-*/
- post_config_state(DAEMON_CONFIGURE);
+ pthread_cleanup_push(config_cleanup, NULL);
+ pthread_mutex_lock(_lock);
+ __post_config_state(DAEMON_IDLE);
+ rc = pthread_create(_thr, _attr, uxlsnrloop, vecs);
+ if (!rc) {
+ /* Wait for uxlsnr startup */
+ while (running_state == DAEMON_IDLE)
+ pthread_cond_wait(_cond, _lock);
+ }
+ pthread_cleanup_pop(1);
+
+ if (rc) {
+ condlog(0, "failed to create cli listener: %d", rc);
+ goto failed;
+ }
+ else if (running_state != DAEMON_CONFIGURE) {
+ condlog(0, "cli listener failed to start");
+ goto failed;
+ }
if (poll_dmevents) {
if (init_dmevent_waiter(vecs)) {
@@ -2761,10 +2786,6 @@ child (void * param)
goto failed;
}
pthread_attr_destroy(_attr);
- if ((rc = pthread_create(_thr, _attr, uxlsnrloop, vecs))) {
- condlog(0, "failed to create cli listener: %d", rc);
- goto failed;
- }
/*
* start threads
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 0/7] multipathd: make uxlsnr errors really fatal
Hi Christophe, this series, based on top of the recently submitted "various multipath-tools patches (v2)" and "checkers overhaul (v3)" series, fixes a problem that I recently observed: despite ee01e841 "multipathd: handle errors in uxlsnr as fatal", multipathd sometimes doesn't quit when the socket setup fails. While at that, I stumbled upon init_path_check_interval(), wondered about its purpose, and came to the conclusion that can be quite easily obsoleted. Changes v2->v3: 04/07: Fixed cleanup code path (Ben). Changes v1->v2: 04/07: Fixed problem observed by Ben. Martin Wilck (7): libmultipath: set pp->checkint in store_pathinfo() multipathd: remove init_path_check_interval() multipathd: print error message if checkint is not initialized multipathd: open client socket early multipathd: set DAEMON_CONFIGURE from uxlsnr thread multipathd: make DAEMON_SHUTDOWN a terminal state multipathd: only grab conf once for filter_path() libmultipath/defaults.h | 1 + libmultipath/dict.c | 14 - libmultipath/discovery.c | 1 + libmultipath/structs.c| 1 + multipathd/cli_handlers.c | 10 ++-- multipathd/main.c | 109 +- multipathd/uxlsnr.c | 14 + multipathd/uxlsnr.h | 5 +- 8 files changed, 98 insertions(+), 57 deletions(-) -- 2.19.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 3/7] multipathd: print error message if checkint is not initialized
This is just a safety measure in case I overlooked something wrt
the checkint initialization. It could be reverted once we know the
error message isn't triggered.
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/defaults.h | 1 +
libmultipath/dict.c | 14 --
libmultipath/structs.c | 1 +
multipathd/main.c | 6 ++
4 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index 7f3839fc..65769398 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -46,6 +46,7 @@
#define DEFAULT_UNKNOWN_FIND_MULTIPATHS_TIMEOUT 1
#define DEFAULT_ALL_TG_PT ALL_TG_PT_OFF
+#define CHECKINT_UNDEF (~0U)
#define DEFAULT_CHECKINT 5
#define MAX_CHECKINT(a)(a << 2)
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index c3f5a6e6..a81c051f 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -264,7 +264,17 @@ snprint_mp_ ## option (struct config *conf, char * buff,
int len, \
return function (buff, len, mpe->option); \
}
-declare_def_handler(checkint, set_int)
+static int checkint_handler(struct config *conf, vector strvec)
+{
+ int rc = set_int(strvec, >checkint);
+
+ if (rc)
+ return rc;
+ if (conf->checkint == CHECKINT_UNDEF)
+ conf->checkint--;
+ return 0;
+}
+
declare_def_snprint(checkint, print_int)
declare_def_handler(max_checkint, set_int)
@@ -1563,7 +1573,7 @@ init_keywords(vector keywords)
{
install_keyword_root("defaults", NULL);
install_keyword("verbosity", _verbosity_handler,
_def_verbosity);
- install_keyword("polling_interval", _checkint_handler,
_def_checkint);
+ install_keyword("polling_interval", _handler,
_def_checkint);
install_keyword("max_polling_interval", _max_checkint_handler,
_def_max_checkint);
install_keyword("reassign_maps", _reassign_maps_handler,
_def_reassign_maps);
install_keyword("multipath_dir", _multipath_dir_handler,
_def_multipath_dir);
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index caa178a6..fee899bd 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -100,6 +100,7 @@ alloc_path (void)
pp->fd = -1;
pp->tpgs = TPGS_UNDEF;
pp->priority = PRIO_UNDEF;
+ pp->checkint = CHECKINT_UNDEF;
checker_clear(>checker);
dm_path_to_gen(pp)->ops = _gen_path_ops;
pp->hwe = vector_alloc();
diff --git a/multipathd/main.c b/multipathd/main.c
index dfc10e54..a022bdb5 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1842,6 +1842,12 @@ check_path (struct vectors * vecs, struct path * pp, int
ticks)
max_checkint = conf->max_checkint;
verbosity = conf->verbosity;
put_multipath_config(conf);
+
+ if (pp->checkint == CHECKINT_UNDEF) {
+ condlog(0, "%s: BUG: checkint is not set", pp->dev);
+ pp->checkint = checkint;
+ };
+
if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {
if (pp->retriggers < retrigger_tries) {
condlog(2, "%s: triggering change event to
reinitialize",
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 7/7] multipathd: only grab conf once for filter_path()
This saves a possibly large number of cleanup push/pop calls and
slightly improves readability.
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
multipathd/main.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index 39f8..a445b050 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2340,16 +2340,17 @@ configure (struct vectors * vecs)
goto fail;
}
+ conf = get_multipath_config();
+ pthread_cleanup_push(put_multipath_config, conf);
vector_foreach_slot (vecs->pathvec, pp, i){
- conf = get_multipath_config();
- pthread_cleanup_push(put_multipath_config, conf);
if (filter_path(conf, pp) > 0){
vector_del_slot(vecs->pathvec, i);
free_path(pp);
i--;
}
- pthread_cleanup_pop(1);
}
+ pthread_cleanup_pop(1);
+
if (map_discovery(vecs)) {
condlog(0, "configure failed at map discovery");
goto fail;
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 6/7] multipathd: make DAEMON_SHUTDOWN a terminal state
It can happen that, before the main thread reacts on DAEMON_SHUTDOWN
and starts cancelling threads, another thread resets the state to
something else. Fix that.
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
multipathd/cli_handlers.c | 9 +++--
multipathd/main.c | 10 +++---
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 4aea4ce7..a0d57a53 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1124,12 +1124,17 @@ cli_switch_group(void * v, char ** reply, int * len,
void * data)
int
cli_reconfigure(void * v, char ** reply, int * len, void * data)
{
+ int rc;
+
condlog(2, "reconfigure (operator)");
- if (set_config_state(DAEMON_CONFIGURE) == ETIMEDOUT) {
+ rc = set_config_state(DAEMON_CONFIGURE);
+ if (rc == ETIMEDOUT) {
condlog(2, "timeout starting reconfiguration");
return 1;
- }
+ } else if (rc == EINVAL)
+ /* daemon shutting down */
+ return 1;
return 0;
}
diff --git a/multipathd/main.c b/multipathd/main.c
index 9052b56d..39f8 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -209,7 +209,7 @@ static void config_cleanup(void *arg)
static void __post_config_state(enum daemon_status state)
{
- if (state != running_state) {
+ if (state != running_state && running_state != DAEMON_SHUTDOWN) {
enum daemon_status old_state = running_state;
running_state = state;
@@ -237,7 +237,9 @@ int set_config_state(enum daemon_status state)
if (running_state != state) {
enum daemon_status old_state = running_state;
- if (running_state != DAEMON_IDLE) {
+ if (running_state == DAEMON_SHUTDOWN)
+ rc = EINVAL;
+ else if (running_state != DAEMON_IDLE) {
struct timespec ts;
clock_gettime(CLOCK_MONOTONIC, );
@@ -2212,7 +2214,9 @@ checkerloop (void *ap)
if (rc == ETIMEDOUT) {
condlog(4, "timeout waiting for DAEMON_IDLE");
continue;
- }
+ } else if (rc == EINVAL)
+ /* daemon shutdown */
+ break;
pthread_cleanup_push(cleanup_lock, >lock);
lock(>lock);
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 00/21] libmultipath: checkers overhaul
Hi Christophe, This is v5 of my "checkers overhaul" series. Changed wrt v4 are 03/22, 11/22, and 21/22. I re-post the whole series to avoid confusion. This series starts with a few minor fixes and then attempts an overhaul of the checker code. First, there's a block of patches to get rid of the "message" char array in struct checker, replacing it with an integer. This topic had been touched in recent discussion between Ben and myself. You may want to collaps the patches in this block (03/21-11/21) into one when commiting; compilation errors will arise if only part of them is a applied. The next larger block fixes problems with checkers that try to check unsupported devices. It's an interesting experience to configure wrong checkers for the existing devices and see what happens. With these patches, paths won't be falsely teared down any more in such situations. The last patch cleans up the checker data structure by splitting it into a "checker class" and the path "checker instance". There's more work to do in this area, but this is a start. changes v4->v5 - 03/21: made checker_message() const again, as 22/22 is gone. Got rid of the static buffer in checker_message() by simply returning the constant message strings, as suggested by Ben in his review of v4. This implies printf format changes in callers; changed format in get_state(). - 11/21: Changed printf format in check_path() (see above). - 21/22: rebased, no actual changes (kept Ben's Reviewed-by). - 22/22: dropped, obsolete. Changes v3->v4: - 03/22: renamed CHECKER_LAST_GENERIC_MSGID -> CHECKER_GENERIC_MSGTABLE_SIZE (Ben). - 12/22: rebased on top of changed 03/22. Changes v2->v3: - 03/22: fixed one minor issue mentioned by Ben; reverted the const-ification of checker_message(), as it will be reverted in 22/22 anyway. - 13/22: fix clariion checker semantics (Ben). - 21/22: rebased on top of updated 03/22. - 22/22: fix thread-safety issue from 03/22 (Ben). Changes v1->v2: - 11/22: rebased on top of "various multipath-tools patches" series Martin Wilck (21): libmultipath: fix use of uninitialized memory in write() libmultipath: fix memory leaks from scandir() use libmultipath/checkers: replace message by msgid libmultipath/checkers: cciss_tur: use message id libmultipath/checkers: directio: use message id libmultipath/checkers: emc_clariion: use message id libmultipath/checkers: hp_sw: use message id libmultipath/checkers: rdac: use message id libmultipath/checkers: readsector0: use message id libmultipath/checkers: tur: use message id multipathd: improve checker message logging libmultipath/checkers: support unsupported paths libmultipath: clariion checker: leave unsupported paths alone libmultipath: hp_sw checker: leave unsupported paths alone libmultipath: rdac checker: leave unsupported paths alone libmultipath: tur checker: leave unsupported paths alone libmultipath: pathinfo: don't blank wwid if checker fails multipathd: check_path: improve logging for "unusable path" case libmultipath: coalesce_paths: improve logging of orphaned paths libmultipath: sync_map_state: log failing paths libmultipath/checkers: cleanup class/instance model libmultipath/checkers.c | 187 +-- libmultipath/checkers.h | 69 ++ libmultipath/checkers/cciss_tur.c| 13 +- libmultipath/checkers/directio.c | 29 +++-- libmultipath/checkers/emc_clariion.c | 114 +--- libmultipath/checkers/hp_sw.c| 39 +++--- libmultipath/checkers/rdac.c | 92 + libmultipath/checkers/readsector0.c | 7 +- libmultipath/checkers/tur.c | 60 + libmultipath/config.c| 10 +- libmultipath/configure.c | 10 +- libmultipath/discovery.c | 11 +- libmultipath/foreign.c | 5 +- libmultipath/foreign/nvme.c | 6 +- libmultipath/print.c | 2 +- libmultipath/propsel.c | 19 +-- libmultipath/structs_vec.c | 5 +- libmultipath/sysfs.c | 5 +- libmultipath/util.c | 9 ++ libmultipath/util.h | 9 ++ multipathd/main.c| 38 -- 21 files changed, 497 insertions(+), 242 deletions(-) -- 2.19.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 11/21] multipathd: improve checker message logging
Don't rely on any variables being defined in LOG_MSG.
If message log level is low, don't bother to fetch the message.
Signed-off-by: Martin Wilck
---
multipathd/main.c | 35 ---
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index bf5f12a6..2f922db7 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -89,12 +89,24 @@ static int use_watchdog;
#define FILE_NAME_SIZE 256
#define CMDSIZE 160
-#define LOG_MSG(a, b) \
-do { \
- if (pp->offline) \
- condlog(a, "%s: %s - path offline", pp->mpp->alias, pp->dev); \
- else if (strlen(b)) \
- condlog(a, "%s: %s - %s", pp->mpp->alias, pp->dev, b); \
+#define LOG_MSG(lvl, verb, pp) \
+do { \
+ if (lvl <= verb) { \
+ if (pp->offline)\
+ condlog(lvl, "%s: %s - path offline", \
+ pp->mpp->alias, pp->dev); \
+ else { \
+ const char *__m = \
+ checker_message(>checker); \
+ \
+ if (strlen(__m)) \
+ condlog(lvl, "%s: %s - %s checker%s", \
+ pp->mpp->alias, \
+ pp->dev, \
+ checker_name(>checker), \
+ __m); \
+ } \
+ } \
} while(0)
struct mpath_event_param
@@ -1811,7 +1823,7 @@ check_path (struct vectors * vecs, struct path * pp, int
ticks)
int add_active;
int disable_reinstate = 0;
int oldchkrstate = pp->chkrstate;
- int retrigger_tries, checkint, max_checkint;
+ int retrigger_tries, checkint, max_checkint, verbosity;
struct config *conf;
int ret;
@@ -1828,6 +1840,7 @@ check_path (struct vectors * vecs, struct path * pp, int
ticks)
retrigger_tries = conf->retrigger_tries;
checkint = conf->checkint;
max_checkint = conf->max_checkint;
+ verbosity = conf->verbosity;
put_multipath_config(conf);
if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {
if (pp->retriggers < retrigger_tries) {
@@ -1970,7 +1983,7 @@ check_path (struct vectors * vecs, struct path * pp, int
ticks)
int oldstate = pp->state;
pp->state = newstate;
- LOG_MSG(1, checker_message(>checker));
+ LOG_MSG(1, verbosity, pp);
/*
* upon state change, reset the checkint
@@ -2058,7 +2071,7 @@ check_path (struct vectors * vecs, struct path * pp, int
ticks)
return 0;
}
} else {
- LOG_MSG(4, checker_message(>checker));
+ LOG_MSG(4, verbosity, pp);
if (pp->checkint != max_checkint) {
/*
* double the next check delay.
@@ -2088,9 +2101,9 @@ check_path (struct vectors * vecs, struct path * pp, int
ticks)
log_checker_err = conf->log_checker_err;
put_multipath_config(conf);
if (log_checker_err == LOG_CHKR_ERR_ONCE)
- LOG_MSG(3, checker_message(>checker));
+ LOG_MSG(3, verbosity, pp);
else
- LOG_MSG(2, checker_message(>checker));
+ LOG_MSG(2, verbosity, pp);
}
}
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 10/21] libmultipath/checkers: tur: use message id
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/checkers/tur.c | 54 -
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index a6c88eb2..22734be2 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -29,12 +29,19 @@
#define TUR_CMD_LEN 6
#define HEAVY_CHECK_COUNT 10
-#define MSG_TUR_UP "tur checker reports path is up"
-#define MSG_TUR_DOWN "tur checker reports path is down"
-#define MSG_TUR_GHOST "tur checker reports path is in standby state"
-#define MSG_TUR_RUNNING"tur checker still running"
-#define MSG_TUR_TIMEOUT"tur checker timed out"
-#define MSG_TUR_FAILED "tur checker failed to initialize"
+enum {
+ MSG_TUR_RUNNING = CHECKER_FIRST_MSGID,
+ MSG_TUR_TIMEOUT,
+ MSG_TUR_FAILED,
+};
+
+#define _IDX(x) (MSG_ ## x - CHECKER_FIRST_MSGID)
+const char *libcheck_msgtable[] = {
+ [_IDX(TUR_RUNNING)] = " still running",
+ [_IDX(TUR_TIMEOUT)] = " timed out",
+ [_IDX(TUR_FAILED)] = " failed to initialize",
+ NULL,
+};
struct tur_checker_context {
dev_t devt;
@@ -47,7 +54,7 @@ struct tur_checker_context {
pthread_mutex_t lock;
pthread_cond_t active;
int holders; /* uatomic access only */
- char message[CHECKER_MSG_LEN];
+ int msgid;
};
int libcheck_init (struct checker * c)
@@ -99,7 +106,7 @@ void libcheck_free (struct checker * c)
}
static int
-tur_check(int fd, unsigned int timeout, char *msg)
+tur_check(int fd, unsigned int timeout, short *msgid)
{
struct sg_io_hdr io_hdr;
unsigned char turCmdBlk[TUR_CMD_LEN] = { 0x00, 0, 0, 0, 0, 0 };
@@ -118,7 +125,7 @@ retry:
io_hdr.timeout = timeout * 1000;
io_hdr.pack_id = 0;
if (ioctl(fd, SG_IO, _hdr) < 0) {
- snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_DOWN);
+ *msgid = CHECKER_MSGID_DOWN;
return PATH_DOWN;
}
if ((io_hdr.status & 0x7e) == 0x18) {
@@ -126,7 +133,7 @@ retry:
* SCSI-3 arrays might return
* reservation conflict on TUR
*/
- snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_UP);
+ *msgid = CHECKER_MSGID_UP;
return PATH_UP;
}
if (io_hdr.info & SG_INFO_OK_MASK) {
@@ -171,14 +178,14 @@ retry:
* LOGICAL UNIT NOT ACCESSIBLE,
* TARGET PORT IN STANDBY STATE
*/
- snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_GHOST);
+ *msgid = CHECKER_MSGID_GHOST;
return PATH_GHOST;
}
}
- snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_DOWN);
+ *msgid = CHECKER_MSGID_DOWN;
return PATH_DOWN;
}
- snprintf(msg, CHECKER_MSG_LEN, MSG_TUR_UP);
+ *msgid = CHECKER_MSGID_UP;
return PATH_UP;
}
@@ -244,7 +251,7 @@ static void *tur_thread(void *ctx)
{
struct tur_checker_context *ct = ctx;
int state, running;
- char msg[CHECKER_MSG_LEN];
+ short msgid;
/* This thread can be canceled, so setup clean up */
tur_thread_cleanup_push(ct);
@@ -254,13 +261,13 @@ static void *tur_thread(void *ctx)
minor(ct->devt));
tur_deep_sleep(ct);
- state = tur_check(ct->fd, ct->timeout, msg);
+ state = tur_check(ct->fd, ct->timeout, );
pthread_testcancel();
/* TUR checker done */
pthread_mutex_lock(>lock);
ct->state = state;
- strlcpy(ct->message, msg, sizeof(ct->message));
+ ct->msgid = msgid;
pthread_cond_signal(>active);
pthread_mutex_unlock(>lock);
@@ -313,7 +320,7 @@ int libcheck_check(struct checker * c)
return PATH_UNCHECKED;
if (c->sync)
- return tur_check(c->fd, c->timeout, c->message);
+ return tur_check(c->fd, c->timeout, >msgid);
/*
* Async mode
@@ -325,13 +332,12 @@ int libcheck_check(struct checker * c)
pthread_cancel(ct->thread);
condlog(3, "%d:%d : tur checker timeout",
major(ct->devt), minor(ct->devt));
- MSG(c, MSG_TUR_TIMEOUT);
+ c->msgid = MSG_TUR_TIMEOUT;
tur_status = PATH_TIMEOUT;
} else {
pthread_mutex_lock(>lock);
tur_status = ct->state;
- strlcpy(c->message, ct->message,
- sizeof(c->message));
+ c->msgid = ct->msgid;
[dm-devel] [PATCH v5 09/21] libmultipath/checkers: readsector0: use message id
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/checkers/readsector0.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/libmultipath/checkers/readsector0.c
b/libmultipath/checkers/readsector0.c
index 1c2a868e..cf79e067 100644
--- a/libmultipath/checkers/readsector0.c
+++ b/libmultipath/checkers/readsector0.c
@@ -6,9 +6,6 @@
#include "checkers.h"
#include "libsg.h"
-#define MSG_READSECTOR0_UP "readsector0 checker reports path is up"
-#define MSG_READSECTOR0_DOWN "readsector0 checker reports path is down"
-
struct readsector0_checker_context {
void * dummy;
};
@@ -35,10 +32,10 @@ int libcheck_check (struct checker * c)
switch (ret)
{
case PATH_DOWN:
- MSG(c, MSG_READSECTOR0_DOWN);
+ c->msgid = CHECKER_MSGID_DOWN;
break;
case PATH_UP:
- MSG(c, MSG_READSECTOR0_UP);
+ c->msgid = CHECKER_MSGID_UP;
break;
default:
break;
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 12/21] libmultipath/checkers: support unsupported paths
We should be able to distinguish the case where a checker
determines a path to be positively down from the case where
the checker fails to obtain necessary information, e.g.
because of a configuration problem (wrong checker).
Use PATH_WILD for the latter case, as it's hardly used now.
Provide a generic message for the situation that a path
checker can't handle a certain path.
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/checkers.c | 1 +
libmultipath/checkers.h | 7 +--
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index a1b5cb45..90313c35 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -279,6 +279,7 @@ static const char
*generic_msg[CHECKER_GENERIC_MSGTABLE_SIZE] = {
[CHECKER_MSGID_UP] = " reports path is up",
[CHECKER_MSGID_DOWN] = " reports path is down",
[CHECKER_MSGID_GHOST] = " reports path is ghost",
+ [CHECKER_MSGID_UNSUPPORTED] = " doesn't support this device",
};
const char *checker_message(const struct checker *c)
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index 8e26f1df..3cd46bdf 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -10,8 +10,10 @@
* Userspace (multipath/multipathd) path states
*
* PATH_WILD:
- * - Use: None of the checkers (returned if we don't have an fd)
- * - Description: Corner case where "fd < 0" for path fd (see checker_check())
+ * - Use: Any checker
+ * - Description: Corner case where "fd < 0" for path fd (see checker_check()),
+ * or where a checker detects an unsupported device
+ * (e.g. wrong checker configured for a given device).
*
* PATH_UNCHECKED:
* - Use: Only in directio checker
@@ -108,6 +110,7 @@ enum {
CHECKER_MSGID_UP,
CHECKER_MSGID_DOWN,
CHECKER_MSGID_GHOST,
+ CHECKER_MSGID_UNSUPPORTED,
CHECKER_GENERIC_MSGTABLE_SIZE,
CHECKER_FIRST_MSGID = 100, /* lowest msgid for checkers */
CHECKER_MSGTABLE_SIZE = 100,/* max msg table size for checkers */
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 4/7] multipathd: open client socket early
Open the unix socket in multipathd code and pass the fd to
uxsock_listen(). This will enable us to make the main thread
wait for successful socket initialization in a follow-up patch.
Signed-off-by: Martin Wilck
---
multipathd/main.c | 27 ++-
multipathd/uxlsnr.c | 14 ++
multipathd/uxlsnr.h | 5 +++--
3 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index a022bdb5..a91a14c6 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -66,6 +66,7 @@ static int use_watchdog;
#include "pgpolicies.h"
#include "uevent.h"
#include "log.h"
+#include "uxsock.h"
#include "mpath_cmd.h"
#include "mpath_persist.h"
@@ -1496,12 +1497,24 @@ uevqloop (void * ap)
static void *
uxlsnrloop (void * ap)
{
+ long ux_sock;
+
+ pthread_cleanup_push(rcu_unregister, NULL);
+ rcu_register_thread();
+
+ ux_sock = ux_socket_listen(DEFAULT_SOCKET);
+ if (ux_sock == -1) {
+ condlog(1, "could not create uxsock: %d", errno);
+ exit_daemon();
+ goto out;
+ }
+ pthread_cleanup_push(uxsock_cleanup, (void *)ux_sock);
+
if (cli_init()) {
condlog(1, "Failed to init uxsock listener");
- return NULL;
+ exit_daemon();
+ goto out_sock;
}
- pthread_cleanup_push(rcu_unregister, NULL);
- rcu_register_thread();
set_handler_callback(LIST+PATHS, cli_list_paths);
set_handler_callback(LIST+PATHS+FMT, cli_list_paths_fmt);
set_handler_callback(LIST+PATHS+RAW+FMT, cli_list_paths_raw);
@@ -1556,8 +1569,12 @@ uxlsnrloop (void * ap)
set_handler_callback(UNSETPRKEY+MAP, cli_unsetprkey);
umask(077);
- uxsock_listen(_trigger, ap);
- pthread_cleanup_pop(1);
+ uxsock_listen(_trigger, ux_sock, ap);
+
+out_sock:
+ pthread_cleanup_pop(1); /* uxsock_cleanup */
+out:
+ pthread_cleanup_pop(1); /* rcu_unregister */
return NULL;
}
diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 6f63..773bc878 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -165,24 +165,15 @@ void uxsock_cleanup(void *arg)
/*
* entry point
*/
-void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void * trigger_data)
+void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
+void * trigger_data)
{
- long ux_sock;
int rlen;
char *inbuf;
char *reply;
sigset_t mask;
int old_clients = MIN_POLLS;
- ux_sock = ux_socket_listen(DEFAULT_SOCKET);
-
- if (ux_sock == -1) {
- condlog(1, "could not create uxsock: %d", errno);
- exit_daemon();
- }
-
- pthread_cleanup_push(uxsock_cleanup, (void *)ux_sock);
-
condlog(3, "uxsock: startup listener");
polls = (struct pollfd *)MALLOC((MIN_POLLS + 1) * sizeof(struct
pollfd));
if (!polls) {
@@ -322,6 +313,5 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void
* trigger_data)
}
}
- pthread_cleanup_pop(1);
return NULL;
}
diff --git a/multipathd/uxlsnr.h b/multipathd/uxlsnr.h
index d51a8f99..18f008d7 100644
--- a/multipathd/uxlsnr.h
+++ b/multipathd/uxlsnr.h
@@ -5,7 +5,8 @@
typedef int (uxsock_trigger_fn)(char *, char **, int *, bool, void *);
-void * uxsock_listen(uxsock_trigger_fn uxsock_trigger,
-void * trigger_data);
+void uxsock_cleanup(void *arg);
+void *uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
+ void * trigger_data);
#endif
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 02/21] libmultipath: fix memory leaks from scandir() use
scandir() users must not only free the resulting dirent* array,
but also every member. Add a cleanup function, and fix the
existing users of scandir() in libmultipath.
Add a small helper macro for casting function pointers to the
type pthread_cleanup_push() expects.
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/config.c | 10 --
libmultipath/foreign.c | 5 -
libmultipath/foreign/nvme.c | 6 +-
libmultipath/sysfs.c| 5 -
libmultipath/util.c | 9 +
libmultipath/util.h | 9 +
6 files changed, 35 insertions(+), 9 deletions(-)
diff --git a/libmultipath/config.c b/libmultipath/config.c
index 0aef186a..5af7af58 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -639,17 +639,13 @@ free_config (struct config * conf)
FREE(conf);
}
-static void free_namelist(void *nl)
-{
- free(nl);
-}
-
/* if multipath fails to process the config directory, it should continue,
* with just a warning message */
static void
process_config_dir(struct config *conf, vector keywords, char *dir)
{
struct dirent **namelist;
+ struct scandir_result sr;
int i, n;
char path[LINE_MAX];
int old_hwtable_size;
@@ -669,7 +665,9 @@ process_config_dir(struct config *conf, vector keywords,
char *dir)
return;
} else if (n == 0)
return;
- pthread_cleanup_push(free_namelist, namelist);
+ sr.di = namelist;
+ sr.n = n;
+ pthread_cleanup_push_cast(free_scandir_result, );
for (i = 0; i < n; i++) {
if (!strstr(namelist[i]->d_name, ".conf"))
continue;
diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
index 80b399ba..48e8d247 100644
--- a/libmultipath/foreign.c
+++ b/libmultipath/foreign.c
@@ -115,6 +115,7 @@ static int _init_foreign(const char *multipath_dir)
{
char pathbuf[PATH_MAX];
struct dirent **di;
+ struct scandir_result sr;
int r, i;
foreigns = vector_alloc();
@@ -135,7 +136,9 @@ static int _init_foreign(const char *multipath_dir)
return -r;
}
- pthread_cleanup_push(free, di);
+ sr.di = di;
+ sr.n = r;
+ pthread_cleanup_push_cast(free_scandir_result, );
for (i = 0; i < r; i++) {
const char *msg, *fn, *c;
struct foreign *fgn;
diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
index 8887a755..c753a747 100644
--- a/libmultipath/foreign/nvme.c
+++ b/libmultipath/foreign/nvme.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include "util.h"
#include "vector.h"
#include "generic.h"
#include "foreign.h"
@@ -534,6 +535,7 @@ static void _find_controllers(struct context *ctx, struct
nvme_map *map)
{
char pathbuf[PATH_MAX], realbuf[PATH_MAX];
struct dirent **di = NULL;
+ struct scandir_result sr;
struct udev_device *subsys;
struct nvme_path *path;
int r, i, n;
@@ -568,7 +570,9 @@ static void _find_controllers(struct context *ctx, struct
nvme_map *map)
return;
}
- pthread_cleanup_push(free, di);
+ sr.di = di;
+ sr.n = r;
+ pthread_cleanup_push_cast(free_scandir_result, );
for (i = 0; i < r; i++) {
char *fn = di[i]->d_name;
struct udev_device *ctrl, *udev;
diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index b7dacaad..558c8d6a 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -303,6 +303,7 @@ static void close_fd(void *arg)
bool sysfs_is_multipathed(const struct path *pp)
{
char pathbuf[PATH_MAX];
+ struct scandir_result sr;
struct dirent **di;
int n, r, i;
bool found = false;
@@ -323,7 +324,9 @@ bool sysfs_is_multipathed(const struct path *pp)
return false;
}
- pthread_cleanup_push(free, di);
+ sr.di = di;
+ sr.n = r;
+ pthread_cleanup_push_cast(free_scandir_result, );
for (i = 0; i < r && !found; i++) {
long fd;
int nr;
diff --git a/libmultipath/util.c b/libmultipath/util.c
index d08112db..66c47611 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -496,3 +496,12 @@ void set_max_fds(int max_fds)
}
}
}
+
+void free_scandir_result(struct scandir_result *res)
+{
+ int i;
+
+ for (i = 0; i < res->n; i++)
+ FREE(res->di[i]);
+ FREE(res->di);
+}
diff --git a/libmultipath/util.h b/libmultipath/util.h
index c2462950..a818e29a 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -30,4 +30,13 @@ void set_max_fds(int max_fds);
#define safe_snprintf(var, size, format, args...) \
snprintf(var, size, format, ##args) >= size
+#define pthread_cleanup_push_cast(f, arg) \
+ pthread_cleanup_push(((void (*)(void *))),
[dm-devel] [PATCH v5 01/21] libmultipath: fix use of uninitialized memory in write()
valgrind complained about this.
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/discovery.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 873035e5..3550c3a7 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -661,7 +661,7 @@ sysfs_set_session_tmo(struct multipath *mpp, struct path
*pp)
} else {
snprintf(value, 11, "%u", mpp->fast_io_fail);
if (sysfs_attr_set_value(session_dev, "recovery_tmo",
-value, 11) <= 0) {
+value, strlen(value)) <= 0) {
condlog(3, "%s: Failed to set recovery_tmo, "
" error %d", pp->dev, errno);
}
@@ -693,7 +693,7 @@ sysfs_set_nexus_loss_tmo(struct multipath *mpp, struct path
*pp)
if (mpp->dev_loss) {
snprintf(value, 11, "%u", mpp->dev_loss);
if (sysfs_attr_set_value(sas_dev, "I_T_nexus_loss_timeout",
-value, 11) <= 0)
+value, strlen(value)) <= 0)
condlog(3, "%s: failed to update "
"I_T Nexus loss timeout, error %d",
pp->dev, errno);
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 03/21] libmultipath/checkers: replace message by msgid
Replace the character array "message" in struct checker with
a "message ID" field.
The generic checker code defines a couple of standard message IDs
and corresponding messages. Checker-specific message IDs start
at CHECKER_FIRST_MSG. Checkers that implement specific message
IDs must provide a table for converting the IDs into actual log
messages.
This simplifies the checker data structure and the handling of
checker messages in the critical checker code path. It comes at
the cost that only constant message strings are supported. It
turns out that only a single checker log message (in the emc_clariion
checker) was dynamically generated, and the missing information can
be provided with a standard condlog message.
Follow-up patches implement this for the existing checkers.
Signed-off-by: Martin Wilck
---
libmultipath/checkers.c | 61 +---
libmultipath/checkers.h | 43
libmultipath/discovery.c | 4 +--
3 files changed, 90 insertions(+), 18 deletions(-)
diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 0bacc864..a1b5cb45 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -141,6 +141,22 @@ struct checker * add_checker (char *multipath_dir, char *
name)
if (!c->free)
goto out;
+ c->msgtable_size = 0;
+ c->msgtable = dlsym(c->handle, "libcheck_msgtable");
+
+ if (c->msgtable != NULL) {
+ const char **p;
+
+ for (p = c->msgtable;
+*p && (p - c->msgtable) < CHECKER_MSGTABLE_SIZE; p++)
+ /* nothing */;
+
+ c->msgtable_size = p - c->msgtable;
+ } else
+ c->msgtable_size = 0;
+ condlog(3, "checker %s: message table size = %d",
+ c->name, c->msgtable_size);
+
done:
c->fd = -1;
c->sync = 1;
@@ -222,16 +238,16 @@ int checker_check (struct checker * c, int path_state)
if (!c)
return PATH_WILD;
- c->message[0] = '\0';
+ c->msgid = CHECKER_MSGID_NONE;
if (c->disable) {
- MSG(c, "checker disabled");
+ c->msgid = CHECKER_MSGID_DISABLED;
return PATH_UNCHECKED;
}
if (!strncmp(c->name, NONE, 4))
return path_state;
if (c->fd < 0) {
- MSG(c, "no usable fd");
+ c->msgid = CHECKER_MSGID_NO_FD;
return PATH_WILD;
}
r = c->check(c);
@@ -248,25 +264,48 @@ int checker_selected (struct checker * c)
return (c->check) ? 1 : 0;
}
-char * checker_name (struct checker * c)
+const char *checker_name(const struct checker *c)
{
if (!c)
return NULL;
return c->name;
}
-char * checker_message (struct checker * c)
+static const char *generic_msg[CHECKER_GENERIC_MSGTABLE_SIZE] = {
+ [CHECKER_MSGID_NONE] = "",
+ [CHECKER_MSGID_DISABLED] = " is disabled",
+ [CHECKER_MSGID_NO_FD] = " has no usable fd",
+ [CHECKER_MSGID_INVALID] = " provided invalid message id",
+ [CHECKER_MSGID_UP] = " reports path is up",
+ [CHECKER_MSGID_DOWN] = " reports path is down",
+ [CHECKER_MSGID_GHOST] = " reports path is ghost",
+};
+
+const char *checker_message(const struct checker *c)
{
- if (!c)
- return NULL;
- return c->message;
+ int id;
+
+ if (!c || c->msgid < 0 ||
+ (c->msgid >= CHECKER_GENERIC_MSGTABLE_SIZE &&
+c->msgid < CHECKER_FIRST_MSGID))
+ goto bad_id;
+
+ if (c->msgid < CHECKER_GENERIC_MSGTABLE_SIZE)
+ return generic_msg[c->msgid];
+
+ id = c->msgid - CHECKER_FIRST_MSGID;
+ if (id < c->cls->msgtable_size)
+ return c->cls->msgtable[id];
+
+bad_id:
+ return generic_msg[CHECKER_MSGID_NONE];
}
void checker_clear_message (struct checker *c)
{
if (!c)
return;
- c->message[0] = '\0';
+ c->msgid = CHECKER_MSGID_NONE;
}
void checker_get (char *multipath_dir, struct checker * dst, char * name)
@@ -288,10 +327,12 @@ void checker_get (char *multipath_dir, struct checker *
dst, char * name)
dst->fd = src->fd;
dst->sync = src->sync;
strncpy(dst->name, src->name, CHECKER_NAME_LEN);
- strncpy(dst->message, src->message, CHECKER_MSG_LEN);
+ dst->msgid = CHECKER_MSGID_NONE;
dst->check = src->check;
dst->init = src->init;
dst->free = src->free;
+ dst->msgtable = src->msgtable;
+ dst->msgtable_size = src->msgtable_size;
dst->handle = NULL;
src->refcount++;
}
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index 7b18a1ac..8e26f1df 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -97,6 +97,22 @@ enum path_check_state {
#define CHECKER_DEV_LEN 256
#define LIB_CHECKER_NAMELEN 256
+/*
+ * Generic message IDs for use in
[dm-devel] [PATCH v5 08/21] libmultipath/checkers: rdac: use message id
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/checkers/rdac.c | 64 +---
1 file changed, 45 insertions(+), 19 deletions(-)
diff --git a/libmultipath/checkers/rdac.c b/libmultipath/checkers/rdac.c
index 5104e4e5..266f8e10 100644
--- a/libmultipath/checkers/rdac.c
+++ b/libmultipath/checkers/rdac.c
@@ -31,9 +31,7 @@
#define CURRENT_PAGE_CODE_VALUES 0
#define CHANGEABLE_PAGE_CODE_VALUES1
-#define MSG_RDAC_UP"rdac checker reports path is up"
-#define MSG_RDAC_DOWN "rdac checker reports path is down"
-#define MSG_RDAC_GHOST "rdac checker reports path is ghost"
+#define MSG_RDAC_DOWN " reports path is down"
#define MSG_RDAC_DOWN_TYPE(STR) MSG_RDAC_DOWN": "STR
#define RTPG_UNAVAILABLE 0x3
@@ -219,41 +217,69 @@ struct volume_access_inq
char dontcare1[34];
};
-const char
-*checker_msg_string(struct volume_access_inq *inq)
+enum {
+ RDAC_MSGID_NOT_CONN = CHECKER_FIRST_MSGID,
+ RDAC_MSGID_IN_STARTUP,
+ RDAC_MSGID_NON_RESPONSIVE,
+ RDAC_MSGID_IN_RESET,
+ RDAC_MSGID_FW_DOWNLOADING,
+ RDAC_MSGID_QUIESCED,
+ RDAC_MSGID_SERVICE_MODE,
+ RDAC_MSGID_UNAVAILABLE,
+ RDAC_MSGID_INQUIRY_FAILED,
+};
+
+#define _IDX(x) (RDAC_MSGID_##x - CHECKER_FIRST_MSGID)
+const char *libcheck_msgtable[] = {
+ [_IDX(NOT_CONN)] = MSG_RDAC_DOWN_TYPE("lun not connected"),
+ [_IDX(IN_STARTUP)] = MSG_RDAC_DOWN_TYPE("ctlr is in startup sequence"),
+ [_IDX(NON_RESPONSIVE)] =
+ MSG_RDAC_DOWN_TYPE("non-responsive to queries"),
+ [_IDX(IN_RESET)] = MSG_RDAC_DOWN_TYPE("ctlr held in reset"),
+ [_IDX(FW_DOWNLOADING)] =
+ MSG_RDAC_DOWN_TYPE("ctlr firmware downloading"),
+ [_IDX(QUIESCED)] = MSG_RDAC_DOWN_TYPE("ctlr quiesced by admin request"),
+ [_IDX(SERVICE_MODE)] = MSG_RDAC_DOWN_TYPE("ctlr is in service mode"),
+ [_IDX(UNAVAILABLE)] = MSG_RDAC_DOWN_TYPE("ctlr is unavailable"),
+ [_IDX(INQUIRY_FAILED)] = MSG_RDAC_DOWN_TYPE("inquiry failed"),
+ NULL,
+};
+
+static int
+checker_msg_string(const struct volume_access_inq *inq)
{
/* lun not connected */
if (((inq->PQ_PDT & 0xE0) == 0x20) || (inq->PQ_PDT & 0x7f))
- return MSG_RDAC_DOWN_TYPE("lun not connected");
+ return RDAC_MSGID_NOT_CONN;
/* if no tpg data is available, give the generic path down message */
if (!(inq->avtcvp & 0x10))
- return MSG_RDAC_DOWN;
+ return CHECKER_MSGID_DOWN;
/* controller is booting up */
if (((inq->aas_cur & 0x0F) == RTPG_TRANSITIONING) &&
(inq->aas_alt & 0x0F) != RTPG_TRANSITIONING)
- return MSG_RDAC_DOWN_TYPE("ctlr is in startup sequence");
+ return RDAC_MSGID_IN_STARTUP;
/* if not unavailable, give generic message */
if ((inq->aas_cur & 0x0F) != RTPG_UNAVAILABLE)
- return MSG_RDAC_DOWN;
+ return CHECKER_MSGID_DOWN;
/* target port group unavailable */
switch (inq->vendor_specific_cur) {
case RTPG_UNAVAIL_NON_RESPONSIVE:
- return MSG_RDAC_DOWN_TYPE("non-responsive to queries");
+ return RDAC_MSGID_NON_RESPONSIVE;
case RTPG_UNAVAIL_IN_RESET:
- return MSG_RDAC_DOWN_TYPE("ctlr held in reset");
+ return RDAC_MSGID_IN_RESET;
case RTPG_UNAVAIL_CFW_DL1:
case RTPG_UNAVAIL_CFW_DL2:
- return MSG_RDAC_DOWN_TYPE("ctlr firmware downloading");
+ return RDAC_MSGID_FW_DOWNLOADING;
case RTPG_UNAVAIL_QUIESCED:
- return MSG_RDAC_DOWN_TYPE("ctlr quiesced by admin request");
+ return RDAC_MSGID_QUIESCED;
case RTPG_UNAVAIL_SERVICE_MODE:
- return MSG_RDAC_DOWN_TYPE("ctlr is in service mode");
+ return RDAC_MSGID_SERVICE_MODE;
default:
- return MSG_RDAC_DOWN_TYPE("ctlr is unavailable");
+ return RDAC_MSGID_UNAVAILABLE;
}
}
@@ -307,14 +333,14 @@ int libcheck_check(struct checker * c)
done:
switch (ret) {
case PATH_DOWN:
- MSG(c, "%s", (inqfail) ? MSG_RDAC_DOWN_TYPE("inquiry failed") :
- checker_msg_string());
+ c->msgid = (inqfail ? RDAC_MSGID_INQUIRY_FAILED :
+ checker_msg_string());
break;
case PATH_UP:
- MSG(c, MSG_RDAC_UP);
+ c->msgid = CHECKER_MSGID_UP;
break;
case PATH_GHOST:
- MSG(c, MSG_RDAC_GHOST);
+ c->msgid = CHECKER_MSGID_GHOST;
break;
}
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 05/21] libmultipath/checkers: directio: use message id
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/checkers/directio.c | 27 ++-
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
index a80848d4..c4a0712e 100644
--- a/libmultipath/checkers/directio.c
+++ b/libmultipath/checkers/directio.c
@@ -18,10 +18,19 @@
#include "checkers.h"
#include "../libmultipath/debug.h"
-#define MSG_DIRECTIO_UNKNOWN "directio checker is not available"
-#define MSG_DIRECTIO_UP"directio checker reports path is up"
-#define MSG_DIRECTIO_DOWN "directio checker reports path is down"
-#define MSG_DIRECTIO_PENDING "directio checker is waiting on aio"
+enum {
+ MSG_DIRECTIO_UNKNOWN = CHECKER_FIRST_MSGID,
+ MSG_DIRECTIO_PENDING,
+ MSG_DIRECTIO_BLOCKSIZE,
+};
+
+#define _IDX(x) (MSG_DIRECTIO_##x - CHECKER_FIRST_MSGID)
+const char *libcheck_msgtable[] = {
+ [_IDX(UNKNOWN)] = " is not available",
+ [_IDX(PENDING)] = " is waiting on aio",
+ [_IDX(BLOCKSIZE)] = " cannot get blocksize, set default",
+ NULL,
+};
#define LOG(prio, fmt, args...) condlog(prio, "directio: " fmt, ##args)
@@ -54,7 +63,7 @@ int libcheck_init (struct checker * c)
}
if (ioctl(c->fd, BLKBSZGET, >blksize) < 0) {
- MSG(c, "cannot get blocksize, set default");
+ c->msgid = MSG_DIRECTIO_BLOCKSIZE;
ct->blksize = 512;
}
if (ct->blksize > 4096) {
@@ -198,16 +207,16 @@ int libcheck_check (struct checker * c)
switch (ret)
{
case PATH_UNCHECKED:
- MSG(c, MSG_DIRECTIO_UNKNOWN);
+ c->msgid = MSG_DIRECTIO_UNKNOWN;
break;
case PATH_DOWN:
- MSG(c, MSG_DIRECTIO_DOWN);
+ c->msgid = CHECKER_MSGID_DOWN;
break;
case PATH_UP:
- MSG(c, MSG_DIRECTIO_UP);
+ c->msgid = CHECKER_MSGID_UP;
break;
case PATH_PENDING:
- MSG(c, MSG_DIRECTIO_PENDING);
+ c->msgid = MSG_DIRECTIO_PENDING;
break;
default:
break;
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 17/21] libmultipath: pathinfo: don't blank wwid if checker fails
Blanking a WWID is a dangerous operation. E.g. configure() would
consider the path in question as invalid and orphan it if the
WWID is blank. Don't do this checker failures which may be transient
or indicate a badly configured or otherwise malfunctioning checker.
Moreover, we try to determine WWID even if path_offline returns
PATH_DOWN in the first place, so why should we not if the checker
has a problem?
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/discovery.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 5e59e273..a6159e4a 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1944,9 +1944,6 @@ int pathinfo(struct path *pp, struct config *conf, int
mask)
if (path_state == PATH_UP) {
pp->chkrstate = pp->state = get_state(pp, conf, 0,
path_state);
- if (pp->state == PATH_UNCHECKED ||
- pp->state == PATH_WILD)
- goto blank;
if (pp->state == PATH_TIMEOUT)
pp->state = PATH_DOWN;
if (pp->state == PATH_UP && !pp->size) {
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v5 07/21] libmultipath/checkers: hp_sw: use message id
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/checkers/hp_sw.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/libmultipath/checkers/hp_sw.c b/libmultipath/checkers/hp_sw.c
index 0ad34a6b..d7f1018c 100644
--- a/libmultipath/checkers/hp_sw.c
+++ b/libmultipath/checkers/hp_sw.c
@@ -27,10 +27,6 @@
#define MX_ALLOC_LEN 255
#define HEAVY_CHECK_COUNT 10
-#define MSG_HP_SW_UP "hp_sw checker reports path is up"
-#define MSG_HP_SW_DOWN "hp_sw checker reports path is down"
-#define MSG_HP_SW_GHOST"hp_sw checker reports path is ghost"
-
struct sw_checker_context {
void * dummy;
};
@@ -128,14 +124,14 @@ int libcheck_check(struct checker * c)
char buff[MX_ALLOC_LEN];
if (0 != do_inq(c->fd, 0, 1, 0x80, buff, MX_ALLOC_LEN, 0, c->timeout)) {
- MSG(c, MSG_HP_SW_DOWN);
+ c->msgid = CHECKER_MSGID_DOWN;
return PATH_DOWN;
- }
+ };
if (do_tur(c->fd, c->timeout)) {
- MSG(c, MSG_HP_SW_GHOST);
+ c->msgid = CHECKER_MSGID_GHOST;
return PATH_GHOST;
}
- MSG(c, MSG_HP_SW_UP);
+ c->msgid = CHECKER_MSGID_UP;
return PATH_UP;
}
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 1/7] libmultipath: set pp->checkint in store_pathinfo()
store_pathinfo is called with valid conf pointer anyway, so
checkint is available. pp->checkint is now valid for every
path after path_discovery().
This fixes a bad conf access in cli_add_path().
Reviewed-by: Benjamin Marzinski
Signed-off-by: Martin Wilck
---
libmultipath/discovery.c | 1 +
multipathd/cli_handlers.c | 1 -
multipathd/main.c | 2 --
3 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index a6159e4a..63558ad8 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -103,6 +103,7 @@ store_pathinfo (vector pathvec, struct config *conf,
err = store_path(pathvec, pp);
if (err)
goto out;
+ pp->checkint = conf->checkint;
out:
if (err)
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 75000807..4aea4ce7 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -736,7 +736,6 @@ cli_add_path (void * v, char ** reply, int * len, void *
data)
condlog(0, "%s: failed to store path info", param);
return 1;
}
- pp->checkint = conf->checkint;
}
return ev_add_path(pp, vecs, 1);
blacklisted:
diff --git a/multipathd/main.c b/multipathd/main.c
index c57aa392..958545a4 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2324,8 +2324,6 @@ configure (struct vectors * vecs)
free_path(pp);
i--;
}
- else
- pp->checkint = conf->checkint;
pthread_cleanup_pop(1);
}
if (map_discovery(vecs)) {
--
2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipathd: display the host WWNN fields When the iSCSI path information is displayed using the multipath command, the host WWNN in the path information is not fully displayed,
Hello Sunao,
On Sat, 2018-10-27 at 12:40 +0800, s90006763 wrote:
> This patch solves this problem and gets the complete display of host
> WWNN related fields,as follows:multipathd show paths format "%N %n"
> Host WWNN Target WWPN
> Iqn.xx-x.com.redhat:86329 iqn.xxx-xx.com.xx:oceanstor:::xx:x.x
> Iqn.xx-x.com.redhat:86329 iqn.xxx-xx.com.xx:oceanstor:::xx:x.x
> ---
> libmultipath/print.c | 46 +-
>
> 1 file changed, 41 insertions(+), 5 deletions(-)
Thanks again for the new submission.
Your patch still has lots of whitespace and other style issues. Please
use checkpatch.pl as I suggested in my previous review. Also, as I
already mentioned, please keep your subject line (summary) at a
readable length (75 chars) and add the other information in the
message body.
Apart from that, I only have one more minor nitpick, see below.
We are getting close.
Best regards,
Martin
>
> diff --git a/libmultipath/print.c b/libmultipath/print.c
> index 7b610b94..956d705d 100644
> --- a/libmultipath/print.c
> +++ b/libmultipath/print.c
> @@ -525,15 +525,13 @@ snprint_path_mpp (char * buff, size_t len,
> const struct path * pp)
> }
>
> static int
> -snprint_host_attr (char * buff, size_t len, const struct path * pp,
> char *attr)
> +snprint_fc_host_attr (char * buff, size_t len, const struct path *
> pp, char *attr)
> {
> struct udev_device *host_dev = NULL;
> char host_id[32];
> const char *value = NULL;
> int ret;
>
> - if (pp->sg_id.proto_id != SCSI_PROTOCOL_FCP)
> - return snprintf(buff, len, "[undef]");
> sprintf(host_id, "host%d", pp->sg_id.host_no);
> host_dev = udev_device_new_from_subsystem_sysname(udev,
> "fc_host",
> host_id);
> @@ -551,16 +549,54 @@ out:
> return ret;
> }
>
> +static int
> +snprint_iscsi_host_attr (char * buff, size_t len, const struct path
> * pp, char *attr)
> +{
> +struct udev_device *host_dev = NULL;
> +char host_id[32];
> +const char *value = NULL;
> +int ret;
> +
> +sprintf(host_id, "session%d", pp->sg_id.transport_id);
> +host_dev = udev_device_new_from_subsystem_sysname(udev,
> "iscsi_session",
> + host_id);
> +if (!host_dev) {
> +condlog(1, "%s: No iscsi_host device for '%s'", pp-
> >dev, host_id);
> +goto out;
> +}
> +value = udev_device_get_sysattr_value(host_dev, attr);
> +if (value)
> +ret = snprint_str(buff, len, value);
> +udev_device_unref(host_dev);
> +out:
> + if (!value)
> + ret = snprintf(buff, len, "[unknown]");
Please use "undef" here, for consistency.
> + return ret;
> +}
> +
> int
> snprint_host_wwnn (char * buff, size_t len, const struct path * pp)
> {
> - return snprint_host_attr(buff, len, pp, "node_name");
> + if(pp->sg_id.proto_id == SCSI_PROTOCOL_ISCSI)
> + {
> + return snprint_iscsi_host_attr(buff, len, pp,
> "initiatorname");
> + }
> + else if (pp->sg_id.proto_id == SCSI_PROTOCOL_FCP)
> + {
> + return snprint_fc_host_attr(buff, len, pp,
> "node_name");
> + }
> + return snprintf(buff, len, "[undef]");
> }
>
> int
> snprint_host_wwpn (char * buff, size_t len, const struct path * pp)
> {
> - return snprint_host_attr(buff, len, pp, "port_name");
> +
> + if (pp->sg_id.proto_id == SCSI_PROTOCOL_FCP)
> + {
> + return snprint_fc_host_attr(buff, len, pp,
> "port_name");
> + }
> + return snprintf(buff, len, "[undef]");
> }
>
> int
--
Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 4/7] multipathd: open client socket early
On Fri, Nov 02, 2018 at 01:23:59PM +0100, Martin Wilck wrote:
> Open the unix socket in multipathd code and pass the fd to
> uxsock_listen(). This will enable us to make the main thread
> wait for successful socket initialization in a follow-up patch.
>
Reviewed-by: Benjamin Marzinski
> Signed-off-by: Martin Wilck
> ---
> multipathd/main.c | 27 ++-
> multipathd/uxlsnr.c | 14 ++
> multipathd/uxlsnr.h | 5 +++--
> 3 files changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index a022bdb5..a91a14c6 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -66,6 +66,7 @@ static int use_watchdog;
> #include "pgpolicies.h"
> #include "uevent.h"
> #include "log.h"
> +#include "uxsock.h"
>
> #include "mpath_cmd.h"
> #include "mpath_persist.h"
> @@ -1496,12 +1497,24 @@ uevqloop (void * ap)
> static void *
> uxlsnrloop (void * ap)
> {
> + long ux_sock;
> +
> + pthread_cleanup_push(rcu_unregister, NULL);
> + rcu_register_thread();
> +
> + ux_sock = ux_socket_listen(DEFAULT_SOCKET);
> + if (ux_sock == -1) {
> + condlog(1, "could not create uxsock: %d", errno);
> + exit_daemon();
> + goto out;
> + }
> + pthread_cleanup_push(uxsock_cleanup, (void *)ux_sock);
> +
> if (cli_init()) {
> condlog(1, "Failed to init uxsock listener");
> - return NULL;
> + exit_daemon();
> + goto out_sock;
> }
> - pthread_cleanup_push(rcu_unregister, NULL);
> - rcu_register_thread();
> set_handler_callback(LIST+PATHS, cli_list_paths);
> set_handler_callback(LIST+PATHS+FMT, cli_list_paths_fmt);
> set_handler_callback(LIST+PATHS+RAW+FMT, cli_list_paths_raw);
> @@ -1556,8 +1569,12 @@ uxlsnrloop (void * ap)
> set_handler_callback(UNSETPRKEY+MAP, cli_unsetprkey);
>
> umask(077);
> - uxsock_listen(_trigger, ap);
> - pthread_cleanup_pop(1);
> + uxsock_listen(_trigger, ux_sock, ap);
> +
> +out_sock:
> + pthread_cleanup_pop(1); /* uxsock_cleanup */
> +out:
> + pthread_cleanup_pop(1); /* rcu_unregister */
> return NULL;
> }
>
> diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> index 6f63..773bc878 100644
> --- a/multipathd/uxlsnr.c
> +++ b/multipathd/uxlsnr.c
> @@ -165,24 +165,15 @@ void uxsock_cleanup(void *arg)
> /*
> * entry point
> */
> -void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, void * trigger_data)
> +void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
> + void * trigger_data)
> {
> - long ux_sock;
> int rlen;
> char *inbuf;
> char *reply;
> sigset_t mask;
> int old_clients = MIN_POLLS;
>
> - ux_sock = ux_socket_listen(DEFAULT_SOCKET);
> -
> - if (ux_sock == -1) {
> - condlog(1, "could not create uxsock: %d", errno);
> - exit_daemon();
> - }
> -
> - pthread_cleanup_push(uxsock_cleanup, (void *)ux_sock);
> -
> condlog(3, "uxsock: startup listener");
> polls = (struct pollfd *)MALLOC((MIN_POLLS + 1) * sizeof(struct
> pollfd));
> if (!polls) {
> @@ -322,6 +313,5 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger,
> void * trigger_data)
> }
> }
>
> - pthread_cleanup_pop(1);
> return NULL;
> }
> diff --git a/multipathd/uxlsnr.h b/multipathd/uxlsnr.h
> index d51a8f99..18f008d7 100644
> --- a/multipathd/uxlsnr.h
> +++ b/multipathd/uxlsnr.h
> @@ -5,7 +5,8 @@
>
> typedef int (uxsock_trigger_fn)(char *, char **, int *, bool, void *);
>
> -void * uxsock_listen(uxsock_trigger_fn uxsock_trigger,
> - void * trigger_data);
> +void uxsock_cleanup(void *arg);
> +void *uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
> + void * trigger_data);
>
> #endif
> --
> 2.19.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v5 03/21] libmultipath/checkers: replace message by msgid
On Fri, Nov 02, 2018 at 01:21:07PM +0100, Martin Wilck wrote:
> Replace the character array "message" in struct checker with
> a "message ID" field.
>
> The generic checker code defines a couple of standard message IDs
> and corresponding messages. Checker-specific message IDs start
> at CHECKER_FIRST_MSG. Checkers that implement specific message
> IDs must provide a table for converting the IDs into actual log
> messages.
>
> This simplifies the checker data structure and the handling of
> checker messages in the critical checker code path. It comes at
> the cost that only constant message strings are supported. It
> turns out that only a single checker log message (in the emc_clariion
> checker) was dynamically generated, and the missing information can
> be provided with a standard condlog message.
>
> Follow-up patches implement this for the existing checkers.
>
Reviewed-by: Benjamin Marzinski
> Signed-off-by: Martin Wilck
> ---
> libmultipath/checkers.c | 61 +---
> libmultipath/checkers.h | 43
> libmultipath/discovery.c | 4 +--
> 3 files changed, 90 insertions(+), 18 deletions(-)
>
> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> index 0bacc864..a1b5cb45 100644
> --- a/libmultipath/checkers.c
> +++ b/libmultipath/checkers.c
> @@ -141,6 +141,22 @@ struct checker * add_checker (char *multipath_dir, char
> * name)
> if (!c->free)
> goto out;
>
> + c->msgtable_size = 0;
> + c->msgtable = dlsym(c->handle, "libcheck_msgtable");
> +
> + if (c->msgtable != NULL) {
> + const char **p;
> +
> + for (p = c->msgtable;
> + *p && (p - c->msgtable) < CHECKER_MSGTABLE_SIZE; p++)
> + /* nothing */;
> +
> + c->msgtable_size = p - c->msgtable;
> + } else
> + c->msgtable_size = 0;
> + condlog(3, "checker %s: message table size = %d",
> + c->name, c->msgtable_size);
> +
> done:
> c->fd = -1;
> c->sync = 1;
> @@ -222,16 +238,16 @@ int checker_check (struct checker * c, int path_state)
> if (!c)
> return PATH_WILD;
>
> - c->message[0] = '\0';
> + c->msgid = CHECKER_MSGID_NONE;
> if (c->disable) {
> - MSG(c, "checker disabled");
> + c->msgid = CHECKER_MSGID_DISABLED;
> return PATH_UNCHECKED;
> }
> if (!strncmp(c->name, NONE, 4))
> return path_state;
>
> if (c->fd < 0) {
> - MSG(c, "no usable fd");
> + c->msgid = CHECKER_MSGID_NO_FD;
> return PATH_WILD;
> }
> r = c->check(c);
> @@ -248,25 +264,48 @@ int checker_selected (struct checker * c)
> return (c->check) ? 1 : 0;
> }
>
> -char * checker_name (struct checker * c)
> +const char *checker_name(const struct checker *c)
> {
> if (!c)
> return NULL;
> return c->name;
> }
>
> -char * checker_message (struct checker * c)
> +static const char *generic_msg[CHECKER_GENERIC_MSGTABLE_SIZE] = {
> + [CHECKER_MSGID_NONE] = "",
> + [CHECKER_MSGID_DISABLED] = " is disabled",
> + [CHECKER_MSGID_NO_FD] = " has no usable fd",
> + [CHECKER_MSGID_INVALID] = " provided invalid message id",
> + [CHECKER_MSGID_UP] = " reports path is up",
> + [CHECKER_MSGID_DOWN] = " reports path is down",
> + [CHECKER_MSGID_GHOST] = " reports path is ghost",
> +};
> +
> +const char *checker_message(const struct checker *c)
> {
> - if (!c)
> - return NULL;
> - return c->message;
> + int id;
> +
> + if (!c || c->msgid < 0 ||
> + (c->msgid >= CHECKER_GENERIC_MSGTABLE_SIZE &&
> + c->msgid < CHECKER_FIRST_MSGID))
> + goto bad_id;
> +
> + if (c->msgid < CHECKER_GENERIC_MSGTABLE_SIZE)
> + return generic_msg[c->msgid];
> +
> + id = c->msgid - CHECKER_FIRST_MSGID;
> + if (id < c->cls->msgtable_size)
> + return c->cls->msgtable[id];
> +
> +bad_id:
> + return generic_msg[CHECKER_MSGID_NONE];
> }
>
> void checker_clear_message (struct checker *c)
> {
> if (!c)
> return;
> - c->message[0] = '\0';
> + c->msgid = CHECKER_MSGID_NONE;
> }
>
> void checker_get (char *multipath_dir, struct checker * dst, char * name)
> @@ -288,10 +327,12 @@ void checker_get (char *multipath_dir, struct checker *
> dst, char * name)
> dst->fd = src->fd;
> dst->sync = src->sync;
> strncpy(dst->name, src->name, CHECKER_NAME_LEN);
> - strncpy(dst->message, src->message, CHECKER_MSG_LEN);
> + dst->msgid = CHECKER_MSGID_NONE;
> dst->check = src->check;
> dst->init = src->init;
> dst->free = src->free;
> + dst->msgtable = src->msgtable;
> + dst->msgtable_size = src->msgtable_size;
> dst->handle = NULL;
> src->refcount++;
> }
> diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
>
Re: [dm-devel] dm crypt: use unsigned long long instead of sector_t to store iv_offset
On Fri, Nov 02 2018 at 12:31am -0400, AliOS system security wrote: > On 2018/11/2 4:06, Mike Snitzer wrote: > >On Thu, Nov 01 2018 at 4:53am -0400, > >AliOS system security wrote: > > > >>The iv_offset in the mapping table of crypt target is a 64bit number > >>when iv mode is plain64 or plain64be. It will be assigned to iv_offset of > >>struct crypt_config, cc_sector of struct convert_context and iv_sector of > >>struct dm_crypt_request. These structures members are defined as a sector_t. > >>But sector_t is 32bit when CONFIG_LBDAF is not set in 32bit kernel. In this > >>situation sector_t is not big enough to store the 64bit iv_offset. > >I really don't think this is needed. > > > >cc->iv_offset can only address a the address space used to access the > >device. Which is expressed in terms of sectors. Therefore if > >CONFIG_LBDAF is not set in 32bit kernel then there is no need to address > >beyond that which 'sector_t' addresses. > > > >Please show proof to the contrary if you still think this change is > >needed. > > > >Mike > > > Sorry I made a mistake. I read > Documentation/device-mapper/dm-crypt.txt again and found that the IV > offset is a sector count. So it make sense to store the iv_offset as > a sector_t. > > In addition, I want to describe what problem I met in the beginning. > I made a crypt.img with the crypt param > "aes-cbc-plain64 0x1234...5678 1311768465173141112 /dev/loop0 0" in > a 32bit kernel with CONFIG_LBDAF=y. > The iv_offset is set to a 64bit number and the iv mode is set to > plain64. Someday I recompiled my kernel but > CONFIG_LBDAF is not set this time. When I reload the crypt.img with > the same crypt param in new kernel, > I got ioctl(..., DM_TABLE_LOAD, ...) return 0 but the content of > /dev/dm-0 is incorrect. > > So, is this situation, set the iv mode to plain64 or plain64be in a > 32bit kernel with CONFIG_LBDAF is not set, a problem? Should the > crypt_ctr() return an error code when this happned? Or we just > support 64bit iv mode > all the time regardless of CONFIG_LBDAF? Please see Milan's reply from earlier today. You did find a real bug. Milan is going to iterate on your patch (likely switch to using uint64_t and update the patch header to capture all the useful context for why this is a real issue -- albeit one that has apparently been around since 2.6.20). I'll still attribute the change to you, but Milan's contribution will be an incremental improvement on your initial patch. Thanks for reporting and fixing this. Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 22/22] libmultipath: make checker_message thread safe
On Thu, 2018-11-01 at 14:53 -0500, Benjamin Marzinski wrote: > On Tue, Oct 30, 2018 at 10:06:53PM +0100, Martin Wilck wrote: > > Get rid of the static char buffer in checker_message() > > introduced in the previous "replace message by msgid" patch. > > What you have is fine, but why not something like this, which doesn't > need to do any allocating? The reason (apart from myself being blind) was that checker_message() prints some extra text around the message. But with only two callers, that's actually over-complicating matters. Thanks for pointint this out, I'll re-post. Martin -- Dr. Martin Wilck , Tel. +49 (0)911 74053 2107 SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
