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

[Pino Toscano]

On Friday 31 October 2014 18:18:14 mzatko at 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