[v3,1/4] btrfs-progs: utils: Introduce function to escape characters
diff mbox

Message ID 20161101080147.13163-2-quwenruo@cn.fujitsu.com
State New
Headers show

Commit Message

Qu Wenruo Nov. 1, 2016, 8:01 a.m. UTC
Introduce new function, escape_string_inplace(), to escape specified
characters in place.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 utils.c | 17 +++++++++++++++++
 utils.h |  2 ++
 2 files changed, 19 insertions(+)

Comments

David Sterba Nov. 1, 2016, 10:08 a.m. UTC | #1
On Tue, Nov 01, 2016 at 04:01:43PM +0800, Qu Wenruo wrote:
> Introduce new function, escape_string_inplace(), to escape specified
> characters in place.

Sorry, the pointer to seq_path was misleading. The actual escape
function is mangle_path and it copies one string to another. As we just
print the path, we can simply switch and call putchar.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Qu Wenruo Nov. 2, 2016, 1:19 a.m. UTC | #2
At 11/01/2016 06:08 PM, David Sterba wrote:
> On Tue, Nov 01, 2016 at 04:01:43PM +0800, Qu Wenruo wrote:
>> Introduce new function, escape_string_inplace(), to escape specified
>> characters in place.
>
> Sorry, the pointer to seq_path was misleading. The actual escape
> function is mangle_path and it copies one string to another. As we just
> print the path, we can simply switch and call putchar.
>

Putchar() method is indeed much easier to implement.

But it makes us hard to do further formatting, like aligning the path to 
given width. (At least we are still using 32 chars alignment for path)

So I still prefer the current full function string escaping and still 
use %-32s for formatting.


And the idea of implementing escape_string_inplace() as a pure string 
manipulation function can make it more agile for later use.
For example, we can reuse it for print-tree.

Although the current esacpe_string_inplace() can be further fine-tuned 
to avoid calling memmove() in a loop, which I can update it in next version.

Thanks,
Qu


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba Nov. 2, 2016, 10:55 a.m. UTC | #3
On Wed, Nov 02, 2016 at 09:19:10AM +0800, Qu Wenruo wrote:
> 
> 
> At 11/01/2016 06:08 PM, David Sterba wrote:
> > On Tue, Nov 01, 2016 at 04:01:43PM +0800, Qu Wenruo wrote:
> >> Introduce new function, escape_string_inplace(), to escape specified
> >> characters in place.
> >
> > Sorry, the pointer to seq_path was misleading. The actual escape
> > function is mangle_path and it copies one string to another. As we just
> > print the path, we can simply switch and call putchar.
> >
> 
> Putchar() method is indeed much easier to implement.
> 
> But it makes us hard to do further formatting, like aligning the path to 
> given width. (At least we are still using 32 chars alignment for path)
> 
> So I still prefer the current full function string escaping and still 
> use %-32s for formatting.
> 
> 
> And the idea of implementing escape_string_inplace() as a pure string 
> manipulation function can make it more agile for later use.
> For example, we can reuse it for print-tree.

Reusing is fine, but I really don't like that the function modifies the
argument. What if the function is called twice on the same string? Also,
in the print-tree, this would mean the extent buffer would be modified,
potentially overwriting other items.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Qu Wenruo Nov. 3, 2016, 12:24 a.m. UTC | #4
At 11/02/2016 06:55 PM, David Sterba wrote:
> On Wed, Nov 02, 2016 at 09:19:10AM +0800, Qu Wenruo wrote:
>>
>>
>> At 11/01/2016 06:08 PM, David Sterba wrote:
>>> On Tue, Nov 01, 2016 at 04:01:43PM +0800, Qu Wenruo wrote:
>>>> Introduce new function, escape_string_inplace(), to escape specified
>>>> characters in place.
>>>
>>> Sorry, the pointer to seq_path was misleading. The actual escape
>>> function is mangle_path and it copies one string to another. As we just
>>> print the path, we can simply switch and call putchar.
>>>
>>
>> Putchar() method is indeed much easier to implement.
>>
>> But it makes us hard to do further formatting, like aligning the path to
>> given width. (At least we are still using 32 chars alignment for path)
>>
>> So I still prefer the current full function string escaping and still
>> use %-32s for formatting.
>>
>>
>> And the idea of implementing escape_string_inplace() as a pure string
>> manipulation function can make it more agile for later use.
>> For example, we can reuse it for print-tree.
>
> Reusing is fine, but I really don't like that the function modifies the
> argument. What if the function is called twice on the same string? Also,
> in the print-tree, this would mean the extent buffer would be modified,
> potentially overwriting other items.
>
>
Then just encapsulate the in-place version into memory allocation version.

In-place version can be encapsulated very easily, while it's not 
possible vice verse.

So there will be two functions, escape_string_inplace() and 
escape_string() for caller to choose.

Would this be OK?

Thanks,
Qu


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Qu Wenruo Nov. 3, 2016, 1:14 a.m. UTC | #5
At 11/02/2016 06:55 PM, David Sterba wrote:
> On Wed, Nov 02, 2016 at 09:19:10AM +0800, Qu Wenruo wrote:
>>
>>
>> At 11/01/2016 06:08 PM, David Sterba wrote:
>>> On Tue, Nov 01, 2016 at 04:01:43PM +0800, Qu Wenruo wrote:
>>>> Introduce new function, escape_string_inplace(), to escape specified
>>>> characters in place.
>>>
>>> Sorry, the pointer to seq_path was misleading. The actual escape
>>> function is mangle_path and it copies one string to another. As we just
>>> print the path, we can simply switch and call putchar.
>>>
>>
>> Putchar() method is indeed much easier to implement.
>>
>> But it makes us hard to do further formatting, like aligning the path to
>> given width. (At least we are still using 32 chars alignment for path)
>>
>> So I still prefer the current full function string escaping and still
>> use %-32s for formatting.
>>
>>
>> And the idea of implementing escape_string_inplace() as a pure string
>> manipulation function can make it more agile for later use.
>> For example, we can reuse it for print-tree.
>
> Reusing is fine, but I really don't like that the function modifies the
> argument. What if the function is called twice on the same string?
Forgot to ask, what's the problem calling the function twice on the same 
string?

escape_string_inplace(dest, " ");
escape_string_inplace(dest, "\n");

is just the same as

escape_string_inplace(dest, " \n");

So I see no problem here.

Thanks,
Qu


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/utils.c b/utils.c
index 3f54245..69faae1 100644
--- a/utils.c
+++ b/utils.c
@@ -4251,3 +4251,20 @@  unsigned int rand_range(unsigned int upper)
 	 */
 	return (unsigned int)(jrand48(rand_seed) % upper);
 }
+
+void string_escape_inplace(char *restrict string, char *restrict escape_chars)
+{
+	int i, j;
+
+	for (i = 0; i < strlen(escape_chars); i++) {
+		j = 0;
+		while (j < strlen(string)) {
+			if (string[j] == escape_chars[i]) {
+				memmove(string + j, string + j + 1,
+					strlen(string) -j);
+				continue;
+			}
+			j++;
+		}
+	}
+}
diff --git a/utils.h b/utils.h
index 1a2dbcd..b36d411 100644
--- a/utils.h
+++ b/utils.h
@@ -457,4 +457,6 @@  unsigned int rand_range(unsigned int upper);
 /* Also allow setting the seed manually */
 void init_rand_seed(u64 seed);
 
+void string_escape_inplace(char *restrict string, char *restrict escape_chars);
+
 #endif