[PATCH] dracut: Verify multipath config_dir option

[PATCH] dracut: Verify multipath config_dir option

[Milan P. Gandhi]

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

Re: [PATCH] dracut: Verify multipath config_dir option

[Dracut GitHub Import Bot]

Patchset imported to github.
Pull request:
<https://github.com/dracutdevs/dracut/compare/master...dracut-mailing-devs:20200121120834.GA23866%40machine1>

Re: [PATCH] dracut: Verify multipath config_dir option

[Martin Wilck]

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

Re: [PATCH] dracut: Verify multipath config_dir option

[Milan P. Gandhi]

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

Re: [PATCH] dracut: Verify multipath config_dir option

[Martin Wilck]

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

Re: [PATCH] dracut: Verify multipath config_dir option

[Milan P. Gandhi]

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.