multipath-tools update Q2/2021 #13
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cc: Martin Wilck <[email protected]> Cc: Benjamin Marzinski <[email protected]> Cc: Christophe Varoqui <[email protected]> Cc: DM-DEVEL ML <[email protected]> Signed-off-by: Xose Vazquez Perez <[email protected]>
We've seen a crash of multipath in disassemble_map because of a params string exceeding PARAMS_SIZE. While the crash could have been fixed by a simple error check, I believe multipath should be able to work with arbitrary long parameter strings passed from the kernel. The parameter list of dm_get_map() has changed. Bumped ABI version and removed dm_get_map() and some functions from the ABI, which are only called from libmultipath itself. Reviewed-by: Benjamin Marzinski <[email protected]>
Add an API for string buffers that grow in size as text is added. This API will be useful in several places of the multipath-tools code base. Add unit tests for these helpers, too. Reviewed-by: Benjamin Marzinski <[email protected]>
Instead of using fixed PARAMS_SIZE-sized arrays for parameters, use dynamically allocated memory. The library version needs to be bumped, because setup_map() argument list has changed. Reviewed-by: Benjamin Marzinski <[email protected]>
Temporary solution for snprint_keyword(), as print.c hasn't been migrated yet. Reviewed-by: Benjamin Marzinski <[email protected]>
Use the strbuf API in print.c, wherever growing string buffers are appropriate. This requires a large amount of changes in print.c itself, and in other files that use functions from print.c. It makes no sense to separate this into smaller patches though, as the various snprint_xyz() functions depend on each other, and need to use similar prototypes. libmultipath version must be bumped, as all the printing functions have changed prototypes. Also, add the required strbuf functions to the libmultipath API. Change the libmultipath calls in prioritizers, foreign libs, and in multipathd according to the strbuf-related API changes. While working on print.c, I made a couple of other other minor changes: - all snprint_xyz() functions return a negative error in case of failure (most likely is -ENOMEM). In case of success, the number of printed characters is returned. - snprint_progress(): use fill_strbuf() rather than repeated iterations - _snprint_multipath(), _snprint_path(), snprint_multipath_header(), snprint_path_header(), _snprint_pathgroup(): use fill_strbuf() instead of the PAD/NOPAD logic. - _snprint_multipath_topology(): simplified the ASCII art generation for the topology tree somewhat. - snprint_json_xyz(): use fill_strbuf() rather than printing the indent repeatedly. - snprint_blacklist_group(), snprint_blacklist_devgroup(): combined two print statements into one. In cli_handlers.c, fixed the returned values for the "len" parameter in some functions (it's supposed to be the strlen of the reply + 1). Reviewed-by: Benjamin Marzinski <[email protected]>
A failure in find_keyword() means an internal error. Fail hard rather than returning an empty string. Reviewed-by: Benjamin Marzinski <[email protected]>
Move all macros to print.c that aren't used in other source files. Reviewed-by: Benjamin Marzinski <[email protected]>
We can avoid some buffer length checks here, too. Also, simplify the implementation of format_devname(). Created a wrapper for the format_devname() test case, to avoid chaning the test cases themselves. Reviewed-by: Benjamin Marzinski <[email protected]>
Here, too, strbuf can be used to simplify code. Reviewed-by: Benjamin Marzinski <[email protected]>
Since systemd commit e39b4d2 ("libudev: re-implement udev-monitor by
sd_device_monitor"), udev_monitor_set_receive_buffer_size() returns 1
on success. Fix the test for the return value to avoid a misleading
"failed to increase buffer size" warning.
Reviewed-by: Benjamin Marzinski <[email protected]>
Reviewed-by: Martin Wilck <[email protected]> Signed-off-by: Benjamin Marzinski <[email protected]>
"mpathpersist -oCK <reservation_key> <device>" will return success if it is run on devices with no usable paths, but nothing is actually done. The -L command will fail, but it should give up sooner, and with a more helpful error message. Reviewed-by: Martin Wilck <[email protected]> Signed-off-by: Benjamin Marzinski <[email protected]>
If multipath notices that multipath devices exist or were created, and multipathd is not running, it now prints a warning message, so users are notified of the issue. Reviewed-by: Martin Wilck <[email protected]> Signed-off-by: Benjamin Marzinski <[email protected]>
The code at the end of coalesce_paths() removes a multipath device that was just created/reloaded, if none of its path devices have pp->dev set. This code is very old, and no longer has any actual effect. Multipath devices no longer get placed in pathvec without having pp->dev set. Even if they could, there's no point in creating/reloading the device and then immediately removing it. Reviewed-by: Martin Wilck <[email protected]> Signed-off-by: Benjamin Marzinski <[email protected]>
Starting in glibc-2.34 (commit 5d98a7da), PTHREAD_STACK_MIN is defined as sysconf(_SC_THREAD_STACK_MIN) if _GNU_SOURCE is defined. sysconf() returns a long and can, at least in theory, return -1. This change causes compilation to fail in setup_thread_attr() due to a comparision with different signedness, since stacksize is a size_t. Reviewed-by: Martin Wilck <[email protected]> Signed-off-by: Benjamin Marzinski <[email protected]>
When remove_map() is called, if the multipath device is in a mpvec, it must be removed from it, because the device will be freed. Now that the mpvec is passed as a separate parameter to remove_map(), the purge_vec parameter is redundant. It was only used by coalesce_paths(), since the multipath device isn't on any vector when remove_map() is called there. Instead, remove_map() can just be called with a NULL mpvec, when there is no mpvec to remove the device from. remove_map_by_alias() also has a redundant purge_vec parameter. Since it only removes a map if it finds in on vec->mpvec, calling it with KEEP_VEC would be a bug, since it would leave a pointer to the freed device in the vector. Reviewed-by: Martin Wilck <[email protected]> Signed-off-by: Benjamin Marzinski <[email protected]>
The patch "libmultipath: drop unnecessary parameter from remove_map()" changed the parameter list of remove_map(). Reviewed-by: Benjamin Marzinski <[email protected]>
The dmevents test needs to be adapted to the ABI change for remove_map_by_alias(). Reviewed-by: Benjamin Marzinski <[email protected]>
Add tests/*.vgr (output files from vagrant tests runs), and a few other missing patterns. Reviewed-by: Benjamin Marzinski <[email protected]>
|
Added Ben's reviewed-by: tags. This should be ready to merge now. |
|
resolves #10. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello Christophe,
Here is another PR for multipath-tools. I'd suggest a version bump too, as the diffstat is quite big.
The last 3 commits
haven't been reviewed by @bmarzins yet, theyare fixups for Ben's latest series.@bmarzins:
@xosevp:
@mwilck:
strbufAPI in libmultipath (this represents the bulk of the changes in the PR).diffstat