[PATCH] Add escape sequence for ':' in command's PV list

[PATCH] Add escape sequence for ':' in command's PV list

[Peter Rajnoha]

Hi,

this little patch provides an escape sequence in command's PV list
where ':' is also used as a delimiter for extent ranges. To escape
this char, we just double it, so '::' is translated to ':' and ':'
is extent range delimiter.
(BZ #491409)

Peter


diff --git a/tools/toollib.c b/tools/toollib.c
index aa33468..b5a43b5 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -1095,8 +1095,9 @@ struct dm_list *create_pv_list(struct dm_pool *mem, struct volume_group *vg, int
 	struct dm_list *r;
 	struct pv_list *pvl;
 	struct dm_list tags, arg_pvnames;
-	const char *pvname = NULL;
+	char *pvname = NULL;
 	char *colon, *tagname;
+	char *ptr, *ptr_end;
 	int i;
 
 	/* Build up list of PVs */
@@ -1128,9 +1129,21 @@ struct dm_list *create_pv_list(struct dm_pool *mem, struct volume_group *vg, int
 			continue;
 		}
 
-		pvname = argv[i];
+		ptr = pvname = argv[i];
+		ptr_end = strchr(ptr, '\0');
+		colon = NULL;
 
-		if ((colon = strchr(pvname, ':'))) {
+		while ((ptr = strchr(ptr, ':'))) {
+			if (ptr[1] == ':') {
+				memmove(ptr, ptr + 1, ptr_end - ptr);
+				ptr_end--;
+			}
+			else if (!colon)
+				colon = ptr;
+			ptr++;
+		}
+
+		if (colon) {
 			if (!(pvname = dm_pool_strndup(mem, pvname,
 						    (unsigned) (colon -
 								pvname)))) {

[PATCH] Add escape sequence for ':' in command's PV list

[Dave Wysochanski]

On Wed, 2009-05-13 at 13:09 +0200, Peter Rajnoha wrote:
> Hi,
> 
> this little patch provides an escape sequence in command's PV list
> where ':' is also used as a delimiter for extent ranges. To escape
> this char, we just double it, so '::' is translated to ':' and ':'
> is extent range delimiter.
> (BZ #491409)
> 
> Peter
> 
> 
> diff --git a/tools/toollib.c b/tools/toollib.c
> index aa33468..b5a43b5 100644
> --- a/tools/toollib.c
> +++ b/tools/toollib.c
> @@ -1095,8 +1095,9 @@ struct dm_list *create_pv_list(struct dm_pool *mem, struct volume_group *vg, int
>  	struct dm_list *r;
>  	struct pv_list *pvl;
>  	struct dm_list tags, arg_pvnames;
> -	const char *pvname = NULL;
> +	char *pvname = NULL;
>  	char *colon, *tagname;
> +	char *ptr, *ptr_end;
>  	int i;
>  
>  	/* Build up list of PVs */
> @@ -1128,9 +1129,21 @@ struct dm_list *create_pv_list(struct dm_pool *mem, struct volume_group *vg, int
>  			continue;
>  		}
>  
> -		pvname = argv[i];
> +		ptr = pvname = argv[i];
> +		ptr_end = strchr(ptr, '\0');
> +		colon = NULL;
>  
> -		if ((colon = strchr(pvname, ':'))) {
> +		while ((ptr = strchr(ptr, ':'))) {
> +			if (ptr[1] == ':') {
> +				memmove(ptr, ptr + 1, ptr_end - ptr);
> +				ptr_end--;
> +			}
> +			else if (!colon)
> +				colon = ptr;
> +			ptr++;
> +		}
> +
> +		if (colon) {
>  			if (!(pvname = dm_pool_strndup(mem, pvname,
>  						    (unsigned) (colon -
>  								pvname)))) {

Reviewed and the code works as advertised, however...

This code should probably go inside lvm-string.c with the other code
that handles special characters.
Also, think about consistency among pvcreate, vgcreate, and lvcreate.

Attached is a patch for a small test I put together that I think should
work once this bug is fixed.  It does not work currently because
arguments to pvcreate and vgcreate are handled differently than lvcreate
(it will pass if you change these args to just $dev1 instead of
$dev1_escaped).  I know, it does not make sense to specify a pe range
for pvcreate or vgcreate.  But shouldn't we be consistent, or at least
very much clarify the arguments are different?  I can just imagine
someone scripting something and then being surprised when they needed to
quote only for lvcreate, pvmove, etc.

To see where it fails look at the verbose output of pvcreate or you can
just read the pvcreate code.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-pvcreate-test-for-escaped.patch
Type: text/x-patch
Size: 1800 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090513/efcbda86/attachment.bin>

[PATCH] Add escape sequence for ':' in command's PV list

[Milan Broz]

> +dev1_escaped=`echo $dev1 | awk '{ print substr($1,1,index($1,":")-1)"::"substr($1,index($1,":")+1) }'`

please can we somehow standardize which tools are required for testsuite?

or at least first test that the tools are available - like awk, mdadm, mke2fsck...

Milan

[PATCH] Add escape sequence for ':' in command's PV list

[Peter Rajnoha]

On 05/13/2009 09:43 PM, Dave Wysochanski wrote:
> I know, it does not make sense to specify a pe range
> for pvcreate or vgcreate.  But shouldn't we be consistent, or at least
> very much clarify the arguments are different?  I can just imagine
> someone scripting something and then being surprised when they needed to
> quote only for lvcreate, pvmove, etc.

Hmm, then the question is, what should we do with single unescaped ':' if
it appears in pvcreate, vgcreate, e.g. "pvcreate /dev/a::b:c" -- the first
double one is OK, but what about the second single one? Should it be considered
as an input error then? (...probably giving the user a message like
"PE range definition not allowed here.")

Peter

[PATCH] Add escape sequence for ':' in command's PV list

[Dave Wysochanski]

On Thu, 2009-05-14 at 11:33 +0200, Peter Rajnoha wrote: 
> On 05/13/2009 09:43 PM, Dave Wysochanski wrote:
> > I know, it does not make sense to specify a pe range
> > for pvcreate or vgcreate.  But shouldn't we be consistent, or at least
> > very much clarify the arguments are different?  I can just imagine
> > someone scripting something and then being surprised when they needed to
> > quote only for lvcreate, pvmove, etc.
> 
> Hmm, then the question is, what should we do with single unescaped ':' if
> it appears in pvcreate, vgcreate, e.g. "pvcreate /dev/a::b:c" -- the first
> double one is OK, but what about the second single one? Should it be considered
> as an input error then? (...probably giving the user a message like
> "PE range definition not allowed here.")
> 

At this point I'd lean towards just clarifing the difference in the
arguments for the commands (e.g. in man pages, etc).  See attached patch
for a stab at updating the man pages.  If we go this route, then your
patch is fine as is IMO.

Also, on IRC yesterday, Alasdair suggested a new built-in command to
escape names such as this 
17:01:40 < deepthot> agk_: on the escaping, you mean someone would do something like: lvcreate -L 16M -n lvname vgname `lvmescape /dev/pname`?
17:05:07 < agk_> almost:   lvmescape /dev/path   then they can append :4-10 on it

IMO, this is a good idea.  However, in the spirit of not getting too
sidetracked, I think if we add proper explanation, we can probably defer
adding the built-in command, which probably requires some design thought
(for example, how to specify whether it is a pvname, vgname, etc - they
may have different characters to escape, etc).  If you want to tackle
that too though, go for it.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Update-man-pages-to-clarify-usage-of-PE-ranges-and.patch
Type: text/x-patch
Size: 5291 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090514/bb0b9abe/attachment.bin>

[PATCH] Add escape sequence for ':' in command's PV list

[Dave Wysochanski]

On Thu, 2009-05-14 at 15:25 -0400, Dave Wysochanski wrote:
> On Thu, 2009-05-14 at 11:33 +0200, Peter Rajnoha wrote: 
> > On 05/13/2009 09:43 PM, Dave Wysochanski wrote:
> > > I know, it does not make sense to specify a pe range
> > > for pvcreate or vgcreate.  But shouldn't we be consistent, or at least
> > > very much clarify the arguments are different?  I can just imagine
> > > someone scripting something and then being surprised when they needed to
> > > quote only for lvcreate, pvmove, etc.
> > 
> > Hmm, then the question is, what should we do with single unescaped ':' if
> > it appears in pvcreate, vgcreate, e.g. "pvcreate /dev/a::b:c" -- the first
> > double one is OK, but what about the second single one? Should it be considered
> > as an input error then? (...probably giving the user a message like
> > "PE range definition not allowed here.")
> > 
> 
> At this point I'd lean towards just clarifing the difference in the
> arguments for the commands (e.g. in man pages, etc).  See attached patch
> for a stab at updating the man pages.  If we go this route, then your
> patch is fine as is IMO.
> 
> Also, on IRC yesterday, Alasdair suggested a new built-in command to
> escape names such as this 
> 17:01:40 < deepthot> agk_: on the escaping, you mean someone would do something like: lvcreate -L 16M -n lvname vgname `lvmescape /dev/pname`?
> 17:05:07 < agk_> almost:   lvmescape /dev/path   then they can append :4-10 on it
> 
> IMO, this is a good idea.  However, in the spirit of not getting too
> sidetracked, I think if we add proper explanation, we can probably defer
> adding the built-in command, which probably requires some design thought
> (for example, how to specify whether it is a pvname, vgname, etc - they
> may have different characters to escape, etc).  If you want to tackle
> that too though, go for it.
> 

Upon further thought, I agree with you Peter - let's reject the ":" in
pvcreate/vgcreate commands and print a message about pe range not
allowed in these commands.  This way the commands are all consistent -
require a "::" if any pvname contains a ":" in any of the commands.

[PATCH] Add escape sequence for ':' in command's PV list

[Dave Wysochanski]

On Fri, 2009-05-15 at 08:07 -0400, Dave Wysochanski wrote:
> On Thu, 2009-05-14 at 15:25 -0400, Dave Wysochanski wrote:
> > On Thu, 2009-05-14 at 11:33 +0200, Peter Rajnoha wrote: 
> > > On 05/13/2009 09:43 PM, Dave Wysochanski wrote:
> > > > I know, it does not make sense to specify a pe range
> > > > for pvcreate or vgcreate.  But shouldn't we be consistent, or at least
> > > > very much clarify the arguments are different?  I can just imagine
> > > > someone scripting something and then being surprised when they needed to
> > > > quote only for lvcreate, pvmove, etc.
> > > 
> > > Hmm, then the question is, what should we do with single unescaped ':' if
> > > it appears in pvcreate, vgcreate, e.g. "pvcreate /dev/a::b:c" -- the first
> > > double one is OK, but what about the second single one? Should it be considered
> > > as an input error then? (...probably giving the user a message like
> > > "PE range definition not allowed here.")
> > > 
> > 
> > At this point I'd lean towards just clarifing the difference in the
> > arguments for the commands (e.g. in man pages, etc).  See attached patch
> > for a stab at updating the man pages.  If we go this route, then your
> > patch is fine as is IMO.
> > 
> > Also, on IRC yesterday, Alasdair suggested a new built-in command to
> > escape names such as this 
> > 17:01:40 < deepthot> agk_: on the escaping, you mean someone would do something like: lvcreate -L 16M -n lvname vgname `lvmescape /dev/pname`?
> > 17:05:07 < agk_> almost:   lvmescape /dev/path   then they can append :4-10 on it
> > 
> > IMO, this is a good idea.  However, in the spirit of not getting too
> > sidetracked, I think if we add proper explanation, we can probably defer
> > adding the built-in command, which probably requires some design thought
> > (for example, how to specify whether it is a pvname, vgname, etc - they
> > may have different characters to escape, etc).  If you want to tackle
> > that too though, go for it.
> > 
> 
> Upon further thought, I agree with you Peter - let's reject the ":" in
> pvcreate/vgcreate commands and print a message about pe range not
> allowed in these commands.  This way the commands are all consistent -
> require a "::" if any pvname contains a ":" in any of the commands.
> 

Updated man page patch is attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Update-man-pages-to-clarify-usage-of-PE-ranges-and.patch
Type: text/x-patch
Size: 5575 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090515/284e1366/attachment.bin>

[PATCH] Add escape sequence for ':' in command's PV list

[Peter Rajnoha]

On 05/15/2009 02:27 PM, Dave Wysochanski wrote:
>> Upon further thought, I agree with you Peter - let's reject the ":" in
>> pvcreate/vgcreate commands and print a message about pe range not
>> allowed in these commands.  This way the commands are all consistent -
>> require a "::" if any pvname contains a ":" in any of the commands.
>>
> 
> Updated man page patch is attached.
> 

OK, thanks. I'll move the code to a separate function then and I'll add checks
for pvcreate/vgcreate, too.

Peter

[PATCH] Add escape sequence for ':' in command's PV list

[Alasdair G Kergon]

On Fri, May 15, 2009 at 07:57:50PM +0200, Peter Rajnoha wrote:
> OK, thanks. I'll move the code to a separate function then and I'll add checks
> for pvcreate/vgcreate, too.
 
Main thing I'm bothered about here is applying a simple policy across the board.
Having different policies in different places is a recipe for confusion.

[Note that doubling the hyphens in device names does not set a precedent
as that is meant to be an internal tool matter, not exposed to the user who
uses /dev/vgname/lvname.]

How many more things need escaping?

Is the policy "any character that needs escaping will be doubled"?  

Or do we go for "any character that needs escaping will be preceded by a
backslash"?
(See config strings - does this work with config settings on cmdline too?)

Alasdair

[PATCH] Add escape sequence for ':' in command's PV list

[Peter Rajnoha]

This is reincarnation of simple, old and forgotten (and not so important) patch
I sent a long long time ago but it faded away somehow. But I'd like to have
this cleaned up finally (..to not make a mess on my desk :)).

At that time, I used doubling of characters to escape the ":". But since
we use "\" everywhere else, let's do it this way. (rhbz #491409)

(I remember there was a discussion about adding some form of generic
mechanism for escaping characters by means of adding a new command to
do that. Do we still want that? Is that really necessary?)

Peter
---
 lib/misc/lvm-string.c |   20 ++++++++++++++++++--
 lib/misc/lvm-string.h |    6 ++++++
 tools/toollib.c       |   15 +++++++--------
 3 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/lib/misc/lvm-string.c b/lib/misc/lvm-string.c
index 7eed799..6c1c1a5 100644
--- a/lib/misc/lvm-string.c
+++ b/lib/misc/lvm-string.c
@@ -105,14 +105,19 @@ static void _quote_characters(char **out, const char *src,
  * Also unquote quote_char.
  */
 static void _unquote_characters(char *src, const int orig_char,
-				const int quote_char)
+				const int quote_char,
+				char **substr_first_unquoted)
 {
 	char *out = src;
 
+	*substr_first_unquoted = NULL;
+
 	while (*src) {
 		if (*src == quote_char &&
 		    (*(src + 1) == orig_char || *(src + 1) == quote_char))
 			src++;
+		else if (*src == orig_char)
+			*substr_first_unquoted = out;
 
 		*out++ = *src++;
 	}
@@ -209,7 +214,18 @@ char *escape_double_quotes(char *out, const char *src)
  */
 void unescape_double_quotes(char *src)
 {
-	_unquote_characters(src, '\"', '\\');
+	char *s;
+
+	_unquote_characters(src, '\"', '\\', &s);
+}
+
+/*
+ * Unescape colons in situ and save the substring starting
+ * at the position of the first unescaped colon.
+ */
+void unescape_colons(char *src, char **substr_first_unquoted)
+{
+	_unquote_characters(src, ':', '\\', substr_first_unquoted);
 }
 
 /*
diff --git a/lib/misc/lvm-string.h b/lib/misc/lvm-string.h
index 35d9245..f649fdc 100644
--- a/lib/misc/lvm-string.h
+++ b/lib/misc/lvm-string.h
@@ -60,4 +60,10 @@ char *escape_double_quotes(char *out, const char *src);
  */
 void unescape_double_quotes(char *src);
 
+/*
+ * Unescape colons in situ and save the substring starting
+ * at the position of the first unescaped colon.
+ */
+void unescape_colons(char *src, char **substr_first_unquoted);
+
 #endif
diff --git a/tools/toollib.c b/tools/toollib.c
index 5da30f4..104f33c 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -1093,7 +1093,7 @@ struct dm_list *create_pv_list(struct dm_pool *mem, struct volume_group *vg, int
 	struct dm_list *r;
 	struct pv_list *pvl;
 	struct dm_list tags, arg_pvnames;
-	const char *pvname = NULL;
+	char *pvname = NULL;
 	char *colon, *tagname;
 	int i;
 
@@ -1128,13 +1128,12 @@ struct dm_list *create_pv_list(struct dm_pool *mem, struct volume_group *vg, int
 
 		pvname = argv[i];
 
-		if ((colon = strchr(pvname, ':'))) {
-			if (!(pvname = dm_pool_strndup(mem, pvname,
-						    (unsigned) (colon -
-								pvname)))) {
-				log_error("Failed to clone PV name");
-				return NULL;
-			}
+		unescape_colons(pvname, &colon);
+
+		if (colon && !(pvname = dm_pool_strndup(mem, pvname,
+					(unsigned) (colon - pvname)))) {
+			log_error("Failed to clone PV name");
+			return NULL;
 		}
 
 		if (!(pvl = find_pv_in_vg(vg, pvname))) {