Libguestfs - Oct 2014 - [PATCH 1/3] fish: rl.{c, h} - escaping functions for readline

From: Maros Zatko <hacxman@gmail.com>

Auxiliary functions for readline to support space character escaping
in filenames in future.

Escaping function is taken from fish.c (used to be parse_quoted_string)
plus its un-escaping counterpart. There are a few tests for both.

Maros Zatko (3):
  fish: rl.{c,h} - escaping functions for readline
  fish: basic tests for readline escaping
  autotools: add fish/test

 Makefile.am             |   1 +
 configure.ac            |   1 +
 fish/rl.c               | 158 ++++++++++++++++++++++++++++++++++++++++++++++++
 fish/rl.h               |  32 ++++++++++
 fish/test/Makefile.am   |  39 ++++++++++++
 fish/test/testquoting.c | 120 ++++++++++++++++++++++++++++++++++++
 6 files changed, 351 insertions(+)
 create mode 100644 fish/rl.c
 create mode 100644 fish/rl.h
 create mode 100644 fish/test/Makefile.am
 create mode 100644 fish/test/testquoting.c

-- 
1.9.3

From: Maros Zatko <mzatko@redhat.com>

---
 fish/rl.c | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fish/rl.h |  32 +++++++++++++
 2 files changed, 190 insertions(+)
 create mode 100644 fish/rl.c
 create mode 100644 fish/rl.h

diff --git a/fish/rl.c b/fish/rl.c
new file mode 100644
index 0000000..bb8fd62
--- /dev/null
+++ b/fish/rl.c
@@ -0,0 +1,158 @@
+/* guestfish - guest filesystem shell
+ * Copyright (C) 2009-2014 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <memory.h>
+
+#include <c-ctype.h>
+
+#include "rl.h"
+
+static char program_name[] = "fish";
+
+int
+hexdigit (char d)
+{
+  switch (d) {
+  case '0'...'9': return d - '0';
+  case 'a'...'f': return d - 'a' + 10;
+  case 'A'...'F': return d - 'A' + 10;
+  default: return -1;
+  }
+}
+
+// backslash unescape for readline
+char *
+bs_unescape_filename (char *str)
+{
+  char *p = calloc(strlen(str) + 1, 1);
+  strcpy(p, str);
+  char *start = p;
+
+  for (; *p; p++) {
+    if (*p == '\\') {
+      int m = 1, c;
+
+      switch (p[1]) {
+      case '\\': break;
+      case 'a': *p = '\a'; break;
+      case 'b': *p = '\b'; break;
+      case 'f': *p = '\f'; break;
+      case 'n': *p = '\n'; break;
+      case 'r': *p = '\r'; break;
+      case 't': *p = '\t'; break;
+      case 'v': *p = '\v'; break;
+      case '"': *p = '"'; break;
+      case '\'': *p = '\''; break;
+      case '?': *p = '?'; break;
+      case ' ': *p = ' '; break;
+
+      case '0'...'7':           /* octal escape - always 3
digits */
+        m = 3;
+        if (p[2] >= '0' && p[2] <= '7' &&
+            p[3] >= '0' && p[3] <= '7') {
+          c = (p[1] - '0') * 0100 + (p[2] - '0') * 010 + (p[3]
- '0');
+          if (c < 1 || c > 255)
+            goto error;
+          *p = c;
+        }
+        else
+          goto error;
+        break;
+
+      case 'x':                 /* hex escape - always 2 digits */
+        m = 3;
+        if (c_isxdigit (p[2]) && c_isxdigit (p[3])) {
+          c = hexdigit (p[2]) * 0x10 + hexdigit (p[3]);
+          if (c < 1 || c > 255)
+            goto error;
+          *p = c;
+        }
+        else
+          goto error;
+        break;
+
+      default:
+      error:
+        fprintf (stderr, ("%s: invalid escape sequence in string (starting
at offset %d)\n"),
+                 program_name, (int) (p - start));
+        return NULL;
+      }
+      memmove (p+1, p+1+m, strlen (p+1+m) + 1);
+    }
+  }
+
+  return start;
+}
+
+// backslash scape
+char *
+bs_escape_filename (char *p)
+{
+  char *start = p;
+  // four times original length - if all chars are unprintable
+  // new string would be \xXY\xWZ
+  char *n = calloc(strlen(p) * 4 + 1, 1);
+  char *nstart = n;
+  if (strlen(p) == 0) {
+    n[0] = '\0';
+    return n;
+  }
+
+  for (; *p; p++, n++) {
+      int m = 1;
+
+      switch (*p) {
+      case '\\': break;
+      case '\a': *(n++) = '\\'; *n = 'a'; break;
+      case '\b': *(n++) = '\\'; *n = 'b'; break;
+      case '\f': *(n++) = '\\'; *n = 'f'; break;
+      case '\n': *(n++) = '\\'; *n = 'n'; break;
+      case '\r': *(n++) = '\\'; *n = 'r'; break;
+      case '\t': *(n++) = '\\'; *n = 't'; break;
+      case '\v': *(n++) = '\\'; *n = 'v'; break;
+      case '"':  *(n++) = '\\'; *n = '"';
break;
+      case '\'': *(n++) = '\\'; *n = '\'';
break;
+      case '?':  *(n++) = '\\'; *n = '?'; break;
+      case ' ':  *(n++) = '\\'; *n = ' '; break;
+
+      default:
+        // Hexadecimal escape unprintable character. This violates identity
+        // after composition of bsquote_filename after debsquote_filename
+        // (i.e. can escape some characters differently).
+        if (!c_isprint(*p)) {
+          n += sprintf(n, "\\x%x", (int) (*p & 0xff)) - 1;
+        } else {
+          *n = *p;
+        }
+        break;
+      error:
+        fprintf (stderr, ("%s: invalid escape sequence in string (starting
at offset %d)\n"),
+                 program_name, (int) (p - start));
+        return NULL;
+      }
+  }
+
+  nstart = realloc(nstart, strlen(nstart));
+  return nstart;
+}
+
diff --git a/fish/rl.h b/fish/rl.h
new file mode 100644
index 0000000..366aa6b
--- /dev/null
+++ b/fish/rl.h
@@ -0,0 +1,32 @@
+/* guestfish - guest filesystem shell
+ * Copyright (C) 2009-2014 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA.
+ */
+
+#ifndef _RL_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+extern char * bs_escape_filename (char *p);
+extern char * bs_unescape_filename (char *p);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* !_RL_H */
-- 
1.9.3

From: Maros Zatko <mzatko@redhat.com>

---
 fish/test/Makefile.am   |  39 ++++++++++++++++
 fish/test/testquoting.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 159 insertions(+)
 create mode 100644 fish/test/Makefile.am
 create mode 100644 fish/test/testquoting.c

diff --git a/fish/test/Makefile.am b/fish/test/Makefile.am
new file mode 100644
index 0000000..d108083
--- /dev/null
+++ b/fish/test/Makefile.am
@@ -0,0 +1,39 @@
+# libguestfs
+# Copyright (C) 2009-2014 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+include $(top_srcdir)/subdir-rules.mk
+
+check_PROGRAMS = testquoting
+
+testquoting_SOURCES = \
+	testquoting.c \
+	$(top_srcdir)/fish/rl.c
+
+testquoting_CPPFLAGS = \
+	-I$(top_srcdir)/src -I$(top_builddir)/src \
+	-I$(top_srcdir)/fish -I$(top_builddir)/fish \
+	-I$(top_srcdir)/gnulib/lib -I$(top_builddir)/gnulib/lib
+
+#testquoting_LDADD = $(top_builddir)/fish/rl.o
+
+
+TESTS_ENVIRONMENT = $(top_builddir)/run --test
+
+TESTS = \
+	testquoting
+
+# testquoting_CFLAGS = -I$(top_srcdir)
diff --git a/fish/test/testquoting.c b/fish/test/testquoting.c
new file mode 100644
index 0000000..b0b525b
--- /dev/null
+++ b/fish/test/testquoting.c
@@ -0,0 +1,120 @@
+#include <config.h>
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <memory.h>
+
+#include <guestfs.h>
+#include <guestfs-internal-frontend.h>
+#include <rl.h>
+
+int
+eq_bracket (char *(*fn)(char*), char * in, char * out)
+{
+  char * q = fn(in);
+  return (q != NULL) && STREQ(q, out);
+}
+
+int
+test_empty_escape (void)
+{
+  return eq_bracket(bs_escape_filename, "", "");
+}
+
+int
+test_empty_unescape (void)
+{
+  return eq_bracket(bs_unescape_filename, "", "");
+}
+
+int
+test_singlespace_escape (void)
+{
+  return eq_bracket(bs_escape_filename, " ", "\\ ");
+}
+
+int
+test_singlespace_unescape (void)
+{
+  return eq_bracket(bs_unescape_filename, "\\ ", " ");
+}
+
+int
+test_singleword_escape (void)
+{
+  return eq_bracket(bs_escape_filename, "singleword",
"singleword");
+}
+
+int
+test_singleword_unescape (void)
+{
+  return eq_bracket(bs_unescape_filename, "singleword",
"singleword");
+}
+
+int
+test_multiword_escape (void)
+{
+  return eq_bracket(bs_escape_filename, "more than one word\n",
"more\\ than\\ one\\ word\\n");
+}
+
+int
+test_nonprinting_escape (void)
+{
+  return eq_bracket(bs_escape_filename, "\xac\xec\x8",
"\\xac\\xec\\b");
+}
+
+int
+test_multiword_unescape (void)
+{
+  return eq_bracket(bs_unescape_filename, "more\\ than\\ one\\ word",
"more than one word");
+}
+
+int
+test_nonprinting_unescape (void)
+{
+  return eq_bracket(bs_unescape_filename, "\\xac\\xec\\b",
"\xac\xec\b");
+}
+
+
+
+struct test_t {
+  char *name;
+  int (*fn)(void);
+  int expect;
+};
+
+struct test_t tests[] = {
+  { .name = "test empty escape", .fn = test_empty_escape, .expect =
1},
+  { .name = "test empty unescape", .fn = test_empty_unescape, .expect
= 1},
+  { .name = "test single space escape", .fn =
test_singlespace_escape, .expect = 1},
+  { .name = "test single space unescape", .fn =
test_singlespace_unescape, .expect = 1},
+  { .name = "test single word escape", .fn = test_singleword_escape,
.expect = 1},
+  { .name = "test single word unescape", .fn =
test_singleword_unescape, .expect = 1},
+  { .name = "test multi word escape", .fn = test_multiword_escape,
.expect = 1},
+  { .name = "test nonprinting escape", .fn = test_nonprinting_escape,
.expect = 1},
+  { .name = "test multi word unescape", .fn =
test_multiword_unescape, .expect = 1},
+  { .name = "test nonprinting unescape", .fn =
test_nonprinting_unescape, .expect = 1},
+};
+
+size_t nr_tests = sizeof(tests) / sizeof(*tests);
+
+int
+main (int argc, char *argv[])
+{
+  int nr_failed = 0; // failed test count
+  setbuf(stdout, NULL);
+
+  for (int i = 0; i < nr_tests; i++) {
+    fprintf(stdout, "%s: ", tests[i].name);
+    int r = tests[i].fn() == tests[i].expect;
+    fprintf(stdout, "%s\n", r ? "PASS" : "FAIL");
+    nr_failed += !r;
+  }
+
+  if (nr_failed > 0) {
+    printf ("***** %zu / %zu tests FAILED *****\n", nr_failed,
nr_tests);
+  }
+
+  return nr_failed > 0 ? EXIT_FAILURE : EXIT_SUCCESS;
+}
+
-- 
1.9.3

From: Maros Zatko <mzatko@redhat.com>

---
 Makefile.am  | 1 +
 configure.ac | 1 +
 2 files changed, 2 insertions(+)

diff --git a/Makefile.am b/Makefile.am
index d55d8d6..629b787 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -78,6 +78,7 @@ SUBDIRS += test-tool
 
 # Guestfish.
 SUBDIRS += fish
+SUBDIRS += fish/test
 
 # virt-tools in C.
 SUBDIRS += align cat diff df edit format inspector make-fs rescue
diff --git a/configure.ac b/configure.ac
index a5e9bed..885b3c4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1673,6 +1673,7 @@ AC_CONFIG_FILES([Makefile
                  erlang/examples/Makefile
                  examples/Makefile
                  fish/Makefile
+                 fish/test/Makefile
                  format/Makefile
                  fuse/Makefile
                  generator/Makefile
-- 
1.9.3

Hi Maros,

On Friday 31 October 2014 18:18:13 mzatko@redhat.com
wrote:
> From: Maros Zatko <hacxman@gmail.com>
> 
> Auxiliary functions for readline to support space character escaping
> in filenames in future.
> 
> Escaping function is taken from fish.c (used to be
> parse_quoted_string) plus its un-escaping counterpart. There are a
> few tests for both.
> 
> Maros Zatko (3):
>   fish: rl.{c,h} - escaping functions for readline
>   fish: basic tests for readline escaping
>   autotools: add fish/test
This looks like a good start in improving guestfish escaping/unescaping 
functions.  I have few general questions/notes:

- the two paragraph of descriptions above may better suited directly for
  the patch adding rl.c/h

- given that rl.c seems extracted from guestfish, may be better to do
  the "function extraction" in two steps: first move the functions
from
  fish.c in an own file (adding it to the build system, etc), and then
  do the improvements on them. This way it is easier to track where they
  came from, and making sure guestfish can still use them.

- most probably testquoting can just be directly in fish/test-quoting.c,
  among the other tests

- take care of the indentation, like the space before a parenthesis

I'm adding few more specific notes/questions in patches.

-- 
Pino Toscano

On Friday 31 October 2014 18:18:14 mzatko@redhat.com
wrote:
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <memory.h>
memory.h is not needed here.
> +#include <c-ctype.h>
Prefer the "..." form for it (comes with gnulib).
> +#include "rl.h"
> +
> +static char program_name[] = "fish";
This is provided by including guestfs-internal-frontend.h, and you'll
need guestfs.h prior including the former; so:
#include "guestfs.h"
#include "guestfs-internal-frontend.h"
> +int
> +hexdigit (char d)
> +{
> +  switch (d) {
> +  case '0'...'9': return d - '0';
> +  case 'a'...'f': return d - 'a' + 10;
> +  case 'A'...'F': return d - 'A' + 10;
> +  default: return -1;
> +  }
> +}
> +
> +// backslash unescape for readline
Prefer comments in the /* ... */ format, so:
/* Backslash unescape for readline. */
> +char *
> +bs_unescape_filename (char *str)
If "str" is not changed, just make it const, so callers know that.
> +{
> +  char *p = calloc(strlen(str) + 1, 1);
> +  strcpy(p, str);
Sounds like strdup? It is POSIX.1-2007, and available also earlier;
in case not, gnulib has a strdup module.
> +      error:
> +        fprintf (stderr, ("%s: invalid escape sequence in string
> (starting at offset %d)\n"),
> +                 program_name, (int) (p - start));
The message above needs gettext, so _(...).
> +// backslash scape
Ditto (for comment style).
> +char *
> +bs_escape_filename (char *p)
Ditto (for constness of parameter).
> +{
> +  char *start = p;
> +  // four times original length - if all chars are unprintable
> +  // new string would be \xXY\xWZ
Ditto (for comment style).
> +  char *n = calloc(strlen(p) * 4 + 1, 1);
I'm not sure calloc'ing the whole buffer is needed here; just create
it as usual with malloc, write on it, and make sure to set the last
character as '\0'.
> +  char *nstart = n;
> +  if (strlen(p) == 0) {
> +    n[0] = '\0';
> +    return n;
> +  }
This optimization for empty strings would be better to be done before
allocating the resulting buffer, something like:
  if (p[0] == '\0')
    return strdup ("");
> +      switch (*p) {
> +      case '\\': break;
Shouldn't \ be escaped as well?
> +      case '\a': *(n++) = '\\'; *n = 'a'; break;
> +      case '\b': *(n++) = '\\'; *n = 'b'; break;
> +      case '\f': *(n++) = '\\'; *n = 'f'; break;
> +      case '\n': *(n++) = '\\'; *n = 'n'; break;
> +      case '\r': *(n++) = '\\'; *n = 'r'; break;
> +      case '\t': *(n++) = '\\'; *n = 't'; break;
> +      case '\v': *(n++) = '\\'; *n = 'v'; break;
> +      case '"':  *(n++) = '\\'; *n =
'"'; break;
> +      case '\'': *(n++) = '\\'; *n = '\'';
break;
> +      case '?':  *(n++) = '\\'; *n = '?'; break;
> +      case ' ':  *(n++) = '\\'; *n = ' '; break;
> +
> +      default:
> +        // Hexadecimal escape unprintable character. This violates
identity
> +        // after composition of bsquote_filename after debsquote_filename
> +        // (i.e. can escape some characters differently).
Ditto (for comment style).
Also, the names mentioned here don't seem the one used for the functions.
> +        if (!c_isprint(*p)) {
> +          n += sprintf(n, "\\x%x", (int) (*p & 0xff)) - 1;
Check the return of sprintf here.
> +        } else {
> +          *n = *p;
> +        }
> +        break;
> +      error:
> +        fprintf (stderr, ("%s: invalid escape sequence in string
> (starting at offset %d)\n"),
> +                 program_name, (int) (p - start));
Ditto (for gettext).
> +  nstart = realloc(nstart, strlen(nstart));
This is done to shrink the nstart buffer? I don't think it's needed,
especially if it is properly '\0'-ended.
> +#ifndef _RL_H
This include guard seems a bit generic, please use something a bit more
specific like FISH_RL_H.
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
There is no C++ code in libguestfs, so this is not needed.

-- 
Pino Toscano

On Friday 31 October 2014 18:18:15 mzatko@redhat.com
wrote:
> +#testquoting_LDADD = $(top_builddir)/fish/rl.o
Make sure to not leave commented stuff around.
> +# testquoting_CFLAGS = -I$(top_srcdir)
Ditto.
> diff --git a/fish/test/testquoting.c b/fish/test/testquoting.c
> new file mode 100644
> index 0000000..b0b525b
> --- /dev/null
> +++ b/fish/test/testquoting.c
> @@ -0,0 +1,120 @@
> +#include <config.h>
Missing copyright/license block here.
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <memory.h>
memory.h is not needed here.
> +#include <guestfs.h>
> +#include <guestfs-internal-frontend.h>
> +#include <rl.h>
Prefer the "..." form for these.
> +int
> +eq_bracket (char *(*fn)(char*), char * in, char * out)
> +{
> +  char * q = fn(in);
> +  return (q != NULL) && STREQ(q, out);
While it's just a test, make sure to free 'q' here; this eases
debugging with valgrind for memory leaks in the non-test code.

Also 'out' can be const, since it is passed as const buffer.
> +int
> +test_empty_escape (void)
> +{
> +  return eq_bracket(bs_escape_filename, "", "");
> +}
> +
> +int
> +test_empty_unescape (void)
> +{
> +  return eq_bracket(bs_unescape_filename, "", "");
> +}
> +
> +int
> +test_singlespace_escape (void)
> +{
> +  return eq_bracket(bs_escape_filename, " ", "\\ ");
> +}
> +
> +int
> +test_singlespace_unescape (void)
> +{
> +  return eq_bracket(bs_unescape_filename, "\\ ", " ");
> +}
> +
> +int
> +test_singleword_escape (void)
> +{
> +  return eq_bracket(bs_escape_filename, "singleword",
"singleword");
> +}
> +
> +int
> +test_singleword_unescape (void)
> +{
> +  return eq_bracket(bs_unescape_filename, "singleword",
> "singleword"); +}
> +
> +int
> +test_multiword_escape (void)
> +{
> +  return eq_bracket(bs_escape_filename, "more than one word\n",
> "more\\ than\\ one\\ word\\n"); +}
> +
> +int
> +test_nonprinting_escape (void)
> +{
> +  return eq_bracket(bs_escape_filename, "\xac\xec\x8",
> "\\xac\\xec\\b"); +}
> +
> +int
> +test_multiword_unescape (void)
> +{
> +  return eq_bracket(bs_unescape_filename, "more\\ than\\ one\\
word",
> "more than one word"); +}
> +
> +int
> +test_nonprinting_unescape (void)
> +{
> +  return eq_bracket(bs_unescape_filename, "\\xac\\xec\\b",
> "\xac\xec\b"); +}
> +
> +
> +
> +struct test_t {
> +  char *name;
> +  int (*fn)(void);
> +  int expect;
> +};
Hmm there's lot of repetition in the code above; what about providing
the tests in a structure? Something like:

struct string_test_data {
  const char *in;
  const char *out;
  int pass;
}

struct string_test_data escape_tests[] = {
  { "", "", 1 },
  { " ", "\\ ", 1 },
  ...
}
size_t nr_escape_tests = sizeof(escape_tests) / sizeof(*escape_tests);

(ditto for unescape tests)

then do two iterations, for escape_tests and unescape_tests.
> +int
> +main (int argc, char *argv[])
> +{
> +  int nr_failed = 0; // failed test count
> +  setbuf(stdout, NULL);
No need to explicitly unbuffer stdout.
> +  for (int i = 0; i < nr_tests; i++) {
> +    fprintf(stdout, "%s: ", tests[i].name);
> +    int r = tests[i].fn() == tests[i].expect;
> +    fprintf(stdout, "%s\n", r ? "PASS" :
"FAIL");
> +    nr_failed += !r;
Just add nr_failed by 1, so the return value of the comparison can
change without affecting the count of failed tests.
-- 
Pino Toscano