On 12/03/2015 05:55 PM, Gabriel Krisman Bertazi wrote:
> 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>
Hi Gabriel,
Thank you for your comments! You make very valid points about the
implicit multipath dependency we seem to have in iprutils. As you said,
I did try to follow the existing code, but that did not remedy some
other problems (e.g. the user not having any multipath tools configured
in their system).
Since these changes might need to be discussed in more detail and there
are other high priority items we need to work on, I will defer this for now.
Thanks,
--
Heitor Ricardo Alves de Siqueira
IBM Linux Technology Center
------------------------------------------------------------------------------
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
