On 12/03/2015 01: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.
I do think this is still goodness overall. It gets rid of some hacky code and
replaces it with less hacky code :) If the multipath call fails, no real harm
will come, we'll just have stale multipath maps sitting around. Although I also
agree that looking for a cleaner way to solve this would be nice. I'd prefer to
avoid adding a dependency on libmultipath though if we can help it. If there is
some other way we could trigger userspace to flush unused maps without calling
multipath directly we should pursue that.
-Brian
--
Brian King
Power Linux I/O
IBM Linux Technology Center
------------------------------------------------------------------------------
_______________________________________________
Iprdd-devel mailing list
Iprdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iprdd-devel
