Heitor Ricardo Alves de Siqueira <hal...@linux.vnet.ibm.com> writes:
> Unused multipath device map flushing in ipr_format_unit() is now done
> with a fork()/exec() combination.
>
> Since commit 0b693bd29cbe (iprutils: Unbind device before JBOD -> AF
> formatting), multipath and /dev/sd device removal is done by the
> binding and unbinding operations prior to formatting. We can now remove
> the less secure system() calls that were in ipr_format_unit().
>
> Signed-off-by: Heitor Ricardo Alves de Siqueira <hal...@linux.vnet.ibm.com>
> ---
> iprlib.c | 23 +++++++++--------------
> iprlib.h | 1 +
> 2 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/iprlib.c b/iprlib.c
> index c5dfd8bb50c0..85e14d918bc6 100644
> --- a/iprlib.c
> +++ b/iprlib.c
> @@ -3738,23 +3738,18 @@ int ipr_format_unit(struct ipr_dev *dev)
> u8 *defect_list_hdr;
> int length = IPR_DEFECT_LIST_HDR_LEN;
> char *name = dev->gen_name;
> - char cmnd[1000];
> - struct ipr_dev *multipath_dev;
>
> if (strlen(dev->dev_name) && dev->scsi_dev_data->device_id) {
> - sprintf(cmnd, "/sbin/multipathd -k\"del path %s\" > /dev/null
> 2>&1", strrchr(dev->dev_name, '/') + 1);
> - system(cmnd);
> - sprintf(cmnd, "/bin/rm -rf %s %s%s", dev->dev_name,
> dev->dev_name, "[0-9]*");
> - system(cmnd);
> -
> - multipath_dev = find_multipath_jbod(dev);
> - if (multipath_dev) {
> - sprintf(cmnd, "/sbin/multipathd -k\"del path %s\" >
> /dev/null 2>&1", strrchr(multipath_dev->dev_name, '/') + 1);
> - system(cmnd);
> - sprintf(cmnd, "/bin/rm -rf %s %s%s",
> multipath_dev->dev_name, multipath_dev->dev_name , "[0-9]*");
> - system(cmnd);
> + int pid, status;
> +
> + /* flush unused multipath device maps */
> + pid = fork();
> + if (pid == 0) {
> + execl("/sbin/multipath", "-F", NULL);
> + _exit(errno);
> + } else {
> + waitpid(pid, &status, 0);
I don't think replacing system with exec* is a much better solution
here. It's OK for your log system fix because we really need to call an
editor, but I think there are better ways to handle it here.
I understand you just followed the original code, and to assume
multipath is in /sbin probably works for some scenarios, but it's
definitely not good and will fail for many others. We should at least
honor user's configured $PATH when invoking multipath.
Much more surprising, looks like we have a *very* implicit dependency
for multipath-tools, which is bad, because none of the distro packages I
know considers that dependency. Not even the one we provide in the spec
directory. This is broken and will fail silently, even if harmlessly ,
in any machine lacking multipath-tools.
If we *really* need to deal with multipath in iprutils, I think we
should just bite the bullet and accept that iprconfig depends on
libmultipath, fix the build system and do a proper linking against it.
This way, we can use libmultipath functions to remove paths, instead of
abusing system/exec calls...
> }
> - system("/sbin/multipath -F > /dev/null 2>&1");
> }
>
> if (strlen(name) == 0)
> diff --git a/iprlib.h b/iprlib.h
> index 16fe1e162650..a4fea452a3f9 100644
> --- a/iprlib.h
> +++ b/iprlib.h
> @@ -38,6 +38,7 @@
> #include <sys/stat.h>
> #include <sys/types.h>
> #include <sys/socket.h>
> +#include <sys/wait.h>
> #include <ctype.h>
> #include <syslog.h>
> #include <term.h>
--
Gabriel Krisman Bertazi
------------------------------------------------------------------------------
Go from Idea to Many App Stores Faster with Intel(R) XDK
Give your users amazing mobile app experiences with Intel(R) XDK.
Use one codebase in this all-in-one HTML5 development environment.
Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs.
http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
_______________________________________________
Iprdd-devel mailing list
Iprdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iprdd-devel
