* [PATCH] dracut: Verify multipath config_dir option
@ 2020-01-21 12:08 Milan P. Gandhi
2020-01-21 13:02 ` Dracut GitHub Import Bot
2020-01-29 9:49 ` Martin Wilck
0 siblings, 2 replies; 6+ messages in thread
From: Milan P. Gandhi @ 2020-01-21 12:08 UTC (permalink / raw)
To: initramfs-u79uwXL29TY76Z2rM5mHXA; +Cc: harald-H+wXaHxf7aLQT0dZR+AlfA
The 90multipath/module-setup.sh file currently does not check the
dm-multipath config_dir option. This option in multipath.conf file is
used to specify a custom directory/path that contains the multipath
configuration files. It's default value is /etc/multipath/conf.d
Currently install function of module-setup.sh has hardcoded the above
path, but users could change it with config_dir option. So, adding a
command to get the directory specified with config_dir option and add
these configuration files to the initial ram disk image.
Signed-off-by: Milan P. Gandhi <mgandhi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
modules.d/90multipath/module-setup.sh | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/modules.d/90multipath/module-setup.sh b/modules.d/90multipath/module-setup.sh
index 1f6a55ec..f7c521c1 100755
--- a/modules.d/90multipath/module-setup.sh
+++ b/modules.d/90multipath/module-setup.sh
@@ -78,6 +78,9 @@ install() {
}
}
+ # Include multipath configuration files from path specified with config_dir
+ config_dir=`/usr/sbin/multipath -t|grep -i config_dir|awk '{print $2}'|sed -e 's/^"//' -e 's/"$//'`/*
+
inst_multiple -o \
dmsetup \
kpartx \
@@ -90,7 +93,7 @@ install() {
/etc/xdrdevices.conf \
/etc/multipath.conf \
/etc/multipath/* \
- /etc/multipath/conf.d/*
+ $config_dir
[[ $hostonly ]] && [[ $hostonly_mode = "strict" ]] && {
for_each_host_dev_and_slaves_all add_hostonly_mpath_conf
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] dracut: Verify multipath config_dir option
2020-01-21 12:08 [PATCH] dracut: Verify multipath config_dir option Milan P. Gandhi
@ 2020-01-21 13:02 ` Dracut GitHub Import Bot
2020-01-29 9:49 ` Martin Wilck
1 sibling, 0 replies; 6+ messages in thread
From: Dracut GitHub Import Bot @ 2020-01-21 13:02 UTC (permalink / raw)
To: initramfs-u79uwXL29TY76Z2rM5mHXA
Patchset imported to github.
Pull request:
<https://github.com/dracutdevs/dracut/compare/master...dracut-mailing-devs:20200121120834.GA23866%40machine1>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dracut: Verify multipath config_dir option
2020-01-21 12:08 [PATCH] dracut: Verify multipath config_dir option Milan P. Gandhi
2020-01-21 13:02 ` Dracut GitHub Import Bot
@ 2020-01-29 9:49 ` Martin Wilck
[not found] ` <2c0c0f100442edd43b10085fa47177b54cd6d0c9.camel-IBi9RG/b67k@public.gmane.org>
1 sibling, 1 reply; 6+ messages in thread
From: Martin Wilck @ 2020-01-29 9:49 UTC (permalink / raw)
To: Milan P. Gandhi, initramfs-u79uwXL29TY76Z2rM5mHXA
Cc: harald-H+wXaHxf7aLQT0dZR+AlfA
On Tue, 2020-01-21 at 17:38 +0530, Milan P. Gandhi wrote:
> The 90multipath/module-setup.sh file currently does not check the
> dm-multipath config_dir option. This option in multipath.conf file is
> used to specify a custom directory/path that contains the multipath
> configuration files. It's default value is /etc/multipath/conf.d
>
> Currently install function of module-setup.sh has hardcoded the above
> path, but users could change it with config_dir option. So, adding a
> command to get the directory specified with config_dir option and add
> these configuration files to the initial ram disk image.
>
> Signed-off-by: Milan P. Gandhi <mgandhi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> modules.d/90multipath/module-setup.sh | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/modules.d/90multipath/module-setup.sh
> b/modules.d/90multipath/module-setup.sh
> index 1f6a55ec..f7c521c1 100755
> --- a/modules.d/90multipath/module-setup.sh
> +++ b/modules.d/90multipath/module-setup.sh
> @@ -78,6 +78,9 @@ install() {
> }
> }
>
> + # Include multipath configuration files from path specified with
> config_dir
> + config_dir=`/usr/sbin/multipath -t|grep -i config_dir|awk
> '{print $2}'|sed -e 's/^"//' -e 's/"$//'`/*
No need to use 'grep -i', multipath configuration directives are case-
sensitive. At least one member of that grep|awk|sed pipe can be
dropped. Actually, all of them, for example like this:
while read _k _v; do
if [[ "$_k" = config_dir ]]; then
_v=${_v#\"}
config_dir=${_v%\"}
fi
done < <(multipath -t)
The path (/usr/sbin/multipath) is inconsistent with what we use in
multipathd.service (/sbin/multipath). I'm not against changing this to
/usr, but we should be consistent.
And: fall back to /etc/multipath/conf.d if the command fails for some
reason?
Martin
> +
> inst_multiple -o \
> dmsetup \
> kpartx \
> @@ -90,7 +93,7 @@ install() {
> /etc/xdrdevices.conf \
> /etc/multipath.conf \
> /etc/multipath/* \
> - /etc/multipath/conf.d/*
> + $config_dir
>
> [[ $hostonly ]] && [[ $hostonly_mode = "strict" ]] && {
> for_each_host_dev_and_slaves_all add_hostonly_mpath_conf
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dracut: Verify multipath config_dir option
[not found] ` <2c0c0f100442edd43b10085fa47177b54cd6d0c9.camel-IBi9RG/b67k@public.gmane.org>
@ 2020-01-31 7:58 ` Milan P. Gandhi
2020-01-31 8:35 ` Martin Wilck
0 siblings, 1 reply; 6+ messages in thread
From: Milan P. Gandhi @ 2020-01-31 7:58 UTC (permalink / raw)
To: Martin Wilck, initramfs-u79uwXL29TY76Z2rM5mHXA
Cc: harald-H+wXaHxf7aLQT0dZR+AlfA
On Wed, Jan 29, 2020 at 10:49:21AM +0100, Martin Wilck wrote:
> On Tue, 2020-01-21 at 17:38 +0530, Milan P. Gandhi wrote:
> > The 90multipath/module-setup.sh file currently does not check the
> > dm-multipath config_dir option. This option in multipath.conf file is
> > used to specify a custom directory/path that contains the multipath
> > configuration files. It's default value is /etc/multipath/conf.d
> >
> > Currently install function of module-setup.sh has hardcoded the above
> > path, but users could change it with config_dir option. So, adding a
> > command to get the directory specified with config_dir option and add
> > these configuration files to the initial ram disk image.
> >
> > Signed-off-by: Milan P. Gandhi <mgandhi-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> > modules.d/90multipath/module-setup.sh | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/modules.d/90multipath/module-setup.sh
> > b/modules.d/90multipath/module-setup.sh
> > index 1f6a55ec..f7c521c1 100755
> > --- a/modules.d/90multipath/module-setup.sh
> > +++ b/modules.d/90multipath/module-setup.sh
> > @@ -78,6 +78,9 @@ install() {
> > }
> > }
> >
> > + # Include multipath configuration files from path specified with
> > config_dir
> > + config_dir=`/usr/sbin/multipath -t|grep -i config_dir|awk
> > '{print $2}'|sed -e 's/^"//' -e 's/"$//'`/*
>
> No need to use 'grep -i', multipath configuration directives are case-
> sensitive. At least one member of that grep|awk|sed pipe can be
> dropped. Actually, all of them, for example like this:
>
> while read _k _v; do
> if [[ "$_k" = config_dir ]]; then
> _v=${_v#\"}
> config_dir=${_v%\"}
> fi
> done < <(multipath -t)
Thank you for your suggestion, Martin. I will update the patch, do some
more testing with it and send a V2 with changes.
> The path (/usr/sbin/multipath) is inconsistent with what we use in
> multipathd.service (/sbin/multipath). I'm not against changing this to
> /usr, but we should be consistent.
I used (/usr/sbin/multipath) because location of multipath binary is
shown in /usr/sbin. I will change it to /sbin/multipath to make it more
consistent with the systemctl output for multipathd.service
$ whereis multipath
multipath: /usr/sbin/multipath /usr/lib64/multipath [...]
> And: fall back to /etc/multipath/conf.d if the command fails for some
> reason?
The 'multipath -t' command runs successfully even when multipathd itself
is stopped. And config_dir option is one of the built-in parameters, so
if users do not modify it then it is automatically set to
/etc/multipath/conf.d
I can add a check to verify if the config_dir parameter read from
multipath -t is an actual directory, and if not, fall back to above
default path - /etc/multipath/conf.d
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dracut: Verify multipath config_dir option
2020-01-31 7:58 ` Milan P. Gandhi
@ 2020-01-31 8:35 ` Martin Wilck
[not found] ` <cc660dff208809a5511ab837f4d781d8faeae442.camel-IBi9RG/b67k@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Martin Wilck @ 2020-01-31 8:35 UTC (permalink / raw)
To: Milan P. Gandhi, initramfs-u79uwXL29TY76Z2rM5mHXA
Cc: harald-H+wXaHxf7aLQT0dZR+AlfA
Hello Milan,
thanks a lot for your response. I'm fine with your suggestions.
On Fri, 2020-01-31 at 13:28 +0530, Milan P. Gandhi wrote:
> On Wed, Jan 29, 2020 at 10:49:21AM +0100, Martin Wilck wrote:
> >
> > The path (/usr/sbin/multipath) is inconsistent with what we use in
> > multipathd.service (/sbin/multipath). I'm not against changing this
> > to
> > /usr, but we should be consistent.
>
> I used (/usr/sbin/multipath) because location of multipath binary is
> shown in /usr/sbin. I will change it to /sbin/multipath to make it
> more
> consistent with the systemctl output for multipathd.service
Well, Fedora made the /usr move, which openSUSE hasn't completed yet.
In general IMO using /usr/sbin is fine, distributions can
patch this as it fits. But for clarity, the move should be made in a
separate patch, so I for your patch, sticking with /sbin is better.
> $ whereis multipath
> multipath: /usr/sbin/multipath /usr/lib64/multipath [...]
>
> > And: fall back to /etc/multipath/conf.d if the command fails for
> > some
> > reason?
>
> The 'multipath -t' command runs successfully even when multipathd
> itself
> is stopped. And config_dir option is one of the built-in parameters,
> so
> if users do not modify it then it is automatically set to
> /etc/multipath/conf.d
Right, that makes sense. If "multipath -t" fails, something is really
fishy and we should probably error out.
> I can add a check to verify if the config_dir parameter read from
> multipath -t is an actual directory, and if not, fall back to above
> default path - /etc/multipath/conf.d
I'm not sure about that. If config_dir exists and is not a directory,
we should error out. If it doesn't exist, we should just skip it.
Btw, while you are at it: There's also the "multipath_dir" config
option that specifies the location of the "plugin" libraries for
multipath. This stuff is currently installed via
inst_libdir_file "libmultipath*" "multipath/"
But in theory at least, users can change this directory, and if dracut
supports a modified "config_dir", it might as well support a modified
"multipath_dir." You may argue that that's unlikely to be modified, but
the same can be said about config_dir. Just a suggestion.
Regards
Martin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dracut: Verify multipath config_dir option
[not found] ` <cc660dff208809a5511ab837f4d781d8faeae442.camel-IBi9RG/b67k@public.gmane.org>
@ 2020-02-03 11:49 ` Milan P. Gandhi
0 siblings, 0 replies; 6+ messages in thread
From: Milan P. Gandhi @ 2020-02-03 11:49 UTC (permalink / raw)
To: Martin Wilck, initramfs-u79uwXL29TY76Z2rM5mHXA
Cc: harald-H+wXaHxf7aLQT0dZR+AlfA
On 1/31/20 2:05 PM, Martin Wilck wrote:
> Hello Milan,
>
> thanks a lot for your response. I'm fine with your suggestions.
>
> On Fri, 2020-01-31 at 13:28 +0530, Milan P. Gandhi wrote:
>> On Wed, Jan 29, 2020 at 10:49:21AM +0100, Martin Wilck wrote:
>>>
>>> The path (/usr/sbin/multipath) is inconsistent with what we use in
>>> multipathd.service (/sbin/multipath). I'm not against changing this
>>> to
>>> /usr, but we should be consistent.
>>
>> I used (/usr/sbin/multipath) because location of multipath binary is
>> shown in /usr/sbin. I will change it to /sbin/multipath to make it
>> more
>> consistent with the systemctl output for multipathd.service
>
> Well, Fedora made the /usr move, which openSUSE hasn't completed yet.
> In general IMO using /usr/sbin is fine, distributions can
> patch this as it fits. But for clarity, the move should be made in a
> separate patch, so I for your patch, sticking with /sbin is better.
>
>> $ whereis multipath
>> multipath: /usr/sbin/multipath /usr/lib64/multipath [...]
>>
>>> And: fall back to /etc/multipath/conf.d if the command fails for
>>> some
>>> reason?
>>
>> The 'multipath -t' command runs successfully even when multipathd
>> itself
>> is stopped. And config_dir option is one of the built-in parameters,
>> so
>> if users do not modify it then it is automatically set to
>> /etc/multipath/conf.d
>
> Right, that makes sense. If "multipath -t" fails, something is really
> fishy and we should probably error out.
>
>> I can add a check to verify if the config_dir parameter read from
>> multipath -t is an actual directory, and if not, fall back to above
>> default path - /etc/multipath/conf.d
>
> I'm not sure about that. If config_dir exists and is not a directory,
> we should error out. If it doesn't exist, we should just skip it.
Thanks Martin!
I have just now sent an updated patch with the changes.
> Btw, while you are at it: There's also the "multipath_dir" config
> option that specifies the location of the "plugin" libraries for
> multipath. This stuff is currently installed via
>
> inst_libdir_file "libmultipath*" "multipath/"
>
> But in theory at least, users can change this directory, and if dracut
> supports a modified "config_dir", it might as well support a modified
> "multipath_dir." You may argue that that's unlikely to be modified, but
> the same can be said about config_dir. Just a suggestion.
>
Users can change "multipath_dir" config option as well, but currently
there is no check for this option. I will soon send a patch to verify
"multipath_dir" option as well.
Regards,
Milan.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-03 11:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-21 12:08 [PATCH] dracut: Verify multipath config_dir option Milan P. Gandhi
2020-01-21 13:02 ` Dracut GitHub Import Bot
2020-01-29 9:49 ` Martin Wilck
[not found] ` <2c0c0f100442edd43b10085fa47177b54cd6d0c9.camel-IBi9RG/b67k@public.gmane.org>
2020-01-31 7:58 ` Milan P. Gandhi
2020-01-31 8:35 ` Martin Wilck
[not found] ` <cc660dff208809a5511ab837f4d781d8faeae442.camel-IBi9RG/b67k@public.gmane.org>
2020-02-03 11:49 ` Milan P. Gandhi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.