[RFC][PATCH 1/5] support command string with space

[RFC][PATCH 1/5] support command string with space

[Takahiro Yasui]

Support escaped space by backslash in the command line string passed
through lvm2 command interface, lvm2_run(). This is used to handle
"--config" option which contains multiple parameters.

e.g. lvconvert --repair --use-policies --config \
     devices{ignore_suspended_devices=1\ filter=[...]} vg/lv


Signed-off-by: Takahiro Yasui <tyasui@redhat.com>
---
 tools/lvmcmdline.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Index: LVM2.02.54-20090928/tools/lvmcmdline.c
===================================================================
--- LVM2.02.54-20090928.orig/tools/lvmcmdline.c
+++ LVM2.02.54-20090928/tools/lvmcmdline.c
@@ -1070,8 +1070,15 @@ int lvm_split(char *str, int *argc, char
 			break;
 
 		e = b;
-		while (*e && !isspace(*e))
+		while (*e && !isspace(*e)) {
+			/*
+			 * 'backslash' is treated as a escape character
+			 * and the string isn't split at 'backslash + space.'
+			 */
+			if (*e == '\\' && isspace(*(e+1)))
+				*e++ = ' ';
 			e++;
+		}
 
 		argv[(*argc)++] = b;
 		if (!*e)

-- 
Takahiro Yasui
Hitachi Computer Products (America), Inc.

[RFC][PATCH 1/5] support command string with space

[Petr Rockai]

Takahiro Yasui <tyasui@redhat.com> writes:

> Support escaped space by backslash in the command line string passed
> through lvm2 command interface, lvm2_run(). This is used to handle
> "--config" option which contains multiple parameters.
>
> e.g. lvconvert --repair --use-policies --config \
>      devices{ignore_suspended_devices=1\ filter=[...]} vg/lv
>
>
> Signed-off-by: Takahiro Yasui <tyasui@redhat.com>
> ---
>  tools/lvmcmdline.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> Index: LVM2.02.54-20090928/tools/lvmcmdline.c
> ===================================================================
> --- LVM2.02.54-20090928.orig/tools/lvmcmdline.c
> +++ LVM2.02.54-20090928/tools/lvmcmdline.c
> @@ -1070,8 +1070,15 @@ int lvm_split(char *str, int *argc, char
>  			break;
>  
>  		e = b;
> -		while (*e && !isspace(*e))
> +		while (*e && !isspace(*e)) {
> +			/*
> +			 * 'backslash' is treated as a escape character
> +			 * and the string isn't split at 'backslash + space.'
> +			 */
> +			if (*e == '\\' && isspace(*(e+1)))
> +				*e++ = ' ';
>  			e++;
> +		}
>  
>  		argv[(*argc)++] = b;
>  		if (!*e)
IIUIC, the code will replace "\ " with "  " (double space) -- which may or may
not be correct, depending on where the space appears. It also breaks any code
that may have previously expected "\ " to have no special meaning, even though I
doubt any exists.

Anyway, a more correct solution would entail a lvm2_runv (or similarly named)
function, that would take char **argv -- no escaping issues to care
about. Since this just adds to the library, it should be backwards compatible
just fine.

Yours,
   Petr.

[RFC][PATCH 1/5] support command string with space

[Takahiro Yasui]

On 09/30/09 16:24, Petr Rockai wrote:
> Takahiro Yasui <tyasui@redhat.com> writes:
> 
>> Support escaped space by backslash in the command line string passed
>> through lvm2 command interface, lvm2_run(). This is used to handle
>> "--config" option which contains multiple parameters.
>>
>> e.g. lvconvert --repair --use-policies --config \
>>      devices{ignore_suspended_devices=1\ filter=[...]} vg/lv
>>
>>
>> Signed-off-by: Takahiro Yasui <tyasui@redhat.com>
>> ---
>>  tools/lvmcmdline.c |    9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> Index: LVM2.02.54-20090928/tools/lvmcmdline.c
>> ===================================================================
>> --- LVM2.02.54-20090928.orig/tools/lvmcmdline.c
>> +++ LVM2.02.54-20090928/tools/lvmcmdline.c
>> @@ -1070,8 +1070,15 @@ int lvm_split(char *str, int *argc, char
>>  			break;
>>  
>>  		e = b;
>> -		while (*e && !isspace(*e))
>> +		while (*e && !isspace(*e)) {
>> +			/*
>> +			 * 'backslash' is treated as a escape character
>> +			 * and the string isn't split at 'backslash + space.'
>> +			 */
>> +			if (*e == '\\' && isspace(*(e+1)))
>> +				*e++ = ' ';
>>  			e++;
>> +		}
>>  
>>  		argv[(*argc)++] = b;
>>  		if (!*e)
> IIUIC, the code will replace "\ " with "  " (double space) -- which may or may
> not be correct, depending on where the space appears. It also breaks any code
> that may have previously expected "\ " to have no special meaning, even though I
> doubt any exists.

Yes, this approach replace "\ " with "  ", and it will work correctly unless
"\ " is used for other purpose previously. Current lvm codes use lvm_split()
only in early stage to split options and there is no case you concerns.

> Anyway, a more correct solution would entail a lvm2_runv (or similarly named)
> function, that would take char **argv -- no escaping issues to care
> about. Since this just adds to the library, it should be backwards compatible
> just fine.

I can add an interface like lvm2_runv. But there is another approach using
single-quotes around an option with spaces as we do in a shell (e.g. bash).

Thanks,
Taka

[RFC][PATCH 1/5] support command string with space

[Petr Rockai]

Takahiro Yasui <tyasui@redhat.com> writes:

> On 09/30/09 16:24, Petr Rockai wrote:
>> IIUIC, the code will replace "\ " with "  " (double space) -- which may or may
>> not be correct, depending on where the space appears. It also breaks any code
>> that may have previously expected "\ " to have no special meaning, even though I
>> doubt any exists.
>
> Yes, this approach replace "\ " with "  ", and it will work correctly unless
> "\ " is used for other purpose previously. Current lvm codes use lvm_split()
> only in early stage to split options and there is no case you concerns.
There may not be *right now* -- I still think this is a bug waiting to
happen. Shell scripts are full of escaping bugs and in dmeventd we have a
simple way around (see below). One example for all -- I know it may be sick,
but you can have device node paths with spaces in them, and I expect this will
break -- either you forget to escape them, or the un-escaping produces wrong
paths. (And as surprising as it may sound, spaces in device names work with
current code -- I can create PVs and VGs on them and I can filter them out or
in.)

>> Anyway, a more correct solution would entail a lvm2_runv (or similarly named)
>> function, that would take char **argv -- no escaping issues to care
>> about. Since this just adds to the library, it should be backwards compatible
>> just fine.
>
> I can add an interface like lvm2_runv. But there is another approach using
> single-quotes around an option with spaces as we do in a shell (e.g. bash).
And how you quote single-quotes then? Doesn't work. Please use something
reliable and robust -- like passing argv, or using varargs function (like
execl). This eliminates all quoting bugs quite simply.

Yours,
   Petr.

[RFC][PATCH 1/5] support command string with space

[Takahiro Yasui]

On 10/01/09 01:54, Petr Rockai wrote:
> Takahiro Yasui <tyasui@redhat.com> writes:
>> On 09/30/09 16:24, Petr Rockai wrote:
>>> IIUIC, the code will replace "\ " with "  " (double space) -- which may or may
>>> not be correct, depending on where the space appears. It also breaks any code
>>> that may have previously expected "\ " to have no special meaning, even though I
>>> doubt any exists.
>>
>> Yes, this approach replace "\ " with "  ", and it will work correctly unless
>> "\ " is used for other purpose previously. Current lvm codes use lvm_split()
>> only in early stage to split options and there is no case you concerns.
> There may not be *right now* -- I still think this is a bug waiting to
> happen. Shell scripts are full of escaping bugs and in dmeventd we have a
> simple way around (see below). One example for all -- I know it may be sick,
> but you can have device node paths with spaces in them, and I expect this will
> break -- either you forget to escape them, or the un-escaping produces wrong
> paths. (And as surprising as it may sound, spaces in device names work with
> current code -- I can create PVs and VGs on them and I can filter them out or
> in.)
> 
>>> Anyway, a more correct solution would entail a lvm2_runv (or similarly named)
>>> function, that would take char **argv -- no escaping issues to care
>>> about. Since this just adds to the library, it should be backwards compatible
>>> just fine.
>>
>> I can add an interface like lvm2_runv. But there is another approach using
>> single-quotes around an option with spaces as we do in a shell (e.g. bash).
> And how you quote single-quotes then? Doesn't work. Please use something
> reliable and robust -- like passing argv, or using varargs function (like
> execl). This eliminates all quoting bugs quite simply.

I see. Your approach, lvm2_runv, seems quite safe, and I will implement it.
Thank you for the helpful comments.

Thanks,
Taka

[RFC][PATCH 1/5] support command string with space

[Alasdair G Kergon]

On Wed, Sep 30, 2009 at 10:24:36PM +0200, Peter Rockai wrote:
> Anyway, a more correct solution would entail a lvm2_runv (or similarly named)
> function, that would take char **argv -- no escaping issues to care
> about. Since this just adds to the library, it should be backwards compatible
> just fine.

Yes - try a lvm2_runv() first, that simply bypasses the call to lvm_split().

Alasdair