Re: [PATCH 7/9] Simplify string_escape_mem
On Thu, Sep 05, 2019 at 03:44:31PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields"
>
> string_escape_mem is harder to use than necessary:
>
> - ESCAPE_NP sounds like it means "escape nonprinting
> characters", but actually means "do not escape printing
> characters"
> - the use of the "only" string to limit the list of escaped
> characters rather than supplement them is confusing and
> unehlpful.
>
> So:
>
> - use the "only" string to add to, rather than replace, the list
> of characters to escape
> - separate flags into those that select which characters to
> escape, and those that choose the format of the escaping ("\ "
> vs "\x20" vs "\040".) Make flags that select characters just
> select the union when they're OR'd together.
>
> Untested. The tests themselves, especially, I'm unsure about.
I'll take a look at these too. Notes below...
>
> Signed-off-by: J. Bruce Fields
> ---
> fs/proc/array.c| 2 +-
> fs/seq_file.c | 2 +-
> include/linux/string_helpers.h | 14 +++
> lib/string_helpers.c | 76 +++---
> lib/test-string_helpers.c | 41 --
> lib/vsprintf.c | 2 +-
> net/sunrpc/cache.c | 2 +-
> 7 files changed, 62 insertions(+), 77 deletions(-)
>
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index 982c95d09176..bdeeb3318fa2 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -111,7 +111,7 @@ void proc_task_name(struct seq_file *m, struct
> task_struct *p, bool escape)
> size = seq_get_buf(m, );
> if (escape) {
> ret = string_escape_str(tcomm, buf, size,
> - ESCAPE_SPECIAL, "\n\\");
> + ESCAPE_STYLE_SLASH, "\n\\");
> if (ret >= size)
> ret = -1;
> } else {
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 1600034a929b..63e5a7c4dbf7 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -379,7 +379,7 @@ void seq_escape(struct seq_file *m, const char *s, const
> char *esc)
> size_t size = seq_get_buf(m, );
> int ret;
>
> - ret = string_escape_str(s, buf, size, ESCAPE_OCTAL, esc);
> + ret = string_escape_str(s, buf, size, ESCAPE_STYLE_OCTAL, esc);
> seq_commit(m, ret < size ? ret : -1);
> }
> EXPORT_SYMBOL(seq_escape);
> diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
> index 7bf0d137373d..5d350f7f6874 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -41,13 +41,13 @@ static inline int string_unescape_any_inplace(char *buf)
> return string_unescape_any(buf, buf, 0);
> }
>
> -#define ESCAPE_SPECIAL 0x02
> -#define ESCAPE_OCTAL 0x08
> -#define ESCAPE_ANY (ESCAPE_OCTAL | ESCAPE_SPECIAL)
> -#define ESCAPE_NP0x10
> -#define ESCAPE_ANY_NP(ESCAPE_ANY | ESCAPE_NP)
> -#define ESCAPE_HEX 0x20
> -#define ESCAPE_FLAG_MASK 0x3a
> +#define ESCAPE_SPECIAL 0x01
> +#define ESCAPE_NP0x02
> +#define ESCAPE_ANY_NP(ESCAPE_SPECIAL | ESCAPE_NP)
> +#define ESCAPE_STYLE_SLASH 0x20
> +#define ESCAPE_STYLE_OCTAL 0x40
> +#define ESCAPE_STYLE_HEX 0x80
> +#define ESCAPE_FLAG_MASK 0xe3
>
> int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
> unsigned int flags, const char *only);
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index ac72159d3980..47f40406f9d4 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -400,6 +400,11 @@ static bool escape_hex(unsigned char c, char **dst, char
> *end)
> return true;
> }
>
> +static bool is_special(char c)
> +{
> + return c == '\0' || strchr("\f\n\r\t\v\\\a\e", c);
> +}
> +
> /**
> * string_escape_mem - quote characters in the given memory buffer
> * @src: source buffer (unescaped)
> @@ -407,23 +412,18 @@ static bool escape_hex(unsigned char c, char **dst,
> char *end)
> * @dst: destination buffer (escaped)
> * @osz: destination buffer size
> * @flags: combination of the flags
> - * @only:NULL-terminated string containing characters used to limit
> - * the selected escape class. If characters are included in @only
> - * that would not normally be escaped by the classes selected
> - * in @flags, they will be copied to @dst unescaped.
> + * @esc: NULL-terminated string containing characters which
> + * should also be escaped.
> *
> - * Description:
> - * The process of escaping byte buffer includes several parts. They are
> applied
> - * in the following sequence.
> *
> - * 1. The character is matched to the printable class, if asked, and in
> - * case of match it passes through to the output.
> - * 2. The
[PATCH 7/9] Simplify string_escape_mem
From: "J. Bruce Fields"
string_escape_mem is harder to use than necessary:
- ESCAPE_NP sounds like it means "escape nonprinting
characters", but actually means "do not escape printing
characters"
- the use of the "only" string to limit the list of escaped
characters rather than supplement them is confusing and
unehlpful.
So:
- use the "only" string to add to, rather than replace, the list
of characters to escape
- separate flags into those that select which characters to
escape, and those that choose the format of the escaping ("\ "
vs "\x20" vs "\040".) Make flags that select characters just
select the union when they're OR'd together.
Untested. The tests themselves, especially, I'm unsure about.
Signed-off-by: J. Bruce Fields
---
fs/proc/array.c| 2 +-
fs/seq_file.c | 2 +-
include/linux/string_helpers.h | 14 +++
lib/string_helpers.c | 76 +++---
lib/test-string_helpers.c | 41 --
lib/vsprintf.c | 2 +-
net/sunrpc/cache.c | 2 +-
7 files changed, 62 insertions(+), 77 deletions(-)
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 982c95d09176..bdeeb3318fa2 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -111,7 +111,7 @@ void proc_task_name(struct seq_file *m, struct task_struct
*p, bool escape)
size = seq_get_buf(m, );
if (escape) {
ret = string_escape_str(tcomm, buf, size,
- ESCAPE_SPECIAL, "\n\\");
+ ESCAPE_STYLE_SLASH, "\n\\");
if (ret >= size)
ret = -1;
} else {
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 1600034a929b..63e5a7c4dbf7 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -379,7 +379,7 @@ void seq_escape(struct seq_file *m, const char *s, const
char *esc)
size_t size = seq_get_buf(m, );
int ret;
- ret = string_escape_str(s, buf, size, ESCAPE_OCTAL, esc);
+ ret = string_escape_str(s, buf, size, ESCAPE_STYLE_OCTAL, esc);
seq_commit(m, ret < size ? ret : -1);
}
EXPORT_SYMBOL(seq_escape);
diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 7bf0d137373d..5d350f7f6874 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -41,13 +41,13 @@ static inline int string_unescape_any_inplace(char *buf)
return string_unescape_any(buf, buf, 0);
}
-#define ESCAPE_SPECIAL 0x02
-#define ESCAPE_OCTAL 0x08
-#define ESCAPE_ANY (ESCAPE_OCTAL | ESCAPE_SPECIAL)
-#define ESCAPE_NP 0x10
-#define ESCAPE_ANY_NP (ESCAPE_ANY | ESCAPE_NP)
-#define ESCAPE_HEX 0x20
-#define ESCAPE_FLAG_MASK 0x3a
+#define ESCAPE_SPECIAL 0x01
+#define ESCAPE_NP 0x02
+#define ESCAPE_ANY_NP (ESCAPE_SPECIAL | ESCAPE_NP)
+#define ESCAPE_STYLE_SLASH 0x20
+#define ESCAPE_STYLE_OCTAL 0x40
+#define ESCAPE_STYLE_HEX 0x80
+#define ESCAPE_FLAG_MASK 0xe3
int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
unsigned int flags, const char *only);
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index ac72159d3980..47f40406f9d4 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -400,6 +400,11 @@ static bool escape_hex(unsigned char c, char **dst, char
*end)
return true;
}
+static bool is_special(char c)
+{
+ return c == '\0' || strchr("\f\n\r\t\v\\\a\e", c);
+}
+
/**
* string_escape_mem - quote characters in the given memory buffer
* @src: source buffer (unescaped)
@@ -407,23 +412,18 @@ static bool escape_hex(unsigned char c, char **dst, char
*end)
* @dst: destination buffer (escaped)
* @osz: destination buffer size
* @flags: combination of the flags
- * @only: NULL-terminated string containing characters used to limit
- * the selected escape class. If characters are included in @only
- * that would not normally be escaped by the classes selected
- * in @flags, they will be copied to @dst unescaped.
+ * @esc: NULL-terminated string containing characters which
+ * should also be escaped.
*
- * Description:
- * The process of escaping byte buffer includes several parts. They are applied
- * in the following sequence.
*
- * 1. The character is matched to the printable class, if asked, and in
- *case of match it passes through to the output.
- * 2. The character is not matched to the one from @only string and thus
- *must go as-is to the output.
- * 3. The character is checked if it falls into the class given by @flags.
- *%ESCAPE_OCTAL and %ESCAPE_HEX are going last since they cover any
- *character.
