On 12/03/2013 01:18 AM, Pádraig Brady wrote:
> On 12/02/2013 11:41 PM, Bernhard Voelker wrote:
>> devmsg ("format String:\n input: %s\n grouping: %s\n"
>> " padding width: %ld\n alignment: %s\n"
>> - " prefix: '%s'\n suffix: '%s'\n",
>> + " prefix: %s\n suffix: %s\n",
>> quote (fmt), (grouping) ? "yes" : "no",
>
> does this need to be quote_n (0,... ?
>
>> padding_width,
>> (padding_alignment == MBS_ALIGN_LEFT) ? "Left" : "Right",
>> - format_str_prefix, format_str_suffix);
>> + quote_n (0, format_str_prefix), quote_n (1, format_str_suffix));
>
> and this adjusted accordingly
>
>> @@ -1067,8 +1068,8 @@ parse_human_number (const char *str, long double
>> /*output */ *value,
>> if (ptr && *ptr != '\0')
>> {
>> if (_invalid != inval_ignore)
>> - error (conv_exit_code, 0, _("invalid suffix in input '%s': '%s'"),
>> - str, ptr);
>> + error (conv_exit_code, 0, _("invalid suffix in input %s: %s"),
>> + quote (str), quote (ptr));
>
> do these need to be quote_n?
ugh, Mondays, sorry.
The attached fixes it.
I also changed the quoting in 'extract_fields' by converting possible
NULL pointers into a proper "" instead of crippling the format string
to bypass the new rule:
-@@ -1271,7 +1275,9 @@ extract_fields (char *line, int _field,
+@@ -1271,8 +1276,10 @@ extract_fields (char *line, int _field,
else
*_suffix = NULL;
- devmsg (" prefix: '%s'\n number: '%s'\n suffix: '%s'\n",
-+ /* Although the quoting notation '%s' violates sc_prohibit_quotes_notation,
-+ avoid using quote() here as e.g. *_prefix may be NULL. */
-+ devmsg (" prefix: '%s""'\n number: '%s""'\n suffix: '%s""'\n",
- *_prefix, *_data, *_suffix);
+- *_prefix, *_data, *_suffix);
++ devmsg (" prefix: %s\n number: %s\n suffix: %s\n",
++ quote_n (0, *_prefix ? *_prefix : ""),
++ quote_n (1, *_data),
++ quote_n (2, *_suffix ? *_suffix : ""));
}
Thanks for the review!
Have a nice day,
Berny
>From 6fb555628a70ac46809bb1b6e5e8286266649de4 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <[email protected]>
Date: Tue, 3 Dec 2013 08:02:57 +0100
Subject: [PATCH] maint: avoid '%s' quoting notation in diagnostic messages
Add a new rule to ensure the use of quote() instead of '%s' or `%s'
in format strings of diagnostics messages.
* cfg.mk (sc_prohibit_quotes_notation): Add rule.
* TODO: Remove the entry regarding the '%s' notation.
* src/mkfifo.c (main): Remove the offending and in this case even
duplicate quoting in the format string of the error diagnostic.
* src/mknod.c (main): Likewise.
* src/df.c (decode_output_arg): Change two invocations of error()
according to the above new rule.
* src/numfmt.c: Fix numerous wrong quote notations to fit the above
new rule, mostly in internal debugging diagnostic messages.
---
TODO | 4 ----
cfg.mk | 8 ++++++++
src/df.c | 6 +++---
src/mkfifo.c | 2 +-
src/mknod.c | 2 +-
src/numfmt.c | 63 ++++++++++++++++++++++++++++++++++--------------------------
6 files changed, 49 insertions(+), 36 deletions(-)
diff --git a/TODO b/TODO
index e10da7c..3dc05af 100644
--- a/TODO
+++ b/TODO
@@ -130,10 +130,6 @@ Add a distcheck-time test to ensure that every distributed
file is either read-only(indicating generated) or is
version-controlled and up to date.
-remove '%s' notation (now that they're all gone, add a maint.mk sc_
- rule to ensure no new ones are added):
- grep -E "\`%.{,4}s'" src/*.c
-
remove all uses of the 'register' keyword: Done. add a maint.mk rule
for this, too.
diff --git a/cfg.mk b/cfg.mk
index 788c351..9e8e8ac 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -151,6 +151,14 @@ sc_system_h_headers: .re-list
1>&2; exit 1; } || :; \
fi
+# Files in src/ should not use '%s' notation in format strings,
+# i.e., single quotes around %s (or similar) should be avoided.
+sc_prohibit_quotes_notation:
+ @cd $(srcdir)/src && GIT_PAGER= git grep -n "\".*[\`']%s'.*\"" *.c \
+ && { echo '$(ME): '"Use quote() to avoid quoted '%s' notation" 1>&2; \
+ exit 1; } \
+ || :
+
sc_sun_os_names:
@grep -nEi \
'solaris[^[:alnum:]]*2\.(7|8|9|[1-9][0-9])|sunos[^[:alnum:]][6-9]' \
diff --git a/src/df.c b/src/df.c
index f6ce79d..ba8ef15 100644
--- a/src/df.c
+++ b/src/df.c
@@ -383,15 +383,15 @@ decode_output_arg (char const *arg)
}
if (field == -1)
{
- error (0, 0, _("option --output: field '%s' unknown"), s);
+ error (0, 0, _("option --output: field %s unknown"), quote (s));
usage (EXIT_FAILURE);
}
if (field_data[field].used)
{
/* Prevent the fields from being used more than once. */
- error (0, 0, _("option --output: field '%s' used more than once"),
- field_data[field].arg);
+ error (0, 0, _("option --output: field %s used more than once"),
+ quote (field_data[field].arg));
usage (EXIT_FAILURE);
}
diff --git a/src/mkfifo.c b/src/mkfifo.c
index df97d6b..2428dd0 100644
--- a/src/mkfifo.c
+++ b/src/mkfifo.c
@@ -170,7 +170,7 @@ main (int argc, char **argv)
}
else if (specified_mode && lchmod (argv[optind], newmode) != 0)
{
- error (0, errno, _("cannot set permissions of `%s'"),
+ error (0, errno, _("cannot set permissions of %s"),
quote (argv[optind]));
exit_status = EXIT_FAILURE;
}
diff --git a/src/mknod.c b/src/mknod.c
index 908d840..7856e51 100644
--- a/src/mknod.c
+++ b/src/mknod.c
@@ -265,7 +265,7 @@ main (int argc, char **argv)
}
if (specified_mode && lchmod (argv[optind], newmode) != 0)
- error (EXIT_FAILURE, errno, _("cannot set permissions of `%s'"),
+ error (EXIT_FAILURE, errno, _("cannot set permissions of %s"),
quote (argv[optind]));
exit (EXIT_SUCCESS);
diff --git a/src/numfmt.c b/src/numfmt.c
index a809949..339c79f 100644
--- a/src/numfmt.c
+++ b/src/numfmt.c
@@ -598,8 +598,9 @@ simple_strtod_human (const char *input_str,
/* 'scale_auto' is checked below. */
int scale_base = default_scale_base (allowed_scaling);
- devmsg ("simple_strtod_human:\n input string: '%s'\n "
- "locale decimal-point: '%s'\n", input_str, decimal_point);
+ devmsg ("simple_strtod_human:\n input string: %s\n "
+ "locale decimal-point: %s\n",
+ quote_n (0, input_str), quote_n (1, decimal_point));
enum simple_strtod_error e =
simple_strtod_float (input_str, endptr, value, precision);
@@ -673,29 +674,29 @@ simple_strtod_fatal (enum simple_strtod_error err, char const *input_str)
abort ();
case SSE_OVERFLOW:
- msgid = N_("value too large to be converted: '%s'");
+ msgid = N_("value too large to be converted: %s");
break;
case SSE_INVALID_NUMBER:
- msgid = N_("invalid number: '%s'");
+ msgid = N_("invalid number: %s");
break;
case SSE_VALID_BUT_FORBIDDEN_SUFFIX:
- msgid = N_("rejecting suffix in input: '%s' (consider using --from)");
+ msgid = N_("rejecting suffix in input: %s (consider using --from)");
break;
case SSE_INVALID_SUFFIX:
- msgid = N_("invalid suffix in input: '%s'");
+ msgid = N_("invalid suffix in input: %s");
break;
case SSE_MISSING_I_SUFFIX:
- msgid = N_("missing 'i' suffix in input: '%s' (e.g Ki/Mi/Gi)");
+ msgid = N_("missing 'i' suffix in input: %s (e.g Ki/Mi/Gi)");
break;
}
if (_invalid != inval_ignore)
- error (conv_exit_code, 0, gettext (msgid), input_str);
+ error (conv_exit_code, 0, gettext (msgid), quote (input_str));
}
/* Convert VAL to a human format string in BUF. */
@@ -766,7 +767,7 @@ double_to_human (long double val, int precision,
if (scale == scale_IEC_I && power > 0)
strncat (buf, "i", buf_size - strlen (buf) - 1);
- devmsg (" returning value: '%s'\n", buf);
+ devmsg (" returning value: %s\n", quote (buf));
return;
}
@@ -784,7 +785,7 @@ unit_to_umax (const char *n_string)
s_err = xstrtoumax (n_string, &end, 10, &n, "KMGTPEZY");
if (s_err != LONGINT_OK || *end || n == 0)
- error (EXIT_FAILURE, 0, _("invalid unit size: '%s'"), n_string);
+ error (EXIT_FAILURE, 0, _("invalid unit size: %s"), quote (n_string));
return n;
}
@@ -1035,11 +1036,12 @@ parse_format_string (char const *fmt)
devmsg ("format String:\n input: %s\n grouping: %s\n"
" padding width: %ld\n alignment: %s\n"
- " prefix: '%s'\n suffix: '%s'\n",
- quote (fmt), (grouping) ? "yes" : "no",
+ " prefix: %s\n suffix: %s\n",
+ quote_n (0, fmt), (grouping) ? "yes" : "no",
padding_width,
(padding_alignment == MBS_ALIGN_LEFT) ? "Left" : "Right",
- format_str_prefix, format_str_suffix);
+ quote_n (1, format_str_prefix ? format_str_prefix : ""),
+ quote_n (2, format_str_suffix ? format_str_suffix : ""));
}
/* Parse a numeric value (with optional suffix) from a string.
@@ -1067,8 +1069,8 @@ parse_human_number (const char *str, long double /*output */ *value,
if (ptr && *ptr != '\0')
{
if (_invalid != inval_ignore)
- error (conv_exit_code, 0, _("invalid suffix in input '%s': '%s'"),
- str, ptr);
+ error (conv_exit_code, 0, _("invalid suffix in input %s: %s"),
+ quote_n (0, str), quote_n (1, ptr));
e = SSE_INVALID_SUFFIX;
}
return e;
@@ -1107,7 +1109,8 @@ prepare_padded_number (const long double val, size_t precision)
if (suffix)
strncat (buf, suffix, sizeof (buf) - strlen (buf) -1);
- devmsg ("formatting output:\n value: %Lf\n humanized: '%s'\n", val, buf);
+ devmsg ("formatting output:\n value: %Lf\n humanized: %s\n",
+ val, quote (buf));
if (padding_width && strlen (buf) < padding_width)
{
@@ -1115,7 +1118,7 @@ prepare_padded_number (const long double val, size_t precision)
mbsalign (buf, padding_buffer, padding_buffer_size, &w,
padding_alignment, MBA_UNIBYTE_ONLY);
- devmsg (" After padding: '%s'\n", padding_buffer);
+ devmsg (" After padding: %s\n", quote (padding_buffer));
}
else
{
@@ -1151,7 +1154,7 @@ process_suffixed_number (char *text, long double *result, size_t *precision)
{
/* trim suffix, ONLY if it's at the end of the text. */
*possible_suffix = '\0';
- devmsg ("trimming suffix '%s'\n", suffix);
+ devmsg ("trimming suffix %s\n", quote (suffix));
}
else
devmsg ("no valid suffix found\n");
@@ -1181,7 +1184,8 @@ process_suffixed_number (char *text, long double *result, size_t *precision)
long double val = 0;
enum simple_strtod_error e = parse_human_number (p, &val, precision);
if (e == SSE_OK_PRECISION_LOSS && debug)
- error (0, 0, _("large input value '%s': possible precision loss"), p);
+ error (0, 0, _("large input value %s: possible precision loss"),
+ quote (p));
if (from_unit_size != 1 || to_unit_size != 1)
val = (val * from_unit_size) / to_unit_size;
@@ -1241,7 +1245,8 @@ extract_fields (char *line, int _field,
*_data = NULL;
*_suffix = NULL;
- devmsg ("extracting Fields:\n input: '%s'\n field: %d\n", line, _field);
+ devmsg ("extracting Fields:\n input: %s\n field: %d\n",
+ quote (line), _field);
if (field > 1)
{
@@ -1251,7 +1256,7 @@ extract_fields (char *line, int _field,
if (*ptr == '\0')
{
/* not enough fields in the input - print warning? */
- devmsg (" TOO FEW FIELDS!\n prefix: '%s'\n", *_prefix);
+ devmsg (" TOO FEW FIELDS!\n prefix: %s\n", quote (*_prefix));
return;
}
@@ -1271,8 +1276,10 @@ extract_fields (char *line, int _field,
else
*_suffix = NULL;
- devmsg (" prefix: '%s'\n number: '%s'\n suffix: '%s'\n",
- *_prefix, *_data, *_suffix);
+ devmsg (" prefix: %s\n number: %s\n suffix: %s\n",
+ quote_n (0, *_prefix ? *_prefix : ""),
+ quote_n (1, *_data),
+ quote_n (2, *_suffix ? *_suffix : ""));
}
@@ -1384,7 +1391,8 @@ main (int argc, char **argv)
case PADDING_OPTION:
if (xstrtol (optarg, NULL, 10, &padding_width, "") != LONGINT_OK
|| padding_width == 0)
- error (EXIT_FAILURE, 0, _("invalid padding value '%s'"), optarg);
+ error (EXIT_FAILURE, 0, _("invalid padding value %s"),
+ quote (optarg));
if (padding_width < 0)
{
padding_alignment = MBS_ALIGN_LEFT;
@@ -1397,7 +1405,8 @@ main (int argc, char **argv)
case FIELD_OPTION:
if (xstrtol (optarg, NULL, 10, &field, "") != LONGINT_OK
|| field <= 0)
- error (EXIT_FAILURE, 0, _("invalid field value '%s'"), optarg);
+ error (EXIT_FAILURE, 0, _("invalid field value %s"),
+ quote (optarg));
break;
case 'd':
@@ -1426,8 +1435,8 @@ main (int argc, char **argv)
{
if (xstrtoumax (optarg, NULL, 10, &header, "") != LONGINT_OK
|| header == 0)
- error (EXIT_FAILURE, 0, _("invalid header value '%s'"),
- optarg);
+ error (EXIT_FAILURE, 0, _("invalid header value %s"),
+ quote (optarg));
}
else
{
--
1.8.3.1
