* [PATCH] Make grub_strtoul() "end" pointer have the right const params.
@ 2020-02-04 21:02 Peter Jones
2020-02-05 1:04 ` Nicholas Vinson
0 siblings, 1 reply; 7+ messages in thread
From: Peter Jones @ 2020-02-04 21:02 UTC (permalink / raw)
To: grub-devel; +Cc: Daniel Kiper, Peter Jones
grub_strtoul() and grub_strtoull() don't make the /pointer/ to "end" be
const like normal implementations do, and as a result, at many places in
the tree, people appear to have abandoned trying to figure out C's
syntax and instead of fixing it, chosen to cast a mutable pointer to an
immutable array (i.e. const char *) to (char **) in order to get *end
out correctly, or similar hacks. This confusion stems from C's syntax
here, which (quoting from C99) is:
declarator:
pointer[opt] direct-declarator
direct-declarator:
identifier
( declarator )
direct-declarator [ type-qualifier-list[opt] assignment-expression[opt] ]
...
direct-declarator [ type-qualifier-list[opt] * ]
...
pointer:
* type-qualifier-list[opt]
* type-qualifier-list[opt] pointer
type-qualifier-list:
type-qualifier
type-qualifier-list type-qualifier
...
type-qualifier:
const
restrict
volatile
So the examples go like:
const char foo; // immutable object
const char *foo; // mutable pointer to object
char * const foo; // immutable pointer to mutable object
const char * const foo; // immutable pointer to immutable object
const char const * const foo; // XXX extra const keyword in the middle
const char * const * const foo; // immutable pointer to immutable
// pointer to immutable object
const char ** const foo; // immutable pointer to mutable pointer
// to immutable object
Making const left-associative for * and right-associative for everything
else may not have been the best choice ever, but here we are, so the
last of those is actually the behavior we want from strtoul()'s "end"
pointer.
This commit fixes the types for grub_strtoul() and grub_strtoull(), as
well as fixing all of the types in all of the callers, and removing all
the type-casting which itself /removes/ const on the inputs.
In one case, we have constructions that pass in a non-const pointer and
the object it points to needs to be cast to const because gcc got
moderately overzealous with its warnings:
int n;
char *ptr, *next;
for (ptr = foo; *ptr; )
{
...
- n = grub_strtoull(ptr, &ptr, ...);
+ n = grub_strtoull(ptr, (const char **)&ptr, ...);
...
}
I have tested that all of this compiles without relevant warnings with
gcc 9 using 'gcc -W -Wall -Wextra' (and that /much/ of it behaves
correctly), with the exception of grub-core/osdep/aros/hostdisk.c ,
which I have no idea how to build.
Signed-off-by: Peter Jones <pjones@redhat.com>
---
grub-core/commands/date.c | 3 ++-
grub-core/commands/i386/cmostest.c | 2 +-
grub-core/commands/i386/pc/play.c | 2 +-
grub-core/commands/i386/rdmsr.c | 2 +-
grub-core/commands/i386/wrmsr.c | 2 +-
grub-core/commands/password_pbkdf2.c | 2 +-
grub-core/commands/pcidump.c | 13 ++++++-------
grub-core/commands/regexp.c | 2 +-
grub-core/commands/setpci.c | 21 ++++++++++-----------
grub-core/commands/test.c | 2 +-
grub-core/commands/videoinfo.c | 2 +-
grub-core/disk/diskfilter.c | 3 ++-
grub-core/disk/lvm.c | 9 +++++----
grub-core/efiemu/pnvram.c | 5 +++--
| 2 +-
| 2 +-
grub-core/kern/fs.c | 2 +-
grub-core/kern/misc.c | 8 ++++----
grub-core/kern/partition.c | 2 +-
grub-core/lib/arg.c | 2 +-
grub-core/lib/json/json.c | 4 ++--
grub-core/lib/legacy_parse.c | 2 +-
grub-core/lib/syslinux_parse.c | 6 +++---
grub-core/loader/i386/bsd.c | 6 +++---
grub-core/loader/i386/linux.c | 2 +-
grub-core/loader/i386/pc/linux.c | 2 +-
grub-core/loader/i386/xen_fileXX.c | 2 +-
grub-core/mmap/mmap.c | 4 ++--
grub-core/net/http.c | 4 ++--
grub-core/net/net.c | 8 ++++----
| 3 +--
grub-core/osdep/aros/hostdisk.c | 2 +-
grub-core/osdep/devmapper/hostdisk.c | 2 +-
grub-core/script/execute.c | 6 +++---
grub-core/term/serial.c | 2 +-
grub-core/term/terminfo.c | 2 +-
grub-core/tests/strtoull_test.c | 2 +-
include/grub/misc.h | 6 +++---
util/grub-fstest.c | 2 +-
39 files changed, 78 insertions(+), 77 deletions(-)
diff --git a/grub-core/commands/date.c b/grub-core/commands/date.c
index 8e1f41f141b..5cb4fafd454 100644
--- a/grub-core/commands/date.c
+++ b/grub-core/commands/date.c
@@ -59,7 +59,8 @@ grub_cmd_date (grub_command_t cmd __attribute__ ((unused)),
for (; argc; argc--, args++)
{
- char *p, c;
+ const char *p;
+ char c;
int m1, ofs, n, cur_mask;
p = args[0];
diff --git a/grub-core/commands/i386/cmostest.c b/grub-core/commands/i386/cmostest.c
index c839b704dc5..9f6b56a2f0c 100644
--- a/grub-core/commands/i386/cmostest.c
+++ b/grub-core/commands/i386/cmostest.c
@@ -27,7 +27,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
static grub_err_t
parse_args (int argc, char *argv[], int *byte, int *bit)
{
- char *rest;
+ const char *rest;
if (argc != 1)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "address required");
diff --git a/grub-core/commands/i386/pc/play.c b/grub-core/commands/i386/pc/play.c
index c8181310515..a980e46883c 100644
--- a/grub-core/commands/i386/pc/play.c
+++ b/grub-core/commands/i386/pc/play.c
@@ -132,7 +132,7 @@ grub_cmd_play (grub_command_t cmd __attribute__ ((unused)),
}
else
{
- char *end;
+ const char *end;
unsigned tempo;
struct note note;
int i;
diff --git a/grub-core/commands/i386/rdmsr.c b/grub-core/commands/i386/rdmsr.c
index 15b9adfca67..46c4346da1b 100644
--- a/grub-core/commands/i386/rdmsr.c
+++ b/grub-core/commands/i386/rdmsr.c
@@ -44,7 +44,7 @@ grub_cmd_msr_read (grub_extcmd_context_t ctxt, int argc, char **argv)
{
grub_uint32_t manufacturer[3], max_cpuid, a, b, c, features, addr;
grub_uint64_t value;
- char *ptr;
+ const char *ptr;
char buf[sizeof("1122334455667788")];
/*
diff --git a/grub-core/commands/i386/wrmsr.c b/grub-core/commands/i386/wrmsr.c
index 9c5e510eb44..fa76f5aed11 100644
--- a/grub-core/commands/i386/wrmsr.c
+++ b/grub-core/commands/i386/wrmsr.c
@@ -37,7 +37,7 @@ grub_cmd_msr_write (grub_command_t cmd __attribute__ ((unused)), int argc, char
{
grub_uint32_t manufacturer[3], max_cpuid, a, b, c, features, addr;
grub_uint64_t value;
- char *ptr;
+ const char *ptr;
/*
* The CPUID instruction should be used to determine whether MSRs
diff --git a/grub-core/commands/password_pbkdf2.c b/grub-core/commands/password_pbkdf2.c
index da636e6217a..ab845d25eb3 100644
--- a/grub-core/commands/password_pbkdf2.c
+++ b/grub-core/commands/password_pbkdf2.c
@@ -86,7 +86,7 @@ grub_cmd_password (grub_command_t cmd __attribute__ ((unused)),
int argc, char **args)
{
grub_err_t err;
- char *ptr, *ptr2;
+ const char *ptr, *ptr2;
grub_uint8_t *ptro;
struct pbkdf2_password *pass;
diff --git a/grub-core/commands/pcidump.c b/grub-core/commands/pcidump.c
index f99ad4a216f..f72628fce2d 100644
--- a/grub-core/commands/pcidump.c
+++ b/grub-core/commands/pcidump.c
@@ -95,7 +95,7 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
if (ctxt->state[0].set)
{
ptr = ctxt->state[0].arg;
- ctx.pciid_check_value |= (grub_strtoul (ptr, (char **) &ptr, 16) & 0xffff);
+ ctx.pciid_check_value |= (grub_strtoul (ptr, &ptr, 16) & 0xffff);
if (grub_errno == GRUB_ERR_BAD_NUMBER)
{
grub_errno = GRUB_ERR_NONE;
@@ -108,8 +108,7 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
if (*ptr != ':')
return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("missing `%c' symbol"), ':');
ptr++;
- ctx.pciid_check_value |= (grub_strtoul (ptr, (char **) &ptr, 16) & 0xffff)
- << 16;
+ ctx.pciid_check_value |= (grub_strtoul (ptr, &ptr, 16) & 0xffff) << 16;
if (grub_errno == GRUB_ERR_BAD_NUMBER)
grub_errno = GRUB_ERR_NONE;
else
@@ -121,10 +120,10 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
if (ctxt->state[1].set)
{
const char *optr;
-
+
ptr = ctxt->state[1].arg;
optr = ptr;
- ctx.bus = grub_strtoul (ptr, (char **) &ptr, 16);
+ ctx.bus = grub_strtoul (ptr, &ptr, 16);
if (grub_errno == GRUB_ERR_BAD_NUMBER)
{
grub_errno = GRUB_ERR_NONE;
@@ -138,7 +137,7 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("missing `%c' symbol"), ':');
ptr++;
optr = ptr;
- ctx.device = grub_strtoul (ptr, (char **) &ptr, 16);
+ ctx.device = grub_strtoul (ptr, &ptr, 16);
if (grub_errno == GRUB_ERR_BAD_NUMBER)
{
grub_errno = GRUB_ERR_NONE;
@@ -149,7 +148,7 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
if (*ptr == '.')
{
ptr++;
- ctx.function = grub_strtoul (ptr, (char **) &ptr, 16);
+ ctx.function = grub_strtoul (ptr, &ptr, 16);
if (grub_errno)
return grub_errno;
ctx.check_function = 1;
diff --git a/grub-core/commands/regexp.c b/grub-core/commands/regexp.c
index f00b184c81e..7c5c72fe460 100644
--- a/grub-core/commands/regexp.c
+++ b/grub-core/commands/regexp.c
@@ -64,7 +64,7 @@ set_matches (char **varnames, char *str, grub_size_t nmatches,
{
int i;
char *p;
- char *q;
+ const char * q;
grub_err_t err;
unsigned long j;
diff --git a/grub-core/commands/setpci.c b/grub-core/commands/setpci.c
index d5bc97d60b2..e966af080a6 100644
--- a/grub-core/commands/setpci.c
+++ b/grub-core/commands/setpci.c
@@ -169,7 +169,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
if (ctxt->state[0].set)
{
ptr = ctxt->state[0].arg;
- pciid_check_value |= (grub_strtoul (ptr, (char **) &ptr, 16) & 0xffff);
+ pciid_check_value |= (grub_strtoul (ptr, &ptr, 16) & 0xffff);
if (grub_errno == GRUB_ERR_BAD_NUMBER)
{
grub_errno = GRUB_ERR_NONE;
@@ -182,8 +182,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
if (*ptr != ':')
return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("missing `%c' symbol"), ':');
ptr++;
- pciid_check_value |= (grub_strtoul (ptr, (char **) &ptr, 16) & 0xffff)
- << 16;
+ pciid_check_value |= (grub_strtoul (ptr, &ptr, 16) & 0xffff) << 16;
if (grub_errno == GRUB_ERR_BAD_NUMBER)
grub_errno = GRUB_ERR_NONE;
else
@@ -197,10 +196,10 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
if (ctxt->state[1].set)
{
const char *optr;
-
+
ptr = ctxt->state[1].arg;
optr = ptr;
- bus = grub_strtoul (ptr, (char **) &ptr, 16);
+ bus = grub_strtoul (ptr, &ptr, 16);
if (grub_errno == GRUB_ERR_BAD_NUMBER)
{
grub_errno = GRUB_ERR_NONE;
@@ -214,7 +213,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("missing `%c' symbol"), ':');
ptr++;
optr = ptr;
- device = grub_strtoul (ptr, (char **) &ptr, 16);
+ device = grub_strtoul (ptr, &ptr, 16);
if (grub_errno == GRUB_ERR_BAD_NUMBER)
{
grub_errno = GRUB_ERR_NONE;
@@ -225,7 +224,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
if (*ptr == '.')
{
ptr++;
- function = grub_strtoul (ptr, (char **) &ptr, 16);
+ function = grub_strtoul (ptr, &ptr, 16);
if (grub_errno)
return grub_errno;
check_function = 1;
@@ -253,7 +252,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
if (i == ARRAY_SIZE (pci_registers))
{
regsize = 0;
- regaddr = grub_strtoul (ptr, (char **) &ptr, 16);
+ regaddr = grub_strtoul (ptr, &ptr, 16);
if (grub_errno)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "unknown register");
}
@@ -270,7 +269,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
if (*ptr == '+')
{
ptr++;
- regaddr += grub_strtoul (ptr, (char **) &ptr, 16);
+ regaddr += grub_strtoul (ptr, &ptr, 16);
if (grub_errno)
return grub_errno;
}
@@ -302,14 +301,14 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
if (*ptr == '=')
{
ptr++;
- regwrite = grub_strtoul (ptr, (char **) &ptr, 16);
+ regwrite = grub_strtoul (ptr, &ptr, 16);
if (grub_errno)
return grub_errno;
write_mask = 0xffffffff;
if (*ptr == ':')
{
ptr++;
- write_mask = grub_strtoul (ptr, (char **) &ptr, 16);
+ write_mask = grub_strtoul (ptr, &ptr, 16);
if (grub_errno)
return grub_errno;
write_mask = 0xffffffff;
diff --git a/grub-core/commands/test.c b/grub-core/commands/test.c
index 4e929e0452e..62d3fb3988f 100644
--- a/grub-core/commands/test.c
+++ b/grub-core/commands/test.c
@@ -31,7 +31,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
/* A simple implementation for signed numbers. */
static int
-grub_strtosl (char *arg, char **end, int base)
+grub_strtosl (char *arg, const char ** const end, int base)
{
if (arg[0] == '-')
return -grub_strtoul (arg + 1, end, base);
diff --git a/grub-core/commands/videoinfo.c b/grub-core/commands/videoinfo.c
index 4be8107d553..016a4d81835 100644
--- a/grub-core/commands/videoinfo.c
+++ b/grub-core/commands/videoinfo.c
@@ -136,7 +136,7 @@ grub_cmd_videoinfo (grub_command_t cmd __attribute__ ((unused)),
ctx.height = ctx.width = ctx.depth = 0;
if (argc)
{
- char *ptr;
+ const char *ptr;
ptr = args[0];
ctx.width = grub_strtoul (ptr, &ptr, 0);
if (grub_errno)
diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
index c3b578acf25..67bf37a9c3c 100644
--- a/grub-core/disk/diskfilter.c
+++ b/grub-core/disk/diskfilter.c
@@ -969,7 +969,8 @@ grub_diskfilter_vg_register (struct grub_diskfilter_vg *vg)
for (p = vgp->lvs; p; p = p->next)
{
int cur_num;
- char *num, *end;
+ char *num;
+ const char *end;
if (!p->fullname)
continue;
if (grub_strncmp (p->fullname, lv->fullname, len) != 0)
diff --git a/grub-core/disk/lvm.c b/grub-core/disk/lvm.c
index 7b265c780c3..0cbd0dd1629 100644
--- a/grub-core/disk/lvm.c
+++ b/grub-core/disk/lvm.c
@@ -38,7 +38,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
at the number. In case STR is not found, *P will be NULL and the
return value will be 0. */
static grub_uint64_t
-grub_lvm_getvalue (char **p, const char *str)
+grub_lvm_getvalue (const char ** const p, const char *str)
{
*p = grub_strstr (*p, str);
if (! *p)
@@ -63,12 +63,12 @@ grub_lvm_checkvalue (char **p, char *str, char *tmpl)
#endif
static int
-grub_lvm_check_flag (char *p, const char *str, const char *flag)
+grub_lvm_check_flag (const char *p, const char *str, const char *flag)
{
grub_size_t len_str = grub_strlen (str), len_flag = grub_strlen (flag);
while (1)
{
- char *q;
+ const char *q;
p = grub_strstr (p, str);
if (! p)
return 0;
@@ -105,7 +105,8 @@ grub_lvm_detect (grub_disk_t disk,
char buf[GRUB_LVM_LABEL_SIZE];
char vg_id[GRUB_LVM_ID_STRLEN+1];
char pv_id[GRUB_LVM_ID_STRLEN+1];
- char *metadatabuf, *p, *q, *vgname;
+ char *metadatabuf, *vgname;
+ const char *p, *q;
struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
struct grub_lvm_pv_header *pvh;
struct grub_lvm_disk_locn *dlocn;
diff --git a/grub-core/efiemu/pnvram.c b/grub-core/efiemu/pnvram.c
index c5c3d4bd3d5..dd42bc69116 100644
--- a/grub-core/efiemu/pnvram.c
+++ b/grub-core/efiemu/pnvram.c
@@ -39,7 +39,7 @@ static grub_size_t nvramsize;
/* Parse signed value */
static int
-grub_strtosl (const char *arg, char **end, int base)
+grub_strtosl (const char *arg, const char ** const end, int base)
{
if (arg[0] == '-')
return -grub_strtoul (arg + 1, end, base);
@@ -120,7 +120,8 @@ nvram_set (void * data __attribute__ ((unused)))
grub_memset (nvram, 0, nvramsize);
FOR_SORTED_ENV (var)
{
- char *guid, *attr, *name, *varname;
+ const char *guid;
+ char *attr, *name, *varname;
struct efi_variable *efivar;
int len = 0;
int i;
--git a/grub-core/gfxmenu/gui_circular_progress.c b/grub-core/gfxmenu/gui_circular_progress.c
index 354dd7b73ee..7578bfbec92 100644
--- a/grub-core/gfxmenu/gui_circular_progress.c
+++ b/grub-core/gfxmenu/gui_circular_progress.c
@@ -230,7 +230,7 @@ circprog_set_state (void *vself, int visible, int start,
static int
parse_angle (const char *value)
{
- char *ptr;
+ const char *ptr;
int angle;
angle = grub_strtol (value, &ptr, 10);
--git a/grub-core/gfxmenu/theme_loader.c b/grub-core/gfxmenu/theme_loader.c
index d6829bb5e90..eae83086bcc 100644
--- a/grub-core/gfxmenu/theme_loader.c
+++ b/grub-core/gfxmenu/theme_loader.c
@@ -484,7 +484,7 @@ parse_proportional_spec (const char *value, signed *abs, grub_fixed_signed_t *pr
ptr++;
}
- num = grub_strtoul (ptr, (char **) &ptr, 0);
+ num = grub_strtoul (ptr, &ptr, 0);
if (grub_errno)
return grub_errno;
if (sig)
diff --git a/grub-core/kern/fs.c b/grub-core/kern/fs.c
index 2b85f4950bd..88d39360da0 100644
--- a/grub-core/kern/fs.c
+++ b/grub-core/kern/fs.c
@@ -134,7 +134,7 @@ struct grub_fs_block
static grub_err_t
grub_fs_blocklist_open (grub_file_t file, const char *name)
{
- char *p = (char *) name;
+ const char *p = name;
unsigned num = 0;
unsigned i;
grub_disk_t disk = file->device->disk;
diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 3b633d51f4c..9d6eccc12ff 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -340,7 +340,7 @@ grub_isspace (int c)
}
unsigned long
-grub_strtoul (const char *str, char **end, int base)
+grub_strtoul (const char *str, const char ** const end, int base)
{
unsigned long long num;
@@ -357,7 +357,7 @@ grub_strtoul (const char *str, char **end, int base)
}
unsigned long long
-grub_strtoull (const char *str, char **end, int base)
+grub_strtoull (const char *str, const char ** const end, int base)
{
unsigned long long num = 0;
int found = 0;
@@ -853,14 +853,14 @@ grub_vsnprintf_real (char *str, grub_size_t max_len, const char *fmt0,
{
if (fmt[0] == '0')
zerofill = '0';
- format1 = grub_strtoul (fmt, (char **) &fmt, 10);
+ format1 = grub_strtoul (fmt, &fmt, 10);
}
if (*fmt == '.')
fmt++;
if (grub_isdigit (*fmt))
- format2 = grub_strtoul (fmt, (char **) &fmt, 10);
+ format2 = grub_strtoul (fmt, &fmt, 10);
if (*fmt == '$')
{
diff --git a/grub-core/kern/partition.c b/grub-core/kern/partition.c
index e499147cbcb..2c401b866c4 100644
--- a/grub-core/kern/partition.c
+++ b/grub-core/kern/partition.c
@@ -126,7 +126,7 @@ grub_partition_probe (struct grub_disk *disk, const char *str)
while (*ptr && grub_isalpha (*ptr))
ptr++;
partname_end = ptr;
- num = grub_strtoul (ptr, (char **) &ptr, 0) - 1;
+ num = grub_strtoul (ptr, &ptr, 0) - 1;
curpart = 0;
/* Use the first partition map type found. */
diff --git a/grub-core/lib/arg.c b/grub-core/lib/arg.c
index fd7744a6ff6..ccc185017ee 100644
--- a/grub-core/lib/arg.c
+++ b/grub-core/lib/arg.c
@@ -375,7 +375,7 @@ grub_arg_parse (grub_extcmd_t cmd, int argc, char **argv,
case ARG_TYPE_INT:
{
- char *tail;
+ const char * tail;
grub_strtoull (option, &tail, 0);
if (tail == 0 || tail == option || *tail != '\0' || grub_errno)
diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
index 412f26f17f0..15c0d99491c 100644
--- a/grub-core/lib/json/json.c
+++ b/grub-core/lib/json/json.c
@@ -227,7 +227,7 @@ grub_json_getuint64 (grub_uint64_t *out, const grub_json_t *parent, const char *
{
grub_json_type_t type;
const char *value;
- char *end;
+ const char *end;
grub_err_t ret;
ret = get_value (&type, &value, parent, key);
@@ -249,7 +249,7 @@ grub_json_getint64 (grub_int64_t *out, const grub_json_t *parent, const char *ke
{
grub_json_type_t type;
const char *value;
- char *end;
+ const char *end;
grub_err_t ret;
ret = get_value (&type, &value, parent, key);
diff --git a/grub-core/lib/legacy_parse.c b/grub-core/lib/legacy_parse.c
index ef56150ac77..05719ab2ccb 100644
--- a/grub-core/lib/legacy_parse.c
+++ b/grub-core/lib/legacy_parse.c
@@ -418,7 +418,7 @@ adjust_file (const char *in, grub_size_t len)
}
if (*comma != ',')
return grub_legacy_escape (in, len);
- part = grub_strtoull (comma + 1, (char **) &rest, 0);
+ part = grub_strtoull (comma + 1, &rest, 0);
if (rest[0] == ',' && rest[1] >= 'a' && rest[1] <= 'z')
{
subpart = rest[1] - 'a';
diff --git a/grub-core/lib/syslinux_parse.c b/grub-core/lib/syslinux_parse.c
index 4afa99279a2..de9fda06f52 100644
--- a/grub-core/lib/syslinux_parse.c
+++ b/grub-core/lib/syslinux_parse.c
@@ -1062,7 +1062,7 @@ write_entry (struct output_buffer *outbuf,
if (ptr[0] == 'h' && ptr[1] == 'd')
{
is_fd = 0;
- devn = grub_strtoul (ptr + 2, &ptr, 0);
+ devn = grub_strtoul (ptr + 2, (const char **)&ptr, 0);
continue;
}
if (grub_strncasecmp (ptr, "file=", 5) == 0)
@@ -1086,12 +1086,12 @@ write_entry (struct output_buffer *outbuf,
if (ptr[0] == 'f' && ptr[1] == 'd')
{
is_fd = 1;
- devn = grub_strtoul (ptr + 2, &ptr, 0);
+ devn = grub_strtoul (ptr + 2, (const char **)&ptr, 0);
continue;
}
if (grub_isdigit (ptr[0]))
{
- part = grub_strtoul (ptr, &ptr, 0);
+ part = grub_strtoul (ptr, (const char **)&ptr, 0);
continue;
}
/* FIXME: isolinux, ntldr, cmldr, *dos, seg, hide
diff --git a/grub-core/loader/i386/bsd.c b/grub-core/loader/i386/bsd.c
index 3730ed38247..eb82391dbea 100644
--- a/grub-core/loader/i386/bsd.c
+++ b/grub-core/loader/i386/bsd.c
@@ -1615,7 +1615,7 @@ grub_cmd_openbsd (grub_extcmd_context_t ctxt, int argc, char *argv[])
return grub_error (GRUB_ERR_BAD_ARGUMENT,
"unknown disk type name");
- unit = grub_strtoul (arg, (char **) &arg, 10);
+ unit = grub_strtoul (arg, &arg, 10);
if (! (arg && *arg >= 'a' && *arg <= 'z'))
return grub_error (GRUB_ERR_BAD_ARGUMENT,
"only device specifications of form "
@@ -1633,7 +1633,7 @@ grub_cmd_openbsd (grub_extcmd_context_t ctxt, int argc, char *argv[])
if (ctxt->state[OPENBSD_SERIAL_ARG].set)
{
struct grub_openbsd_bootarg_console serial;
- char *ptr;
+ const char *ptr;
unsigned port = 0;
unsigned speed = 9600;
@@ -1735,7 +1735,7 @@ grub_cmd_netbsd (grub_extcmd_context_t ctxt, int argc, char *argv[])
if (ctxt->state[NETBSD_SERIAL_ARG].set)
{
struct grub_netbsd_btinfo_serial serial;
- char *ptr;
+ const char *ptr;
grub_memset (&serial, 0, sizeof (serial));
grub_strcpy (serial.devname, "com");
diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
index ee95cd374b7..ac1fae72e3b 100644
--- a/grub-core/loader/i386/linux.c
+++ b/grub-core/loader/i386/linux.c
@@ -944,7 +944,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
#endif /* GRUB_MACHINE_PCBIOS */
if (grub_memcmp (argv[i], "mem=", 4) == 0)
{
- char *val = argv[i] + 4;
+ const char *val = argv[i] + 4;
linux_mem_size = grub_strtoul (val, &val, 0);
diff --git a/grub-core/loader/i386/pc/linux.c b/grub-core/loader/i386/pc/linux.c
index 47ea2945e4f..f56af7c501f 100644
--- a/grub-core/loader/i386/pc/linux.c
+++ b/grub-core/loader/i386/pc/linux.c
@@ -259,7 +259,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
}
else if (grub_memcmp (argv[i], "mem=", 4) == 0)
{
- char *val = argv[i] + 4;
+ const char *val = argv[i] + 4;
linux_mem_size = grub_strtoul (val, &val, 0);
diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c
index 6329ec01038..27afcaacbce 100644
--- a/grub-core/loader/i386/xen_fileXX.c
+++ b/grub-core/loader/i386/xen_fileXX.c
@@ -25,7 +25,7 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
grub_off_t off, grub_size_t sz)
{
char *buf;
- char *ptr;
+ const char *ptr;
int has_paddr = 0;
grub_errno = GRUB_ERR_NONE;
diff --git a/grub-core/mmap/mmap.c b/grub-core/mmap/mmap.c
index 6a31cbae325..b569cb23b5a 100644
--- a/grub-core/mmap/mmap.c
+++ b/grub-core/mmap/mmap.c
@@ -423,7 +423,7 @@ static grub_err_t
grub_cmd_badram (grub_command_t cmd __attribute__ ((unused)),
int argc, char **args)
{
- char * str;
+ const char *str;
struct badram_entry entry;
if (argc != 1)
@@ -465,7 +465,7 @@ static grub_uint64_t
parsemem (const char *str)
{
grub_uint64_t ret;
- char *ptr;
+ const char *ptr;
ret = grub_strtoul (str, &ptr, 0);
diff --git a/grub-core/net/http.c b/grub-core/net/http.c
index ec3647f9a0c..b616cf40b1e 100644
--- a/grub-core/net/http.c
+++ b/grub-core/net/http.c
@@ -107,7 +107,7 @@ parse_line (grub_file_t file, http_data_t data, char *ptr, grub_size_t len)
return GRUB_ERR_NONE;
}
ptr += sizeof ("HTTP/1.1 ") - 1;
- code = grub_strtoul (ptr, &ptr, 10);
+ code = grub_strtoul (ptr, (const char **)&ptr, 10);
if (grub_errno)
return grub_errno;
switch (code)
@@ -134,7 +134,7 @@ parse_line (grub_file_t file, http_data_t data, char *ptr, grub_size_t len)
== 0 && !data->size_recv)
{
ptr += sizeof ("Content-Length: ") - 1;
- file->size = grub_strtoull (ptr, &ptr, 10);
+ file->size = grub_strtoull (ptr, (const char **)&ptr, 10);
data->size_recv = 1;
return GRUB_ERR_NONE;
}
diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index d5d726a315e..2fe2bbecb01 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -406,7 +406,7 @@ parse_ip (const char *val, grub_uint32_t *ip, const char **rest)
for (i = 0; i < 4; i++)
{
unsigned long t;
- t = grub_strtoul (ptr, (char **) &ptr, 0);
+ t = grub_strtoul (ptr, &ptr, 0);
if (grub_errno)
{
grub_errno = GRUB_ERR_NONE;
@@ -453,7 +453,7 @@ parse_ip6 (const char *val, grub_uint64_t *ip, const char **rest)
ptr++;
continue;
}
- t = grub_strtoul (ptr, (char **) &ptr, 16);
+ t = grub_strtoul (ptr, &ptr, 16);
if (grub_errno)
{
grub_errno = GRUB_ERR_NONE;
@@ -563,7 +563,7 @@ grub_net_resolve_net_address (const char *name,
addr->type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
if (*rest == '/')
{
- addr->ipv4.masksize = grub_strtoul (rest + 1, (char **) &rest, 0);
+ addr->ipv4.masksize = grub_strtoul (rest + 1, &rest, 0);
if (!grub_errno && *rest == 0)
return GRUB_ERR_NONE;
grub_errno = GRUB_ERR_NONE;
@@ -579,7 +579,7 @@ grub_net_resolve_net_address (const char *name,
addr->type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
if (*rest == '/')
{
- addr->ipv6.masksize = grub_strtoul (rest + 1, (char **) &rest, 0);
+ addr->ipv6.masksize = grub_strtoul (rest + 1, &rest, 0);
if (!grub_errno && *rest == 0)
return GRUB_ERR_NONE;
grub_errno = GRUB_ERR_NONE;
--git a/grub-core/normal/menu.c b/grub-core/normal/menu.c
index d5e0c79a70e..da66ad89180 100644
--- a/grub-core/normal/menu.c
+++ b/grub-core/normal/menu.c
@@ -170,8 +170,7 @@ grub_menu_set_timeout (int timeout)
static int
get_and_remove_first_entry_number (const char *name)
{
- const char *val;
- char *tail;
+ const char *val, *tail;
int entry;
val = grub_env_get (name);
diff --git a/grub-core/osdep/aros/hostdisk.c b/grub-core/osdep/aros/hostdisk.c
index 2be654ca3bd..3b2c9de2496 100644
--- a/grub-core/osdep/aros/hostdisk.c
+++ b/grub-core/osdep/aros/hostdisk.c
@@ -194,7 +194,7 @@ grub_util_fd_open (const char *dev, int flg)
p1 = dev + strlen (dev);
else
{
- unit = grub_strtoul (p1 + 1, (char **) &p2, 16);
+ unit = grub_strtoul (p1 + 1, &p2, 16);
if (p2 && *p2 == '/')
flags = grub_strtoul (p2 + 1, 0, 16);
}
diff --git a/grub-core/osdep/devmapper/hostdisk.c b/grub-core/osdep/devmapper/hostdisk.c
index a697bcb4d8d..a8afc0c9408 100644
--- a/grub-core/osdep/devmapper/hostdisk.c
+++ b/grub-core/osdep/devmapper/hostdisk.c
@@ -113,7 +113,7 @@ grub_util_get_dm_node_linear_info (dev_t dev,
void *next = NULL;
uint64_t length, start;
char *target, *params;
- char *ptr;
+ const char *ptr;
int major = 0, minor = 0;
int first = 1;
grub_disk_addr_t partstart = 0;
diff --git a/grub-core/script/execute.c b/grub-core/script/execute.c
index ee299fd0ea6..8cc61ee7d78 100644
--- a/grub-core/script/execute.c
+++ b/grub-core/script/execute.c
@@ -122,7 +122,7 @@ replace_scope (struct grub_script_scope *new_scope)
grub_err_t
grub_script_break (grub_command_t cmd, int argc, char *argv[])
{
- char *p = 0;
+ const char *p = NULL;
unsigned long count;
if (argc == 0)
@@ -154,7 +154,7 @@ grub_err_t
grub_script_shift (grub_command_t cmd __attribute__((unused)),
int argc, char *argv[])
{
- char *p = 0;
+ const char *p = NULL;
unsigned long n = 0;
if (! scope)
@@ -215,7 +215,7 @@ grub_err_t
grub_script_return (grub_command_t cmd __attribute__((unused)),
int argc, char *argv[])
{
- char *p;
+ const char *p = NULL;
unsigned long n;
if (! scope || argc > 1)
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index db80b3ba0fb..f9271b09239 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -269,7 +269,7 @@ grub_cmd_serial (grub_extcmd_context_t ctxt, int argc, char **args)
if (state[OPTION_BASE_CLOCK].set)
{
- char *ptr;
+ const char *ptr;
config.base_clock = grub_strtoull (state[OPTION_BASE_CLOCK].arg, &ptr, 0);
if (grub_errno)
return grub_errno;
diff --git a/grub-core/term/terminfo.c b/grub-core/term/terminfo.c
index d317efa368d..0e9de7f8fa9 100644
--- a/grub-core/term/terminfo.c
+++ b/grub-core/term/terminfo.c
@@ -737,7 +737,7 @@ grub_cmd_terminfo (grub_extcmd_context_t ctxt, int argc, char **args)
if (state[OPTION_GEOMETRY].set)
{
- char *ptr = state[OPTION_GEOMETRY].arg;
+ const char *ptr = state[OPTION_GEOMETRY].arg;
w = grub_strtoul (ptr, &ptr, 0);
if (grub_errno)
return grub_errno;
diff --git a/grub-core/tests/strtoull_test.c b/grub-core/tests/strtoull_test.c
index 7da615ff33e..5488ab26b43 100644
--- a/grub-core/tests/strtoull_test.c
+++ b/grub-core/tests/strtoull_test.c
@@ -25,7 +25,7 @@ static void
strtoull_testcase (const char *input, int base, unsigned long long expected,
int num_digits, grub_err_t error)
{
- char *output;
+ const char *output;
unsigned long long value;
grub_errno = 0;
value = grub_strtoull(input, &output, base);
diff --git a/include/grub/misc.h b/include/grub/misc.h
index ee48eb7a726..8d8d260f8e4 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -243,11 +243,11 @@ grub_strncasecmp (const char *s1, const char *s2, grub_size_t n)
- (int) grub_tolower ((grub_uint8_t) *s2);
}
-unsigned long EXPORT_FUNC(grub_strtoul) (const char *str, char **end, int base);
-unsigned long long EXPORT_FUNC(grub_strtoull) (const char *str, char **end, int base);
+unsigned long EXPORT_FUNC(grub_strtoul) (const char *str, const char ** const end, int base);
+unsigned long long EXPORT_FUNC(grub_strtoull) (const char *str, const char ** const end, int base);
static inline long
-grub_strtol (const char *str, char **end, int base)
+grub_strtol (const char *str, const char ** const end, int base)
{
int negative = 0;
unsigned long long magnitude;
diff --git a/util/grub-fstest.c b/util/grub-fstest.c
index f14e02d9727..7f14620cd49 100644
--- a/util/grub-fstest.c
+++ b/util/grub-fstest.c
@@ -538,7 +538,7 @@ void (*argp_program_version_hook) (FILE *, struct argp_state *) = print_version;
static error_t
argp_parser (int key, char *arg, struct argp_state *state)
{
- char *p;
+ const char *p;
switch (key)
{
--
2.24.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Make grub_strtoul() "end" pointer have the right const params.
2020-02-04 21:02 [PATCH] Make grub_strtoul() "end" pointer have the right const params Peter Jones
@ 2020-02-05 1:04 ` Nicholas Vinson
2020-02-19 0:32 ` Peter Jones
0 siblings, 1 reply; 7+ messages in thread
From: Nicholas Vinson @ 2020-02-05 1:04 UTC (permalink / raw)
To: grub-devel
On 2/4/20 16:02, Peter Jones wrote:
> grub_strtoul() and grub_strtoull() don't make the /pointer/ to "end" be
> const like normal implementations do, and as a result, at many places in
grub_strtoul() and grub_strtoull() appear to be patterned after the C
standard functions strtoul() and strtoull().
According to C18, the signatures for those functions are:
unsigned long int strtoul(const char * restrict nptr,
char ** restrict endptr, int base);
unsigned long long int strtoull(const char * restrict nptr,
char ** restrict endptr, int base);
Therefore, I recommend the GRUB maintainers keep the existing signatures
for grub_strtoul() and grub_strtoull() as they more closely follow the
signatures of the standard functions.
I also skimmed through the rest of the changes and it seems to me most
of the misuse of these calls could be solved simply by doing some
variation of:
const char *ptr = ...;
char *end;
strtoul(ptr, &end, 10);
ptr = end;
which is what I would personally recommend instead. If the compiler
issues an error or warning with the above, then that's a bug the
compiler maintainers should fix.
- Nicholas Vinson
> the tree, people appear to have abandoned trying to figure out C's
> syntax and instead of fixing it, chosen to cast a mutable pointer to an
> immutable array (i.e. const char *) to (char **) in order to get *end
> out correctly, or similar hacks. This confusion stems from C's syntax
> here, which (quoting from C99) is:
>
> declarator:
> pointer[opt] direct-declarator
> direct-declarator:
> identifier
> ( declarator )
> direct-declarator [ type-qualifier-list[opt] assignment-expression[opt] ]
> ...
> direct-declarator [ type-qualifier-list[opt] * ]
> ...
> pointer:
> * type-qualifier-list[opt]
> * type-qualifier-list[opt] pointer
> type-qualifier-list:
> type-qualifier
> type-qualifier-list type-qualifier
> ...
> type-qualifier:
> const
> restrict
> volatile
>
> So the examples go like:
>
> const char foo; // immutable object
> const char *foo; // mutable pointer to object
> char * const foo; // immutable pointer to mutable object
> const char * const foo; // immutable pointer to immutable object
> const char const * const foo; // XXX extra const keyword in the middle
> const char * const * const foo; // immutable pointer to immutable
> // pointer to immutable object
> const char ** const foo; // immutable pointer to mutable pointer
> // to immutable object
>
> Making const left-associative for * and right-associative for everything
> else may not have been the best choice ever, but here we are, so the
> last of those is actually the behavior we want from strtoul()'s "end"
> pointer.
>
> This commit fixes the types for grub_strtoul() and grub_strtoull(), as
> well as fixing all of the types in all of the callers, and removing all
> the type-casting which itself /removes/ const on the inputs.
>
> In one case, we have constructions that pass in a non-const pointer and
> the object it points to needs to be cast to const because gcc got
> moderately overzealous with its warnings:
>
> int n;
> char *ptr, *next;
>
> for (ptr = foo; *ptr; )
> {
> ...
> - n = grub_strtoull(ptr, &ptr, ...);
> + n = grub_strtoull(ptr, (const char **)&ptr, ...);
> ...
> }
>
> I have tested that all of this compiles without relevant warnings with
> gcc 9 using 'gcc -W -Wall -Wextra' (and that /much/ of it behaves
> correctly), with the exception of grub-core/osdep/aros/hostdisk.c ,
> which I have no idea how to build.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> ---
> grub-core/commands/date.c | 3 ++-
> grub-core/commands/i386/cmostest.c | 2 +-
> grub-core/commands/i386/pc/play.c | 2 +-
> grub-core/commands/i386/rdmsr.c | 2 +-
> grub-core/commands/i386/wrmsr.c | 2 +-
> grub-core/commands/password_pbkdf2.c | 2 +-
> grub-core/commands/pcidump.c | 13 ++++++-------
> grub-core/commands/regexp.c | 2 +-
> grub-core/commands/setpci.c | 21 ++++++++++-----------
> grub-core/commands/test.c | 2 +-
> grub-core/commands/videoinfo.c | 2 +-
> grub-core/disk/diskfilter.c | 3 ++-
> grub-core/disk/lvm.c | 9 +++++----
> grub-core/efiemu/pnvram.c | 5 +++--
> grub-core/gfxmenu/gui_circular_progress.c | 2 +-
> grub-core/gfxmenu/theme_loader.c | 2 +-
> grub-core/kern/fs.c | 2 +-
> grub-core/kern/misc.c | 8 ++++----
> grub-core/kern/partition.c | 2 +-
> grub-core/lib/arg.c | 2 +-
> grub-core/lib/json/json.c | 4 ++--
> grub-core/lib/legacy_parse.c | 2 +-
> grub-core/lib/syslinux_parse.c | 6 +++---
> grub-core/loader/i386/bsd.c | 6 +++---
> grub-core/loader/i386/linux.c | 2 +-
> grub-core/loader/i386/pc/linux.c | 2 +-
> grub-core/loader/i386/xen_fileXX.c | 2 +-
> grub-core/mmap/mmap.c | 4 ++--
> grub-core/net/http.c | 4 ++--
> grub-core/net/net.c | 8 ++++----
> grub-core/normal/menu.c | 3 +--
> grub-core/osdep/aros/hostdisk.c | 2 +-
> grub-core/osdep/devmapper/hostdisk.c | 2 +-
> grub-core/script/execute.c | 6 +++---
> grub-core/term/serial.c | 2 +-
> grub-core/term/terminfo.c | 2 +-
> grub-core/tests/strtoull_test.c | 2 +-
> include/grub/misc.h | 6 +++---
> util/grub-fstest.c | 2 +-
> 39 files changed, 78 insertions(+), 77 deletions(-)
>
> diff --git a/grub-core/commands/date.c b/grub-core/commands/date.c
> index 8e1f41f141b..5cb4fafd454 100644
> --- a/grub-core/commands/date.c
> +++ b/grub-core/commands/date.c
> @@ -59,7 +59,8 @@ grub_cmd_date (grub_command_t cmd __attribute__ ((unused)),
>
> for (; argc; argc--, args++)
> {
> - char *p, c;
> + const char *p;
> + char c;
> int m1, ofs, n, cur_mask;
>
> p = args[0];
> diff --git a/grub-core/commands/i386/cmostest.c b/grub-core/commands/i386/cmostest.c
> index c839b704dc5..9f6b56a2f0c 100644
> --- a/grub-core/commands/i386/cmostest.c
> +++ b/grub-core/commands/i386/cmostest.c
> @@ -27,7 +27,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
> static grub_err_t
> parse_args (int argc, char *argv[], int *byte, int *bit)
> {
> - char *rest;
> + const char *rest;
>
> if (argc != 1)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "address required");
> diff --git a/grub-core/commands/i386/pc/play.c b/grub-core/commands/i386/pc/play.c
> index c8181310515..a980e46883c 100644
> --- a/grub-core/commands/i386/pc/play.c
> +++ b/grub-core/commands/i386/pc/play.c
> @@ -132,7 +132,7 @@ grub_cmd_play (grub_command_t cmd __attribute__ ((unused)),
> }
> else
> {
> - char *end;
> + const char *end;
> unsigned tempo;
> struct note note;
> int i;
> diff --git a/grub-core/commands/i386/rdmsr.c b/grub-core/commands/i386/rdmsr.c
> index 15b9adfca67..46c4346da1b 100644
> --- a/grub-core/commands/i386/rdmsr.c
> +++ b/grub-core/commands/i386/rdmsr.c
> @@ -44,7 +44,7 @@ grub_cmd_msr_read (grub_extcmd_context_t ctxt, int argc, char **argv)
> {
> grub_uint32_t manufacturer[3], max_cpuid, a, b, c, features, addr;
> grub_uint64_t value;
> - char *ptr;
> + const char *ptr;
> char buf[sizeof("1122334455667788")];
>
> /*
> diff --git a/grub-core/commands/i386/wrmsr.c b/grub-core/commands/i386/wrmsr.c
> index 9c5e510eb44..fa76f5aed11 100644
> --- a/grub-core/commands/i386/wrmsr.c
> +++ b/grub-core/commands/i386/wrmsr.c
> @@ -37,7 +37,7 @@ grub_cmd_msr_write (grub_command_t cmd __attribute__ ((unused)), int argc, char
> {
> grub_uint32_t manufacturer[3], max_cpuid, a, b, c, features, addr;
> grub_uint64_t value;
> - char *ptr;
> + const char *ptr;
>
> /*
> * The CPUID instruction should be used to determine whether MSRs
> diff --git a/grub-core/commands/password_pbkdf2.c b/grub-core/commands/password_pbkdf2.c
> index da636e6217a..ab845d25eb3 100644
> --- a/grub-core/commands/password_pbkdf2.c
> +++ b/grub-core/commands/password_pbkdf2.c
> @@ -86,7 +86,7 @@ grub_cmd_password (grub_command_t cmd __attribute__ ((unused)),
> int argc, char **args)
> {
> grub_err_t err;
> - char *ptr, *ptr2;
> + const char *ptr, *ptr2;
> grub_uint8_t *ptro;
> struct pbkdf2_password *pass;
>
> diff --git a/grub-core/commands/pcidump.c b/grub-core/commands/pcidump.c
> index f99ad4a216f..f72628fce2d 100644
> --- a/grub-core/commands/pcidump.c
> +++ b/grub-core/commands/pcidump.c
> @@ -95,7 +95,7 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
> if (ctxt->state[0].set)
> {
> ptr = ctxt->state[0].arg;
> - ctx.pciid_check_value |= (grub_strtoul (ptr, (char **) &ptr, 16) & 0xffff);
> + ctx.pciid_check_value |= (grub_strtoul (ptr, &ptr, 16) & 0xffff);
> if (grub_errno == GRUB_ERR_BAD_NUMBER)
> {
> grub_errno = GRUB_ERR_NONE;
> @@ -108,8 +108,7 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
> if (*ptr != ':')
> return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("missing `%c' symbol"), ':');
> ptr++;
> - ctx.pciid_check_value |= (grub_strtoul (ptr, (char **) &ptr, 16) & 0xffff)
> - << 16;
> + ctx.pciid_check_value |= (grub_strtoul (ptr, &ptr, 16) & 0xffff) << 16;
> if (grub_errno == GRUB_ERR_BAD_NUMBER)
> grub_errno = GRUB_ERR_NONE;
> else
> @@ -121,10 +120,10 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
> if (ctxt->state[1].set)
> {
> const char *optr;
> -
> +
> ptr = ctxt->state[1].arg;
> optr = ptr;
> - ctx.bus = grub_strtoul (ptr, (char **) &ptr, 16);
> + ctx.bus = grub_strtoul (ptr, &ptr, 16);
> if (grub_errno == GRUB_ERR_BAD_NUMBER)
> {
> grub_errno = GRUB_ERR_NONE;
> @@ -138,7 +137,7 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
> return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("missing `%c' symbol"), ':');
> ptr++;
> optr = ptr;
> - ctx.device = grub_strtoul (ptr, (char **) &ptr, 16);
> + ctx.device = grub_strtoul (ptr, &ptr, 16);
> if (grub_errno == GRUB_ERR_BAD_NUMBER)
> {
> grub_errno = GRUB_ERR_NONE;
> @@ -149,7 +148,7 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
> if (*ptr == '.')
> {
> ptr++;
> - ctx.function = grub_strtoul (ptr, (char **) &ptr, 16);
> + ctx.function = grub_strtoul (ptr, &ptr, 16);
> if (grub_errno)
> return grub_errno;
> ctx.check_function = 1;
> diff --git a/grub-core/commands/regexp.c b/grub-core/commands/regexp.c
> index f00b184c81e..7c5c72fe460 100644
> --- a/grub-core/commands/regexp.c
> +++ b/grub-core/commands/regexp.c
> @@ -64,7 +64,7 @@ set_matches (char **varnames, char *str, grub_size_t nmatches,
> {
> int i;
> char *p;
> - char *q;
> + const char * q;
> grub_err_t err;
> unsigned long j;
>
> diff --git a/grub-core/commands/setpci.c b/grub-core/commands/setpci.c
> index d5bc97d60b2..e966af080a6 100644
> --- a/grub-core/commands/setpci.c
> +++ b/grub-core/commands/setpci.c
> @@ -169,7 +169,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
> if (ctxt->state[0].set)
> {
> ptr = ctxt->state[0].arg;
> - pciid_check_value |= (grub_strtoul (ptr, (char **) &ptr, 16) & 0xffff);
> + pciid_check_value |= (grub_strtoul (ptr, &ptr, 16) & 0xffff);
> if (grub_errno == GRUB_ERR_BAD_NUMBER)
> {
> grub_errno = GRUB_ERR_NONE;
> @@ -182,8 +182,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
> if (*ptr != ':')
> return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("missing `%c' symbol"), ':');
> ptr++;
> - pciid_check_value |= (grub_strtoul (ptr, (char **) &ptr, 16) & 0xffff)
> - << 16;
> + pciid_check_value |= (grub_strtoul (ptr, &ptr, 16) & 0xffff) << 16;
> if (grub_errno == GRUB_ERR_BAD_NUMBER)
> grub_errno = GRUB_ERR_NONE;
> else
> @@ -197,10 +196,10 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
> if (ctxt->state[1].set)
> {
> const char *optr;
> -
> +
> ptr = ctxt->state[1].arg;
> optr = ptr;
> - bus = grub_strtoul (ptr, (char **) &ptr, 16);
> + bus = grub_strtoul (ptr, &ptr, 16);
> if (grub_errno == GRUB_ERR_BAD_NUMBER)
> {
> grub_errno = GRUB_ERR_NONE;
> @@ -214,7 +213,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("missing `%c' symbol"), ':');
> ptr++;
> optr = ptr;
> - device = grub_strtoul (ptr, (char **) &ptr, 16);
> + device = grub_strtoul (ptr, &ptr, 16);
> if (grub_errno == GRUB_ERR_BAD_NUMBER)
> {
> grub_errno = GRUB_ERR_NONE;
> @@ -225,7 +224,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
> if (*ptr == '.')
> {
> ptr++;
> - function = grub_strtoul (ptr, (char **) &ptr, 16);
> + function = grub_strtoul (ptr, &ptr, 16);
> if (grub_errno)
> return grub_errno;
> check_function = 1;
> @@ -253,7 +252,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
> if (i == ARRAY_SIZE (pci_registers))
> {
> regsize = 0;
> - regaddr = grub_strtoul (ptr, (char **) &ptr, 16);
> + regaddr = grub_strtoul (ptr, &ptr, 16);
> if (grub_errno)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "unknown register");
> }
> @@ -270,7 +269,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
> if (*ptr == '+')
> {
> ptr++;
> - regaddr += grub_strtoul (ptr, (char **) &ptr, 16);
> + regaddr += grub_strtoul (ptr, &ptr, 16);
> if (grub_errno)
> return grub_errno;
> }
> @@ -302,14 +301,14 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
> if (*ptr == '=')
> {
> ptr++;
> - regwrite = grub_strtoul (ptr, (char **) &ptr, 16);
> + regwrite = grub_strtoul (ptr, &ptr, 16);
> if (grub_errno)
> return grub_errno;
> write_mask = 0xffffffff;
> if (*ptr == ':')
> {
> ptr++;
> - write_mask = grub_strtoul (ptr, (char **) &ptr, 16);
> + write_mask = grub_strtoul (ptr, &ptr, 16);
> if (grub_errno)
> return grub_errno;
> write_mask = 0xffffffff;
> diff --git a/grub-core/commands/test.c b/grub-core/commands/test.c
> index 4e929e0452e..62d3fb3988f 100644
> --- a/grub-core/commands/test.c
> +++ b/grub-core/commands/test.c
> @@ -31,7 +31,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
>
> /* A simple implementation for signed numbers. */
> static int
> -grub_strtosl (char *arg, char **end, int base)
> +grub_strtosl (char *arg, const char ** const end, int base)
> {
> if (arg[0] == '-')
> return -grub_strtoul (arg + 1, end, base);
> diff --git a/grub-core/commands/videoinfo.c b/grub-core/commands/videoinfo.c
> index 4be8107d553..016a4d81835 100644
> --- a/grub-core/commands/videoinfo.c
> +++ b/grub-core/commands/videoinfo.c
> @@ -136,7 +136,7 @@ grub_cmd_videoinfo (grub_command_t cmd __attribute__ ((unused)),
> ctx.height = ctx.width = ctx.depth = 0;
> if (argc)
> {
> - char *ptr;
> + const char *ptr;
> ptr = args[0];
> ctx.width = grub_strtoul (ptr, &ptr, 0);
> if (grub_errno)
> diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
> index c3b578acf25..67bf37a9c3c 100644
> --- a/grub-core/disk/diskfilter.c
> +++ b/grub-core/disk/diskfilter.c
> @@ -969,7 +969,8 @@ grub_diskfilter_vg_register (struct grub_diskfilter_vg *vg)
> for (p = vgp->lvs; p; p = p->next)
> {
> int cur_num;
> - char *num, *end;
> + char *num;
> + const char *end;
> if (!p->fullname)
> continue;
> if (grub_strncmp (p->fullname, lv->fullname, len) != 0)
> diff --git a/grub-core/disk/lvm.c b/grub-core/disk/lvm.c
> index 7b265c780c3..0cbd0dd1629 100644
> --- a/grub-core/disk/lvm.c
> +++ b/grub-core/disk/lvm.c
> @@ -38,7 +38,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
> at the number. In case STR is not found, *P will be NULL and the
> return value will be 0. */
> static grub_uint64_t
> -grub_lvm_getvalue (char **p, const char *str)
> +grub_lvm_getvalue (const char ** const p, const char *str)
> {
> *p = grub_strstr (*p, str);
> if (! *p)
> @@ -63,12 +63,12 @@ grub_lvm_checkvalue (char **p, char *str, char *tmpl)
> #endif
>
> static int
> -grub_lvm_check_flag (char *p, const char *str, const char *flag)
> +grub_lvm_check_flag (const char *p, const char *str, const char *flag)
> {
> grub_size_t len_str = grub_strlen (str), len_flag = grub_strlen (flag);
> while (1)
> {
> - char *q;
> + const char *q;
> p = grub_strstr (p, str);
> if (! p)
> return 0;
> @@ -105,7 +105,8 @@ grub_lvm_detect (grub_disk_t disk,
> char buf[GRUB_LVM_LABEL_SIZE];
> char vg_id[GRUB_LVM_ID_STRLEN+1];
> char pv_id[GRUB_LVM_ID_STRLEN+1];
> - char *metadatabuf, *p, *q, *vgname;
> + char *metadatabuf, *vgname;
> + const char *p, *q;
> struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
> struct grub_lvm_pv_header *pvh;
> struct grub_lvm_disk_locn *dlocn;
> diff --git a/grub-core/efiemu/pnvram.c b/grub-core/efiemu/pnvram.c
> index c5c3d4bd3d5..dd42bc69116 100644
> --- a/grub-core/efiemu/pnvram.c
> +++ b/grub-core/efiemu/pnvram.c
> @@ -39,7 +39,7 @@ static grub_size_t nvramsize;
>
> /* Parse signed value */
> static int
> -grub_strtosl (const char *arg, char **end, int base)
> +grub_strtosl (const char *arg, const char ** const end, int base)
> {
> if (arg[0] == '-')
> return -grub_strtoul (arg + 1, end, base);
> @@ -120,7 +120,8 @@ nvram_set (void * data __attribute__ ((unused)))
> grub_memset (nvram, 0, nvramsize);
> FOR_SORTED_ENV (var)
> {
> - char *guid, *attr, *name, *varname;
> + const char *guid;
> + char *attr, *name, *varname;
> struct efi_variable *efivar;
> int len = 0;
> int i;
> diff --git a/grub-core/gfxmenu/gui_circular_progress.c b/grub-core/gfxmenu/gui_circular_progress.c
> index 354dd7b73ee..7578bfbec92 100644
> --- a/grub-core/gfxmenu/gui_circular_progress.c
> +++ b/grub-core/gfxmenu/gui_circular_progress.c
> @@ -230,7 +230,7 @@ circprog_set_state (void *vself, int visible, int start,
> static int
> parse_angle (const char *value)
> {
> - char *ptr;
> + const char *ptr;
> int angle;
>
> angle = grub_strtol (value, &ptr, 10);
> diff --git a/grub-core/gfxmenu/theme_loader.c b/grub-core/gfxmenu/theme_loader.c
> index d6829bb5e90..eae83086bcc 100644
> --- a/grub-core/gfxmenu/theme_loader.c
> +++ b/grub-core/gfxmenu/theme_loader.c
> @@ -484,7 +484,7 @@ parse_proportional_spec (const char *value, signed *abs, grub_fixed_signed_t *pr
> ptr++;
> }
>
> - num = grub_strtoul (ptr, (char **) &ptr, 0);
> + num = grub_strtoul (ptr, &ptr, 0);
> if (grub_errno)
> return grub_errno;
> if (sig)
> diff --git a/grub-core/kern/fs.c b/grub-core/kern/fs.c
> index 2b85f4950bd..88d39360da0 100644
> --- a/grub-core/kern/fs.c
> +++ b/grub-core/kern/fs.c
> @@ -134,7 +134,7 @@ struct grub_fs_block
> static grub_err_t
> grub_fs_blocklist_open (grub_file_t file, const char *name)
> {
> - char *p = (char *) name;
> + const char *p = name;
> unsigned num = 0;
> unsigned i;
> grub_disk_t disk = file->device->disk;
> diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
> index 3b633d51f4c..9d6eccc12ff 100644
> --- a/grub-core/kern/misc.c
> +++ b/grub-core/kern/misc.c
> @@ -340,7 +340,7 @@ grub_isspace (int c)
> }
>
> unsigned long
> -grub_strtoul (const char *str, char **end, int base)
> +grub_strtoul (const char *str, const char ** const end, int base)
> {
> unsigned long long num;
>
> @@ -357,7 +357,7 @@ grub_strtoul (const char *str, char **end, int base)
> }
>
> unsigned long long
> -grub_strtoull (const char *str, char **end, int base)
> +grub_strtoull (const char *str, const char ** const end, int base)
> {
> unsigned long long num = 0;
> int found = 0;
> @@ -853,14 +853,14 @@ grub_vsnprintf_real (char *str, grub_size_t max_len, const char *fmt0,
> {
> if (fmt[0] == '0')
> zerofill = '0';
> - format1 = grub_strtoul (fmt, (char **) &fmt, 10);
> + format1 = grub_strtoul (fmt, &fmt, 10);
> }
>
> if (*fmt == '.')
> fmt++;
>
> if (grub_isdigit (*fmt))
> - format2 = grub_strtoul (fmt, (char **) &fmt, 10);
> + format2 = grub_strtoul (fmt, &fmt, 10);
>
> if (*fmt == '$')
> {
> diff --git a/grub-core/kern/partition.c b/grub-core/kern/partition.c
> index e499147cbcb..2c401b866c4 100644
> --- a/grub-core/kern/partition.c
> +++ b/grub-core/kern/partition.c
> @@ -126,7 +126,7 @@ grub_partition_probe (struct grub_disk *disk, const char *str)
> while (*ptr && grub_isalpha (*ptr))
> ptr++;
> partname_end = ptr;
> - num = grub_strtoul (ptr, (char **) &ptr, 0) - 1;
> + num = grub_strtoul (ptr, &ptr, 0) - 1;
>
> curpart = 0;
> /* Use the first partition map type found. */
> diff --git a/grub-core/lib/arg.c b/grub-core/lib/arg.c
> index fd7744a6ff6..ccc185017ee 100644
> --- a/grub-core/lib/arg.c
> +++ b/grub-core/lib/arg.c
> @@ -375,7 +375,7 @@ grub_arg_parse (grub_extcmd_t cmd, int argc, char **argv,
>
> case ARG_TYPE_INT:
> {
> - char *tail;
> + const char * tail;
>
> grub_strtoull (option, &tail, 0);
> if (tail == 0 || tail == option || *tail != '\0' || grub_errno)
> diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
> index 412f26f17f0..15c0d99491c 100644
> --- a/grub-core/lib/json/json.c
> +++ b/grub-core/lib/json/json.c
> @@ -227,7 +227,7 @@ grub_json_getuint64 (grub_uint64_t *out, const grub_json_t *parent, const char *
> {
> grub_json_type_t type;
> const char *value;
> - char *end;
> + const char *end;
> grub_err_t ret;
>
> ret = get_value (&type, &value, parent, key);
> @@ -249,7 +249,7 @@ grub_json_getint64 (grub_int64_t *out, const grub_json_t *parent, const char *ke
> {
> grub_json_type_t type;
> const char *value;
> - char *end;
> + const char *end;
> grub_err_t ret;
>
> ret = get_value (&type, &value, parent, key);
> diff --git a/grub-core/lib/legacy_parse.c b/grub-core/lib/legacy_parse.c
> index ef56150ac77..05719ab2ccb 100644
> --- a/grub-core/lib/legacy_parse.c
> +++ b/grub-core/lib/legacy_parse.c
> @@ -418,7 +418,7 @@ adjust_file (const char *in, grub_size_t len)
> }
> if (*comma != ',')
> return grub_legacy_escape (in, len);
> - part = grub_strtoull (comma + 1, (char **) &rest, 0);
> + part = grub_strtoull (comma + 1, &rest, 0);
> if (rest[0] == ',' && rest[1] >= 'a' && rest[1] <= 'z')
> {
> subpart = rest[1] - 'a';
> diff --git a/grub-core/lib/syslinux_parse.c b/grub-core/lib/syslinux_parse.c
> index 4afa99279a2..de9fda06f52 100644
> --- a/grub-core/lib/syslinux_parse.c
> +++ b/grub-core/lib/syslinux_parse.c
> @@ -1062,7 +1062,7 @@ write_entry (struct output_buffer *outbuf,
> if (ptr[0] == 'h' && ptr[1] == 'd')
> {
> is_fd = 0;
> - devn = grub_strtoul (ptr + 2, &ptr, 0);
> + devn = grub_strtoul (ptr + 2, (const char **)&ptr, 0);
> continue;
> }
> if (grub_strncasecmp (ptr, "file=", 5) == 0)
> @@ -1086,12 +1086,12 @@ write_entry (struct output_buffer *outbuf,
> if (ptr[0] == 'f' && ptr[1] == 'd')
> {
> is_fd = 1;
> - devn = grub_strtoul (ptr + 2, &ptr, 0);
> + devn = grub_strtoul (ptr + 2, (const char **)&ptr, 0);
> continue;
> }
> if (grub_isdigit (ptr[0]))
> {
> - part = grub_strtoul (ptr, &ptr, 0);
> + part = grub_strtoul (ptr, (const char **)&ptr, 0);
> continue;
> }
> /* FIXME: isolinux, ntldr, cmldr, *dos, seg, hide
> diff --git a/grub-core/loader/i386/bsd.c b/grub-core/loader/i386/bsd.c
> index 3730ed38247..eb82391dbea 100644
> --- a/grub-core/loader/i386/bsd.c
> +++ b/grub-core/loader/i386/bsd.c
> @@ -1615,7 +1615,7 @@ grub_cmd_openbsd (grub_extcmd_context_t ctxt, int argc, char *argv[])
> return grub_error (GRUB_ERR_BAD_ARGUMENT,
> "unknown disk type name");
>
> - unit = grub_strtoul (arg, (char **) &arg, 10);
> + unit = grub_strtoul (arg, &arg, 10);
> if (! (arg && *arg >= 'a' && *arg <= 'z'))
> return grub_error (GRUB_ERR_BAD_ARGUMENT,
> "only device specifications of form "
> @@ -1633,7 +1633,7 @@ grub_cmd_openbsd (grub_extcmd_context_t ctxt, int argc, char *argv[])
> if (ctxt->state[OPENBSD_SERIAL_ARG].set)
> {
> struct grub_openbsd_bootarg_console serial;
> - char *ptr;
> + const char *ptr;
> unsigned port = 0;
> unsigned speed = 9600;
>
> @@ -1735,7 +1735,7 @@ grub_cmd_netbsd (grub_extcmd_context_t ctxt, int argc, char *argv[])
> if (ctxt->state[NETBSD_SERIAL_ARG].set)
> {
> struct grub_netbsd_btinfo_serial serial;
> - char *ptr;
> + const char *ptr;
>
> grub_memset (&serial, 0, sizeof (serial));
> grub_strcpy (serial.devname, "com");
> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> index ee95cd374b7..ac1fae72e3b 100644
> --- a/grub-core/loader/i386/linux.c
> +++ b/grub-core/loader/i386/linux.c
> @@ -944,7 +944,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
> #endif /* GRUB_MACHINE_PCBIOS */
> if (grub_memcmp (argv[i], "mem=", 4) == 0)
> {
> - char *val = argv[i] + 4;
> + const char *val = argv[i] + 4;
>
> linux_mem_size = grub_strtoul (val, &val, 0);
>
> diff --git a/grub-core/loader/i386/pc/linux.c b/grub-core/loader/i386/pc/linux.c
> index 47ea2945e4f..f56af7c501f 100644
> --- a/grub-core/loader/i386/pc/linux.c
> +++ b/grub-core/loader/i386/pc/linux.c
> @@ -259,7 +259,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
> }
> else if (grub_memcmp (argv[i], "mem=", 4) == 0)
> {
> - char *val = argv[i] + 4;
> + const char *val = argv[i] + 4;
>
> linux_mem_size = grub_strtoul (val, &val, 0);
>
> diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c
> index 6329ec01038..27afcaacbce 100644
> --- a/grub-core/loader/i386/xen_fileXX.c
> +++ b/grub-core/loader/i386/xen_fileXX.c
> @@ -25,7 +25,7 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
> grub_off_t off, grub_size_t sz)
> {
> char *buf;
> - char *ptr;
> + const char *ptr;
> int has_paddr = 0;
>
> grub_errno = GRUB_ERR_NONE;
> diff --git a/grub-core/mmap/mmap.c b/grub-core/mmap/mmap.c
> index 6a31cbae325..b569cb23b5a 100644
> --- a/grub-core/mmap/mmap.c
> +++ b/grub-core/mmap/mmap.c
> @@ -423,7 +423,7 @@ static grub_err_t
> grub_cmd_badram (grub_command_t cmd __attribute__ ((unused)),
> int argc, char **args)
> {
> - char * str;
> + const char *str;
> struct badram_entry entry;
>
> if (argc != 1)
> @@ -465,7 +465,7 @@ static grub_uint64_t
> parsemem (const char *str)
> {
> grub_uint64_t ret;
> - char *ptr;
> + const char *ptr;
>
> ret = grub_strtoul (str, &ptr, 0);
>
> diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> index ec3647f9a0c..b616cf40b1e 100644
> --- a/grub-core/net/http.c
> +++ b/grub-core/net/http.c
> @@ -107,7 +107,7 @@ parse_line (grub_file_t file, http_data_t data, char *ptr, grub_size_t len)
> return GRUB_ERR_NONE;
> }
> ptr += sizeof ("HTTP/1.1 ") - 1;
> - code = grub_strtoul (ptr, &ptr, 10);
> + code = grub_strtoul (ptr, (const char **)&ptr, 10);
> if (grub_errno)
> return grub_errno;
> switch (code)
> @@ -134,7 +134,7 @@ parse_line (grub_file_t file, http_data_t data, char *ptr, grub_size_t len)
> == 0 && !data->size_recv)
> {
> ptr += sizeof ("Content-Length: ") - 1;
> - file->size = grub_strtoull (ptr, &ptr, 10);
> + file->size = grub_strtoull (ptr, (const char **)&ptr, 10);
> data->size_recv = 1;
> return GRUB_ERR_NONE;
> }
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index d5d726a315e..2fe2bbecb01 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -406,7 +406,7 @@ parse_ip (const char *val, grub_uint32_t *ip, const char **rest)
> for (i = 0; i < 4; i++)
> {
> unsigned long t;
> - t = grub_strtoul (ptr, (char **) &ptr, 0);
> + t = grub_strtoul (ptr, &ptr, 0);
> if (grub_errno)
> {
> grub_errno = GRUB_ERR_NONE;
> @@ -453,7 +453,7 @@ parse_ip6 (const char *val, grub_uint64_t *ip, const char **rest)
> ptr++;
> continue;
> }
> - t = grub_strtoul (ptr, (char **) &ptr, 16);
> + t = grub_strtoul (ptr, &ptr, 16);
> if (grub_errno)
> {
> grub_errno = GRUB_ERR_NONE;
> @@ -563,7 +563,7 @@ grub_net_resolve_net_address (const char *name,
> addr->type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
> if (*rest == '/')
> {
> - addr->ipv4.masksize = grub_strtoul (rest + 1, (char **) &rest, 0);
> + addr->ipv4.masksize = grub_strtoul (rest + 1, &rest, 0);
> if (!grub_errno && *rest == 0)
> return GRUB_ERR_NONE;
> grub_errno = GRUB_ERR_NONE;
> @@ -579,7 +579,7 @@ grub_net_resolve_net_address (const char *name,
> addr->type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> if (*rest == '/')
> {
> - addr->ipv6.masksize = grub_strtoul (rest + 1, (char **) &rest, 0);
> + addr->ipv6.masksize = grub_strtoul (rest + 1, &rest, 0);
> if (!grub_errno && *rest == 0)
> return GRUB_ERR_NONE;
> grub_errno = GRUB_ERR_NONE;
> diff --git a/grub-core/normal/menu.c b/grub-core/normal/menu.c
> index d5e0c79a70e..da66ad89180 100644
> --- a/grub-core/normal/menu.c
> +++ b/grub-core/normal/menu.c
> @@ -170,8 +170,7 @@ grub_menu_set_timeout (int timeout)
> static int
> get_and_remove_first_entry_number (const char *name)
> {
> - const char *val;
> - char *tail;
> + const char *val, *tail;
> int entry;
>
> val = grub_env_get (name);
> diff --git a/grub-core/osdep/aros/hostdisk.c b/grub-core/osdep/aros/hostdisk.c
> index 2be654ca3bd..3b2c9de2496 100644
> --- a/grub-core/osdep/aros/hostdisk.c
> +++ b/grub-core/osdep/aros/hostdisk.c
> @@ -194,7 +194,7 @@ grub_util_fd_open (const char *dev, int flg)
> p1 = dev + strlen (dev);
> else
> {
> - unit = grub_strtoul (p1 + 1, (char **) &p2, 16);
> + unit = grub_strtoul (p1 + 1, &p2, 16);
> if (p2 && *p2 == '/')
> flags = grub_strtoul (p2 + 1, 0, 16);
> }
> diff --git a/grub-core/osdep/devmapper/hostdisk.c b/grub-core/osdep/devmapper/hostdisk.c
> index a697bcb4d8d..a8afc0c9408 100644
> --- a/grub-core/osdep/devmapper/hostdisk.c
> +++ b/grub-core/osdep/devmapper/hostdisk.c
> @@ -113,7 +113,7 @@ grub_util_get_dm_node_linear_info (dev_t dev,
> void *next = NULL;
> uint64_t length, start;
> char *target, *params;
> - char *ptr;
> + const char *ptr;
> int major = 0, minor = 0;
> int first = 1;
> grub_disk_addr_t partstart = 0;
> diff --git a/grub-core/script/execute.c b/grub-core/script/execute.c
> index ee299fd0ea6..8cc61ee7d78 100644
> --- a/grub-core/script/execute.c
> +++ b/grub-core/script/execute.c
> @@ -122,7 +122,7 @@ replace_scope (struct grub_script_scope *new_scope)
> grub_err_t
> grub_script_break (grub_command_t cmd, int argc, char *argv[])
> {
> - char *p = 0;
> + const char *p = NULL;
> unsigned long count;
>
> if (argc == 0)
> @@ -154,7 +154,7 @@ grub_err_t
> grub_script_shift (grub_command_t cmd __attribute__((unused)),
> int argc, char *argv[])
> {
> - char *p = 0;
> + const char *p = NULL;
> unsigned long n = 0;
>
> if (! scope)
> @@ -215,7 +215,7 @@ grub_err_t
> grub_script_return (grub_command_t cmd __attribute__((unused)),
> int argc, char *argv[])
> {
> - char *p;
> + const char *p = NULL;
> unsigned long n;
>
> if (! scope || argc > 1)
> diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> index db80b3ba0fb..f9271b09239 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -269,7 +269,7 @@ grub_cmd_serial (grub_extcmd_context_t ctxt, int argc, char **args)
>
> if (state[OPTION_BASE_CLOCK].set)
> {
> - char *ptr;
> + const char *ptr;
> config.base_clock = grub_strtoull (state[OPTION_BASE_CLOCK].arg, &ptr, 0);
> if (grub_errno)
> return grub_errno;
> diff --git a/grub-core/term/terminfo.c b/grub-core/term/terminfo.c
> index d317efa368d..0e9de7f8fa9 100644
> --- a/grub-core/term/terminfo.c
> +++ b/grub-core/term/terminfo.c
> @@ -737,7 +737,7 @@ grub_cmd_terminfo (grub_extcmd_context_t ctxt, int argc, char **args)
>
> if (state[OPTION_GEOMETRY].set)
> {
> - char *ptr = state[OPTION_GEOMETRY].arg;
> + const char *ptr = state[OPTION_GEOMETRY].arg;
> w = grub_strtoul (ptr, &ptr, 0);
> if (grub_errno)
> return grub_errno;
> diff --git a/grub-core/tests/strtoull_test.c b/grub-core/tests/strtoull_test.c
> index 7da615ff33e..5488ab26b43 100644
> --- a/grub-core/tests/strtoull_test.c
> +++ b/grub-core/tests/strtoull_test.c
> @@ -25,7 +25,7 @@ static void
> strtoull_testcase (const char *input, int base, unsigned long long expected,
> int num_digits, grub_err_t error)
> {
> - char *output;
> + const char *output;
> unsigned long long value;
> grub_errno = 0;
> value = grub_strtoull(input, &output, base);
> diff --git a/include/grub/misc.h b/include/grub/misc.h
> index ee48eb7a726..8d8d260f8e4 100644
> --- a/include/grub/misc.h
> +++ b/include/grub/misc.h
> @@ -243,11 +243,11 @@ grub_strncasecmp (const char *s1, const char *s2, grub_size_t n)
> - (int) grub_tolower ((grub_uint8_t) *s2);
> }
>
> -unsigned long EXPORT_FUNC(grub_strtoul) (const char *str, char **end, int base);
> -unsigned long long EXPORT_FUNC(grub_strtoull) (const char *str, char **end, int base);
> +unsigned long EXPORT_FUNC(grub_strtoul) (const char *str, const char ** const end, int base);
> +unsigned long long EXPORT_FUNC(grub_strtoull) (const char *str, const char ** const end, int base);
>
> static inline long
> -grub_strtol (const char *str, char **end, int base)
> +grub_strtol (const char *str, const char ** const end, int base)
> {
> int negative = 0;
> unsigned long long magnitude;
> diff --git a/util/grub-fstest.c b/util/grub-fstest.c
> index f14e02d9727..7f14620cd49 100644
> --- a/util/grub-fstest.c
> +++ b/util/grub-fstest.c
> @@ -538,7 +538,7 @@ void (*argp_program_version_hook) (FILE *, struct argp_state *) = print_version;
> static error_t
> argp_parser (int key, char *arg, struct argp_state *state)
> {
> - char *p;
> + const char *p;
>
> switch (key)
> {
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Make grub_strtoul() "end" pointer have the right const params.
2020-02-05 1:04 ` Nicholas Vinson
@ 2020-02-19 0:32 ` Peter Jones
2020-02-19 3:39 ` Nicholas Vinson
0 siblings, 1 reply; 7+ messages in thread
From: Peter Jones @ 2020-02-19 0:32 UTC (permalink / raw)
To: The development of GNU GRUB
On Tue, Feb 04, 2020 at 08:04:30PM -0500, Nicholas Vinson wrote:
> On 2/4/20 16:02, Peter Jones wrote:
> > grub_strtoul() and grub_strtoull() don't make the /pointer/ to "end" be
> > const like normal implementations do, and as a result, at many places in
>
> grub_strtoul() and grub_strtoull() appear to be patterned after the C
> standard functions strtoul() and strtoull().
>
> According to C18, the signatures for those functions are:
>
> unsigned long int strtoul(const char * restrict nptr,
> char ** restrict endptr, int base);
>
> unsigned long long int strtoull(const char * restrict nptr,
> char ** restrict endptr, int base);
It's true that the standard does not mandate any const usage on the
endptr parameter, and it does have restrict. I'm happy to add restrict,
though I'm going to ignore it for the rest of this email, because the
standard literally says it changes no semantics. Having "const" on both the
object pointed to by **endptr and on the outer pointer is still
compatible with the standard, though - nothing that's allowed by the
standard isn't allowed with those qualifiers. Semantically, the
behavior is described in a way that is compatible with making those
const as well: the value written to *endptr must point inside the object
pointed to by nptr, and neither function is allowed to modify that
object.
Adding the const requirement does give us benefits though:
- It means when someone is modifying grub_strtoul() or grub_strtoull(),
as inevitably happens every once in a while, making an error which
would violate those requirements will cause an error building that
change, rather than making it in to the tree. That is, "const char **"
protects us from making an error that writes modifies the object nptr
points at, and "char ** const" protects us from accidentally changing
the local pointer instead of the caller's pointer.
- Likewise, if someone isn't thinking through what they're doing and
takes the const out in order to "fix" some perceived problem, that
should alert whomever is reviewing the code. In both of these
regards, having the const qualifiers helps us trust that our
implementation is correct.
- It lets us pass in pointers that we *want* to be const from the
caller's point of view. It means that if a function calling strtoul()
has a "const char *" argument, it can safely pass that as nptr as well
as passing its address as endptr; without making the object const (i.e
at least "const char ** endptr"), that'll get you a warning about
discarding the type qualifier. That applies equally to cases that
want to wrap these functions with another function that has similar
semantics; having const qualifiers allows that function to declare
those things const or not. Not having the qualifiers here means the
calling function also can't, which is where we wind up having a lot of
the ugly code.
- The same applies to cases where the object is (or should be) genuinely
immutable - for instance, data that's in .rodata, in data provided
by an option ROM, or firmware configuration table. It is better to be
able to keep the type qualifier throughout the whole stack.
- It allows the optimizer to make assumptions about how we're using it
which may allow it to generate more efficient output without having to
analyze the caller.
> Therefore, I recommend the GRUB maintainers keep the existing signatures for
> grub_strtoul() and grub_strtoull() as they more closely follow the
> signatures of the standard functions.
Obviously, I don't agree.
> I also skimmed through the rest of the changes and it seems to me most of
> the misuse of these calls could be solved simply by doing some variation of:
>
> const char *ptr = ...;
> char *end;
>
> strtoul(ptr, &end, 10);
> ptr = end;
While this is valid in many of the existing cases, it's not as good in
several ways. In addition to the last three points I made above, this
also creates an unnecessary alias into the character object, which the
compiler then has to track. In this case, since it is both a qualified
type of the same type of object and a character object, it doesn't
violate the aliasing rules ("-fstrict-aliasing -Wstrict-aliasing" isn't
allowed to complain), but it can cause the compiler to both take longer
and generate more, slower code than if that were:
const char *ptr = ...;
unsigned long val;
val = strtoul(ptr, &ptr, 10);
That's true for both the caller and in the implementation of strtoul()
itself - though I don't think with current GCC it *will* cause the
strtoul() output to be much different, because it's pretty simple, but
for a more complex function like sscanf() calling it, the chances are a
bit rougher. Hypothetically, it can also reduce the chances that it
winds up getting inlined if we were to use -flto.
> which is what I would personally recommend instead. If the compiler
> issues an error or warning with the above, then that's a bug the
> compiler maintainers should fix.
That's true, but it puts limits on the caller's usage and the compiler's
output.
--
Peter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Make grub_strtoul() "end" pointer have the right const params.
2020-02-19 0:32 ` Peter Jones
@ 2020-02-19 3:39 ` Nicholas Vinson
2020-02-21 21:35 ` Peter Jones
0 siblings, 1 reply; 7+ messages in thread
From: Nicholas Vinson @ 2020-02-19 3:39 UTC (permalink / raw)
To: grub-devel
On 2/18/20 19:32, Peter Jones wrote:
> On Tue, Feb 04, 2020 at 08:04:30PM -0500, Nicholas Vinson wrote:
>> On 2/4/20 16:02, Peter Jones wrote:
>>> grub_strtoul() and grub_strtoull() don't make the /pointer/ to "end" be
>>> const like normal implementations do, and as a result, at many places in
>>
>> grub_strtoul() and grub_strtoull() appear to be patterned after the C
>> standard functions strtoul() and strtoull().
>>
>> According to C18, the signatures for those functions are:
>>
>> unsigned long int strtoul(const char * restrict nptr,
>> char ** restrict endptr, int base);
>>
>> unsigned long long int strtoull(const char * restrict nptr,
>> char ** restrict endptr, int base);
>
> It's true that the standard does not mandate any const usage on the
> endptr parameter, and it does have restrict. I'm happy to add restrict,
> though I'm going to ignore it for the rest of this email, because the
> standard literally says it changes no semantics. Having "const" on both the
> object pointed to by **endptr and on the outer pointer is still
> compatible with the standard, though - nothing that's allowed by the
> standard isn't allowed with those qualifiers. Semantically, the
> behavior is described in a way that is compatible with making those
> const as well: the value written to *endptr must point inside the object
I'm afraid that's not true. That interpretation ignores the fact that
the caller is permitted to use both nptr (provided the first argument is
not const char *) and endptr to modify the underlying object. For
example, the standard allows:
char *num = "12345_";
char *end;
unsigned long long l;
l = strtoull(num, &end, 10); // returns 12,345. end points to '_'
**endptr = '6';
l = strtoull(num, &end, 10); // returns 123,456. end points to '\0'
With the proposed modifications all such use cases would require
explicit casts to suppress the warning gcc would generate.
> pointed to by nptr, and neither function is allowed to modify that
> object. >
> Adding the const requirement does give us benefits though:
> - It means when someone is modifying grub_strtoul() or grub_strtoull(),
> as inevitably happens every once in a while, making an error which
> would violate those requirements will cause an error building that
> change, rather than making it in to the tree. That is, "const char **"
> protects us from making an error that writes modifies the object nptr
> points at, and "char ** const" protects us from accidentally changing
> the local pointer instead of the caller's pointer
'const char **' also requires the caller to cast away the const or use
strange pointer arithmetic to modify the underlying object.
'char ** const' is probably OK semantically.
> - Likewise, if someone isn't thinking through what they're doing and
> takes the const out in order to "fix" some perceived problem, that
> should alert whomever is reviewing the code. In both of these
agreed.
> regards, having the const qualifiers helps us trust that our
> implementation is correct.
> - It lets us pass in pointers that we *want* to be const from the
> caller's point of view. It means that if a function calling strtoul()
> has a "const char *" argument, it can safely pass that as nptr as well
> as passing its address as endptr; without making the object const (i.e
If you add the 'restrict' keyword, then endptr cannot point to any part
of the object nptr points to. The standard C functions have never
allowed this usage which is why 'restrict' was added to the declarations
in C99.
It's syntactically valid to do this with the GRUB variants (just like it
is with C89), but it's not logically valid.
> at least "const char ** endptr"), that'll get you a warning about
> discarding the type qualifier. That applies equally to cases that
> want to wrap these functions with another function that has similar
> semantics; having const qualifiers allows that function to declare
> those things const or not. Not having the qualifiers here means the
> calling function also can't, which is where we wind up having a lot of
> the ugly code.
The wrapping functions can have all const parameters and still use the
strto* functions without casting. Example below:
long foo(const char *nptr, const char ** const endptr) {
char *tmp
long l;
l = strtoul(nptr, &tmp, 10);
*endptr = tmp;
return l;
}
Are there functions in GRUB that attempt to do this?
> - The same applies to cases where the object is (or should be) genuinely
> immutable - for instance, data that's in .rodata, in data provided
> by an option ROM, or firmware configuration table. It is better to be
> able to keep the type qualifier throughout the whole stack.
> - It allows the optimizer to make assumptions about how we're using it
> which may allow it to generate more efficient output without having to
> analyze the caller.
>
>> Therefore, I recommend the GRUB maintainers keep the existing signatures for
>> grub_strtoul() and grub_strtoull() as they more closely follow the
>> signatures of the standard functions.
>
> Obviously, I don't agree.
>
>> I also skimmed through the rest of the changes and it seems to me most of
>> the misuse of these calls could be solved simply by doing some variation of:
>>
>> const char *ptr = ...;
>> char *end;
>>
>> strtoul(ptr, &end, 10);
>> ptr = end;
>
> While this is valid in many of the existing cases, it's not as good in
> several ways. In addition to the last three points I made above, this
> also creates an unnecessary alias into the character object, which the
> compiler then has to track. In this case, since it is both a qualified
> type of the same type of object and a character object, it doesn't
> violate the aliasing rules ("-fstrict-aliasing -Wstrict-aliasing" isn't
> allowed to complain), but it can cause the compiler to both take longer
> and generate more, slower code than if that were:
>
> const char *ptr = ...;
> unsigned long val;
>
> val = strtoul(ptr, &ptr, 10);
>
As I stated above this is not logically valid.
> That's true for both the caller and in the implementation of strtoul()
> itself - though I don't think with current GCC it *will* cause the
> strtoul() output to be much different, because it's pretty simple, but
> for a more complex function like sscanf() calling it, the chances are a
> bit rougher. Hypothetically, it can also reduce the chances that it
> winds up getting inlined if we were to use -flto.
It's also not logically valid to use sscanf() that way either. The only
standard C function I can think of where it is logically valid to have
source and destination buffers overlap is memmove().
>
>> which is what I would personally recommend instead. If the compiler
>> issues an error or warning with the above, then that's a bug the
>> compiler maintainers should fix.
>
> That's true, but it puts limits on the caller's usage and the compiler's
> output >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Make grub_strtoul() "end" pointer have the right const params.
2020-02-19 3:39 ` Nicholas Vinson
@ 2020-02-21 21:35 ` Peter Jones
2020-02-21 21:39 ` [PATCH] Make grub_strtol() "end" pointers have safer const qualifiers. (v2) Peter Jones
0 siblings, 1 reply; 7+ messages in thread
From: Peter Jones @ 2020-02-21 21:35 UTC (permalink / raw)
To: The development of GNU GRUB
On Tue, Feb 18, 2020 at 10:39:07PM -0500, Nicholas Vinson wrote:
> On 2/18/20 19:32, Peter Jones wrote:
> > On Tue, Feb 04, 2020 at 08:04:30PM -0500, Nicholas Vinson wrote:
> > > On 2/4/20 16:02, Peter Jones wrote:
> > > > grub_strtoul() and grub_strtoull() don't make the /pointer/ to "end" be
> > > > const like normal implementations do, and as a result, at many places in
> > >
> > > grub_strtoul() and grub_strtoull() appear to be patterned after the C
> > > standard functions strtoul() and strtoull().
> > >
> > > According to C18, the signatures for those functions are:
> > >
> > > unsigned long int strtoul(const char * restrict nptr,
> > > char ** restrict endptr, int base);
> > >
> > > unsigned long long int strtoull(const char * restrict nptr,
> > > char ** restrict endptr, int base);
> >
> > It's true that the standard does not mandate any const usage on the
> > endptr parameter, and it does have restrict. I'm happy to add restrict,
> > though I'm going to ignore it for the rest of this email, because the
> > standard literally says it changes no semantics. Having "const" on both the
> > object pointed to by **endptr and on the outer pointer is still
> > compatible with the standard, though - nothing that's allowed by the
> > standard isn't allowed with those qualifiers. Semantically, the
> > behavior is described in a way that is compatible with making those
> > const as well: the value written to *endptr must point inside the object
>
> I'm afraid that's not true. That interpretation ignores the fact that the
> caller is permitted to use both nptr (provided the first argument is not
> const char *) and endptr to modify the underlying object. For example, the
> standard allows:
>
> char *num = "12345_";
> char *end;
> unsigned long long l;
>
> l = strtoull(num, &end, 10); // returns 12,345. end points to '_'
>
> **endptr = '6';
(assuming you mean *end not **endptr)
>
> l = strtoull(num, &end, 10); // returns 123,456. end points to '\0'
>
> With the proposed modifications all such use cases would require explicit
> casts to suppress the warning gcc would generate.
That's a fair point, this usage will get you a warning - but this is
another case where having to cast it to const for the call is a giant
flag saying "I'm doing something really weird" to anyone reading the
code. This is also something that currently occurs in our moderately
large source tree exactly zero times, though there are five cases (three
in grub-core/lib/syslinux_parse.c , two in grub-core/net/http.c ) that
look vaguely similar to that, without actually changing the character
objects.
> > pointed to by nptr, and neither function is allowed to modify that
> > object. >
> > Adding the const requirement does give us benefits though:
> > - It means when someone is modifying grub_strtoul() or grub_strtoull(),
> > as inevitably happens every once in a while, making an error which
> > would violate those requirements will cause an error building that
> > change, rather than making it in to the tree. That is, "const char **"
> > protects us from making an error that writes modifies the object nptr
> > points at, and "char ** const" protects us from accidentally changing
> > the local pointer instead of the caller's pointer
>
> 'const char **' also requires the caller to cast away the const or use
> strange pointer arithmetic to modify the underlying object.
That's true, but it's also the case that since grub_strtoul() is not
strtoul(), neither having it or not having it is "right" or "wrong", but
just slightly different. Looking at the code we've got, that gives us five cases
where we have to do that cast, versus 26 cases where we currently have
to cast pointers to const char * objects by using (char **). Between
that being a pretty big ratio and the actual advantages we get, I'd much
prefer to have this problem over what we've already got.
> 'char ** const' is probably OK semantically.
Yes, this part constrains behavior that is implicitly required of
upstream strtoul() implementations anyway.
> > - Likewise, if someone isn't thinking through what they're doing and
> > takes the const out in order to "fix" some perceived problem, that
> > should alert whomever is reviewing the code. In both of these
>
> agreed.
>
> > regards, having the const qualifiers helps us trust that our
> > implementation is correct.
> > - It lets us pass in pointers that we *want* to be const from the
> > caller's point of view. It means that if a function calling strtoul()
> > has a "const char *" argument, it can safely pass that as nptr as well
> > as passing its address as endptr; without making the object const (i.e
>
> If you add the 'restrict' keyword, then endptr cannot point to any part of
> the object nptr points to.
Assuming you meant that *endptr can't (endptr shouldn't even without
restrict, it's a pointer to a pointer), this is incorrect for several
reasons. Firstly, because the type qualifier places restrictions on
*accesses* of objects referred to by the pointer, not on the *values* of
that pointer or any other. Secondly, because of the type qualifier
syntax for * (see original commit message), with this declaration (and
the matching definition):
unsigned long strtoul(const char * restrict nptr,
char ** restrict endptr, int base);
the second argument declares the symbol endptr, which represents a
restrict-qualified pointer to a pointer to a character object. Accesses
to the character object, i.e. *nptr=... or eventually **endptr=...) are
not restricted via the *second* argument's declaration, only the
accesses to *endptr are. This qualifier says that nothing strtoul()
does will change the value of *endptr except assigning to *endptr,
either directly or indirectly. That is, strtoul() can still do the
slightly obnoxious:
char ** restrict * endptrptr = &endptr;
**endptrptr = nptr;
but it can't do the rather insane:
/* assume the caller did:
* char *ptrs[2] = { "12345_", NULL };
* unsigned long l = strtoul(ptrs[0], &ptrs[1], 10);
*/
char **nptrptr = &nptr;
*endptr = NULL;
nptrptr[1] = nptr;
This code is horrifying, but without restrict on the second argument
(given that there are no other callers of strtoul()) the behavior is
defined and even correct. In the presence of restrict it will result in
undefined behavior - but it's strtoul()'s behavior that's undefined, not
that of the caller.
There are also several behaviors banned by the restrict parameter on
nptr.
I can go into significantly more detail here, but the point is that
restrict doesn't actually matter to the "const" parts at all, and it
also doesn't really change much practically in terms of strtoul().
> The standard C functions have never allowed this
> usage which is why 'restrict' was added to the declarations in C99.
Again, no, that's not correct, it just wasn't ever a good idea, and in
an API it can lead to undesired or undefined behavior; with restrict
both of those cases become undefined.
> It's syntactically valid to do this with the GRUB variants (just like
> it is with C89), but it's not logically valid.
>
> > at least "const char ** endptr"), that'll get you a warning about
> > discarding the type qualifier. That applies equally to cases that
> > want to wrap these functions with another function that has similar
> > semantics; having const qualifiers allows that function to declare
> > those things const or not. Not having the qualifiers here means the
> > calling function also can't, which is where we wind up having a lot of
> > the ugly code.
>
> The wrapping functions can have all const parameters and still use the
> strto* functions without casting. Example below:
>
> long foo(const char *nptr, const char ** const endptr) {
> char *tmp
> long l;
>
> l = strtoul(nptr, &tmp, 10);
> *endptr = tmp;
> return l;
> }
>
> Are there functions in GRUB that attempt to do this?
Well, of course both grub_strtol() and grub_strtoul() just call into
grub_strtoull().
> > - The same applies to cases where the object is (or should be) genuinely
> > immutable - for instance, data that's in .rodata, in data provided
> > by an option ROM, or firmware configuration table. It is better to be
> > able to keep the type qualifier throughout the whole stack.
> > - It allows the optimizer to make assumptions about how we're using it
> > which may allow it to generate more efficient output without having to
> > analyze the caller.
> >
> > > Therefore, I recommend the GRUB maintainers keep the existing signatures for
> > > grub_strtoul() and grub_strtoull() as they more closely follow the
> > > signatures of the standard functions.
> >
> > Obviously, I don't agree.
> >
> > > I also skimmed through the rest of the changes and it seems to me most of
> > > the misuse of these calls could be solved simply by doing some variation of:
> > >
> > > const char *ptr = ...;
> > > char *end;
> > >
> > > strtoul(ptr, &end, 10);
> > > ptr = end;
> >
> > While this is valid in many of the existing cases, it's not as good in
> > several ways. In addition to the last three points I made above, this
> > also creates an unnecessary alias into the character object, which the
> > compiler then has to track. In this case, since it is both a qualified
> > type of the same type of object and a character object, it doesn't
> > violate the aliasing rules ("-fstrict-aliasing -Wstrict-aliasing" isn't
> > allowed to complain), but it can cause the compiler to both take longer
> > and generate more, slower code than if that were:
> >
> > const char *ptr = ...;
> > unsigned long val;
> >
> > val = strtoul(ptr, &ptr, 10);
> >
>
> As I stated above this is not logically valid.
Again I don't agree; the compiler does the expected thing with it, and
we use it usefully already in many places. It appears to comply with
the standard in every way *except* that it discards the const qualifier,
though the called function does in fact treat the character array and
the local endptr as if they are const.
> > That's true for both the caller and in the implementation of strtoul()
> > itself - though I don't think with current GCC it *will* cause the
> > strtoul() output to be much different, because it's pretty simple, but
> > for a more complex function like sscanf() calling it, the chances are a
> > bit rougher. Hypothetically, it can also reduce the chances that it
> > winds up getting inlined if we were to use -flto.
>
> It's also not logically valid to use sscanf() that way either. The only
> standard C function I can think of where it is logically valid to have
> source and destination buffers overlap is memmove().
None of these examples are of buffers overlapping. They have one
argument that points to a buffer as an input parameter and one that
points to a pointer, which is an output parameter and is not allowed to
be inside the buffer the other one points to. They don't write to the
buffer.
--
Peter
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] Make grub_strtol() "end" pointers have safer const qualifiers. (v2)
2020-02-21 21:35 ` Peter Jones
@ 2020-02-21 21:39 ` Peter Jones
2020-02-24 9:42 ` Daniel Kiper
0 siblings, 1 reply; 7+ messages in thread
From: Peter Jones @ 2020-02-21 21:39 UTC (permalink / raw)
To: grub-devel; +Cc: Daniel Kiper, Peter Jones
Currently the string functions grub_strtol(), grub_strtoul(), and
grub_strtoull() don't declare the "end" pointer in such a way as to
require the pointer itself or the character array to be immutable to the
implementation, nor does the C standard do so in its similar functions,
though it does require us not to change any of it.
The typical declarations of these functions follow this pattern:
long
strtol(const char * restrict nptr, char ** restrict endptr, int base);
Much of the reason for this is historic, and a discussion of that
follows below, after the explanation of this change. (GRUB currently
does not include the "restrict" qualifiers, and we name the arguments a
bit differently.)
The implementation is semantically required to treat the character array
as immutable, but such accidental modifications aren't stopped by the
compiler, and the semantics for both the callers and the implementation
of these functions are sometimes also helped by adding that requirement.
This patch changes these declarations to follow this pattern instead:
long
strtol(const char * restrict nptr,
const char ** const restrict endptr,
int base);
This means that if any modification to these functions accidentally
introduces either an errant modification to the underlying character
array, or an accidental assignment to endptr rather than *endptr, the
compiler should generate an error. (The two uses of "restrict" in this
case basically mean strtol() isn't allowed to modify the character array
by going through *endptr, and endptr isn't allowed to point inside the
array.)
It also means the typical use case changes to:
char *s = ...;
const char *end;
long l;
l = strtol(s, &end, 10);
Or even:
const char *p = str;
while (p && *p) {
long l = strtol(p, &p, 10);
...
}
This fixes 26 places where we discard our attempts at treating the data
safely by doing:
const char *p = str;
long l;
l = strtol(p, (char **)&ptr, 10);
It also adds 5 places where we do:
char *p = str;
while (p && *p) {
long l = strtol(p, (const char ** const)&p, 10);
...
/* more calls that need p not to be pointer-to-const */
}
While moderately distasteful, this is a better problem to have.
With one minor exception, I have tested that all of this compiles
without relevant warnings or errors, and that /much/ of it behaves
correctly, with gcc 9 using 'gcc -W -Wall -Wextra'. The one exception
is the changes in grub-core/osdep/aros/hostdisk.c , which I have no idea
how to build.
Because the C standard defined type-qualifiers in a way that can be
confusing, in the past there's been a slow but fairly regular stream of
churn within our patches, which add and remove the const qualifier in many
of the users of these functions. This change should help avoid that in
the future, and in order to help ensure this, I've added an explanation
in misc.h so that when someone does get a compiler warning about a type
error, they have the fix at hand.
The reason we don't have "const" in these calls in the standard is
purely anachronistic: C78 (de facto) did not have type qualifiers in the
syntax, and the "const" type qualifier was added for C89 (I think; it
may have been later). strtol() appears to date from 4.3BSD in 1986,
which means it could not be added to those functions in the standard
without breaking compatibility, which is usually avoided.
The syntax chosen for type qualifiers is what has led to the churn
regarding usage of const, and is especially confusing on string
functions due to the lack of a string type. Quoting from C99, the
syntax is:
declarator:
pointer[opt] direct-declarator
direct-declarator:
identifier
( declarator )
direct-declarator [ type-qualifier-list[opt] assignment-expression[opt] ]
...
direct-declarator [ type-qualifier-list[opt] * ]
...
pointer:
* type-qualifier-list[opt]
* type-qualifier-list[opt] pointer
type-qualifier-list:
type-qualifier
type-qualifier-list type-qualifier
...
type-qualifier:
const
restrict
volatile
So the examples go like:
const char foo; // immutable object
const char *foo; // mutable pointer to object
char * const foo; // immutable pointer to mutable object
const char * const foo; // immutable pointer to immutable object
const char const * const foo; // XXX extra const keyword in the middle
const char * const * const foo; // immutable pointer to immutable
// pointer to immutable object
const char ** const foo; // immutable pointer to mutable pointer
// to immutable object
Making const left-associative for * and right-associative for everything
else may not have been the best choice ever, but here we are, and the
inevitable result is people using trying to use const (as they should!),
putting it at the wrong place, fighting with the compiler for a bit, and
then either removing it or typecasting something in a bad way. I won't
go into describing restrict, but its syntax has exactly the same issue
as with const.
Anyway, the last example above actually represents the *behavior* that's
required of strtol()-like functions, so that's our choice for the "end"
pointer.
Signed-off-by: Peter Jones <pjones@redhat.com>
---
grub-core/commands/date.c | 3 ++-
grub-core/commands/i386/cmostest.c | 2 +-
grub-core/commands/i386/pc/play.c | 2 +-
grub-core/commands/i386/rdmsr.c | 2 +-
grub-core/commands/i386/wrmsr.c | 2 +-
grub-core/commands/password_pbkdf2.c | 2 +-
grub-core/commands/pcidump.c | 13 ++++++------
grub-core/commands/regexp.c | 2 +-
grub-core/commands/setpci.c | 21 ++++++++++----------
grub-core/commands/test.c | 2 +-
grub-core/commands/videoinfo.c | 2 +-
grub-core/disk/diskfilter.c | 3 ++-
grub-core/disk/lvm.c | 9 +++++----
grub-core/efiemu/pnvram.c | 5 +++--
| 2 +-
| 2 +-
grub-core/kern/fs.c | 2 +-
grub-core/kern/misc.c | 10 ++++++----
grub-core/kern/partition.c | 2 +-
grub-core/lib/arg.c | 2 +-
grub-core/lib/json/json.c | 4 ++--
grub-core/lib/legacy_parse.c | 2 +-
grub-core/lib/syslinux_parse.c | 6 +++---
grub-core/loader/i386/bsd.c | 6 +++---
grub-core/loader/i386/linux.c | 2 +-
grub-core/loader/i386/pc/linux.c | 2 +-
grub-core/loader/i386/xen_fileXX.c | 2 +-
grub-core/mmap/mmap.c | 4 ++--
grub-core/net/http.c | 4 ++--
grub-core/net/net.c | 8 ++++----
| 3 +--
grub-core/osdep/aros/hostdisk.c | 2 +-
grub-core/osdep/devmapper/hostdisk.c | 2 +-
grub-core/script/execute.c | 6 +++---
grub-core/term/serial.c | 2 +-
grub-core/term/terminfo.c | 2 +-
grub-core/tests/strtoull_test.c | 2 +-
include/grub/misc.h | 24 ++++++++++++++++++++---
util/grub-fstest.c | 2 +-
39 files changed, 98 insertions(+), 77 deletions(-)
diff --git a/grub-core/commands/date.c b/grub-core/commands/date.c
index 8e1f41f141b..5cb4fafd454 100644
--- a/grub-core/commands/date.c
+++ b/grub-core/commands/date.c
@@ -59,7 +59,8 @@ grub_cmd_date (grub_command_t cmd __attribute__ ((unused)),
for (; argc; argc--, args++)
{
- char *p, c;
+ const char *p;
+ char c;
int m1, ofs, n, cur_mask;
p = args[0];
diff --git a/grub-core/commands/i386/cmostest.c b/grub-core/commands/i386/cmostest.c
index c839b704dc5..9f6b56a2f0c 100644
--- a/grub-core/commands/i386/cmostest.c
+++ b/grub-core/commands/i386/cmostest.c
@@ -27,7 +27,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
static grub_err_t
parse_args (int argc, char *argv[], int *byte, int *bit)
{
- char *rest;
+ const char *rest;
if (argc != 1)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "address required");
diff --git a/grub-core/commands/i386/pc/play.c b/grub-core/commands/i386/pc/play.c
index c8181310515..a980e46883c 100644
--- a/grub-core/commands/i386/pc/play.c
+++ b/grub-core/commands/i386/pc/play.c
@@ -132,7 +132,7 @@ grub_cmd_play (grub_command_t cmd __attribute__ ((unused)),
}
else
{
- char *end;
+ const char *end;
unsigned tempo;
struct note note;
int i;
diff --git a/grub-core/commands/i386/rdmsr.c b/grub-core/commands/i386/rdmsr.c
index 15b9adfca67..46c4346da1b 100644
--- a/grub-core/commands/i386/rdmsr.c
+++ b/grub-core/commands/i386/rdmsr.c
@@ -44,7 +44,7 @@ grub_cmd_msr_read (grub_extcmd_context_t ctxt, int argc, char **argv)
{
grub_uint32_t manufacturer[3], max_cpuid, a, b, c, features, addr;
grub_uint64_t value;
- char *ptr;
+ const char *ptr;
char buf[sizeof("1122334455667788")];
/*
diff --git a/grub-core/commands/i386/wrmsr.c b/grub-core/commands/i386/wrmsr.c
index 9c5e510eb44..fa76f5aed11 100644
--- a/grub-core/commands/i386/wrmsr.c
+++ b/grub-core/commands/i386/wrmsr.c
@@ -37,7 +37,7 @@ grub_cmd_msr_write (grub_command_t cmd __attribute__ ((unused)), int argc, char
{
grub_uint32_t manufacturer[3], max_cpuid, a, b, c, features, addr;
grub_uint64_t value;
- char *ptr;
+ const char *ptr;
/*
* The CPUID instruction should be used to determine whether MSRs
diff --git a/grub-core/commands/password_pbkdf2.c b/grub-core/commands/password_pbkdf2.c
index da636e6217a..ab845d25eb3 100644
--- a/grub-core/commands/password_pbkdf2.c
+++ b/grub-core/commands/password_pbkdf2.c
@@ -86,7 +86,7 @@ grub_cmd_password (grub_command_t cmd __attribute__ ((unused)),
int argc, char **args)
{
grub_err_t err;
- char *ptr, *ptr2;
+ const char *ptr, *ptr2;
grub_uint8_t *ptro;
struct pbkdf2_password *pass;
diff --git a/grub-core/commands/pcidump.c b/grub-core/commands/pcidump.c
index f99ad4a216f..f72628fce2d 100644
--- a/grub-core/commands/pcidump.c
+++ b/grub-core/commands/pcidump.c
@@ -95,7 +95,7 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
if (ctxt->state[0].set)
{
ptr = ctxt->state[0].arg;
- ctx.pciid_check_value |= (grub_strtoul (ptr, (char **) &ptr, 16) & 0xffff);
+ ctx.pciid_check_value |= (grub_strtoul (ptr, &ptr, 16) & 0xffff);
if (grub_errno == GRUB_ERR_BAD_NUMBER)
{
grub_errno = GRUB_ERR_NONE;
@@ -108,8 +108,7 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
if (*ptr != ':')
return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("missing `%c' symbol"), ':');
ptr++;
- ctx.pciid_check_value |= (grub_strtoul (ptr, (char **) &ptr, 16) & 0xffff)
- << 16;
+ ctx.pciid_check_value |= (grub_strtoul (ptr, &ptr, 16) & 0xffff) << 16;
if (grub_errno == GRUB_ERR_BAD_NUMBER)
grub_errno = GRUB_ERR_NONE;
else
@@ -121,10 +120,10 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
if (ctxt->state[1].set)
{
const char *optr;
-
+
ptr = ctxt->state[1].arg;
optr = ptr;
- ctx.bus = grub_strtoul (ptr, (char **) &ptr, 16);
+ ctx.bus = grub_strtoul (ptr, &ptr, 16);
if (grub_errno == GRUB_ERR_BAD_NUMBER)
{
grub_errno = GRUB_ERR_NONE;
@@ -138,7 +137,7 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("missing `%c' symbol"), ':');
ptr++;
optr = ptr;
- ctx.device = grub_strtoul (ptr, (char **) &ptr, 16);
+ ctx.device = grub_strtoul (ptr, &ptr, 16);
if (grub_errno == GRUB_ERR_BAD_NUMBER)
{
grub_errno = GRUB_ERR_NONE;
@@ -149,7 +148,7 @@ grub_cmd_pcidump (grub_extcmd_context_t ctxt,
if (*ptr == '.')
{
ptr++;
- ctx.function = grub_strtoul (ptr, (char **) &ptr, 16);
+ ctx.function = grub_strtoul (ptr, &ptr, 16);
if (grub_errno)
return grub_errno;
ctx.check_function = 1;
diff --git a/grub-core/commands/regexp.c b/grub-core/commands/regexp.c
index f00b184c81e..7c5c72fe460 100644
--- a/grub-core/commands/regexp.c
+++ b/grub-core/commands/regexp.c
@@ -64,7 +64,7 @@ set_matches (char **varnames, char *str, grub_size_t nmatches,
{
int i;
char *p;
- char *q;
+ const char * q;
grub_err_t err;
unsigned long j;
diff --git a/grub-core/commands/setpci.c b/grub-core/commands/setpci.c
index d5bc97d60b2..e966af080a6 100644
--- a/grub-core/commands/setpci.c
+++ b/grub-core/commands/setpci.c
@@ -169,7 +169,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
if (ctxt->state[0].set)
{
ptr = ctxt->state[0].arg;
- pciid_check_value |= (grub_strtoul (ptr, (char **) &ptr, 16) & 0xffff);
+ pciid_check_value |= (grub_strtoul (ptr, &ptr, 16) & 0xffff);
if (grub_errno == GRUB_ERR_BAD_NUMBER)
{
grub_errno = GRUB_ERR_NONE;
@@ -182,8 +182,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
if (*ptr != ':')
return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("missing `%c' symbol"), ':');
ptr++;
- pciid_check_value |= (grub_strtoul (ptr, (char **) &ptr, 16) & 0xffff)
- << 16;
+ pciid_check_value |= (grub_strtoul (ptr, &ptr, 16) & 0xffff) << 16;
if (grub_errno == GRUB_ERR_BAD_NUMBER)
grub_errno = GRUB_ERR_NONE;
else
@@ -197,10 +196,10 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
if (ctxt->state[1].set)
{
const char *optr;
-
+
ptr = ctxt->state[1].arg;
optr = ptr;
- bus = grub_strtoul (ptr, (char **) &ptr, 16);
+ bus = grub_strtoul (ptr, &ptr, 16);
if (grub_errno == GRUB_ERR_BAD_NUMBER)
{
grub_errno = GRUB_ERR_NONE;
@@ -214,7 +213,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("missing `%c' symbol"), ':');
ptr++;
optr = ptr;
- device = grub_strtoul (ptr, (char **) &ptr, 16);
+ device = grub_strtoul (ptr, &ptr, 16);
if (grub_errno == GRUB_ERR_BAD_NUMBER)
{
grub_errno = GRUB_ERR_NONE;
@@ -225,7 +224,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
if (*ptr == '.')
{
ptr++;
- function = grub_strtoul (ptr, (char **) &ptr, 16);
+ function = grub_strtoul (ptr, &ptr, 16);
if (grub_errno)
return grub_errno;
check_function = 1;
@@ -253,7 +252,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
if (i == ARRAY_SIZE (pci_registers))
{
regsize = 0;
- regaddr = grub_strtoul (ptr, (char **) &ptr, 16);
+ regaddr = grub_strtoul (ptr, &ptr, 16);
if (grub_errno)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "unknown register");
}
@@ -270,7 +269,7 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
if (*ptr == '+')
{
ptr++;
- regaddr += grub_strtoul (ptr, (char **) &ptr, 16);
+ regaddr += grub_strtoul (ptr, &ptr, 16);
if (grub_errno)
return grub_errno;
}
@@ -302,14 +301,14 @@ grub_cmd_setpci (grub_extcmd_context_t ctxt, int argc, char **argv)
if (*ptr == '=')
{
ptr++;
- regwrite = grub_strtoul (ptr, (char **) &ptr, 16);
+ regwrite = grub_strtoul (ptr, &ptr, 16);
if (grub_errno)
return grub_errno;
write_mask = 0xffffffff;
if (*ptr == ':')
{
ptr++;
- write_mask = grub_strtoul (ptr, (char **) &ptr, 16);
+ write_mask = grub_strtoul (ptr, &ptr, 16);
if (grub_errno)
return grub_errno;
write_mask = 0xffffffff;
diff --git a/grub-core/commands/test.c b/grub-core/commands/test.c
index 4e929e0452e..62d3fb3988f 100644
--- a/grub-core/commands/test.c
+++ b/grub-core/commands/test.c
@@ -31,7 +31,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
/* A simple implementation for signed numbers. */
static int
-grub_strtosl (char *arg, char **end, int base)
+grub_strtosl (char *arg, const char ** const end, int base)
{
if (arg[0] == '-')
return -grub_strtoul (arg + 1, end, base);
diff --git a/grub-core/commands/videoinfo.c b/grub-core/commands/videoinfo.c
index 4be8107d553..016a4d81835 100644
--- a/grub-core/commands/videoinfo.c
+++ b/grub-core/commands/videoinfo.c
@@ -136,7 +136,7 @@ grub_cmd_videoinfo (grub_command_t cmd __attribute__ ((unused)),
ctx.height = ctx.width = ctx.depth = 0;
if (argc)
{
- char *ptr;
+ const char *ptr;
ptr = args[0];
ctx.width = grub_strtoul (ptr, &ptr, 0);
if (grub_errno)
diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
index c3b578acf25..67bf37a9c3c 100644
--- a/grub-core/disk/diskfilter.c
+++ b/grub-core/disk/diskfilter.c
@@ -969,7 +969,8 @@ grub_diskfilter_vg_register (struct grub_diskfilter_vg *vg)
for (p = vgp->lvs; p; p = p->next)
{
int cur_num;
- char *num, *end;
+ char *num;
+ const char *end;
if (!p->fullname)
continue;
if (grub_strncmp (p->fullname, lv->fullname, len) != 0)
diff --git a/grub-core/disk/lvm.c b/grub-core/disk/lvm.c
index 7b265c780c3..0cbd0dd1629 100644
--- a/grub-core/disk/lvm.c
+++ b/grub-core/disk/lvm.c
@@ -38,7 +38,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
at the number. In case STR is not found, *P will be NULL and the
return value will be 0. */
static grub_uint64_t
-grub_lvm_getvalue (char **p, const char *str)
+grub_lvm_getvalue (const char ** const p, const char *str)
{
*p = grub_strstr (*p, str);
if (! *p)
@@ -63,12 +63,12 @@ grub_lvm_checkvalue (char **p, char *str, char *tmpl)
#endif
static int
-grub_lvm_check_flag (char *p, const char *str, const char *flag)
+grub_lvm_check_flag (const char *p, const char *str, const char *flag)
{
grub_size_t len_str = grub_strlen (str), len_flag = grub_strlen (flag);
while (1)
{
- char *q;
+ const char *q;
p = grub_strstr (p, str);
if (! p)
return 0;
@@ -105,7 +105,8 @@ grub_lvm_detect (grub_disk_t disk,
char buf[GRUB_LVM_LABEL_SIZE];
char vg_id[GRUB_LVM_ID_STRLEN+1];
char pv_id[GRUB_LVM_ID_STRLEN+1];
- char *metadatabuf, *p, *q, *vgname;
+ char *metadatabuf, *vgname;
+ const char *p, *q;
struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
struct grub_lvm_pv_header *pvh;
struct grub_lvm_disk_locn *dlocn;
diff --git a/grub-core/efiemu/pnvram.c b/grub-core/efiemu/pnvram.c
index c5c3d4bd3d5..dd42bc69116 100644
--- a/grub-core/efiemu/pnvram.c
+++ b/grub-core/efiemu/pnvram.c
@@ -39,7 +39,7 @@ static grub_size_t nvramsize;
/* Parse signed value */
static int
-grub_strtosl (const char *arg, char **end, int base)
+grub_strtosl (const char *arg, const char ** const end, int base)
{
if (arg[0] == '-')
return -grub_strtoul (arg + 1, end, base);
@@ -120,7 +120,8 @@ nvram_set (void * data __attribute__ ((unused)))
grub_memset (nvram, 0, nvramsize);
FOR_SORTED_ENV (var)
{
- char *guid, *attr, *name, *varname;
+ const char *guid;
+ char *attr, *name, *varname;
struct efi_variable *efivar;
int len = 0;
int i;
--git a/grub-core/gfxmenu/gui_circular_progress.c b/grub-core/gfxmenu/gui_circular_progress.c
index 354dd7b73ee..7578bfbec92 100644
--- a/grub-core/gfxmenu/gui_circular_progress.c
+++ b/grub-core/gfxmenu/gui_circular_progress.c
@@ -230,7 +230,7 @@ circprog_set_state (void *vself, int visible, int start,
static int
parse_angle (const char *value)
{
- char *ptr;
+ const char *ptr;
int angle;
angle = grub_strtol (value, &ptr, 10);
--git a/grub-core/gfxmenu/theme_loader.c b/grub-core/gfxmenu/theme_loader.c
index d6829bb5e90..eae83086bcc 100644
--- a/grub-core/gfxmenu/theme_loader.c
+++ b/grub-core/gfxmenu/theme_loader.c
@@ -484,7 +484,7 @@ parse_proportional_spec (const char *value, signed *abs, grub_fixed_signed_t *pr
ptr++;
}
- num = grub_strtoul (ptr, (char **) &ptr, 0);
+ num = grub_strtoul (ptr, &ptr, 0);
if (grub_errno)
return grub_errno;
if (sig)
diff --git a/grub-core/kern/fs.c b/grub-core/kern/fs.c
index 2b85f4950bd..88d39360da0 100644
--- a/grub-core/kern/fs.c
+++ b/grub-core/kern/fs.c
@@ -134,7 +134,7 @@ struct grub_fs_block
static grub_err_t
grub_fs_blocklist_open (grub_file_t file, const char *name)
{
- char *p = (char *) name;
+ const char *p = name;
unsigned num = 0;
unsigned i;
grub_disk_t disk = file->device->disk;
diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 76e7fb22872..b714c97b90f 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -340,7 +340,8 @@ grub_isspace (int c)
}
unsigned long
-grub_strtoul (const char *str, char **end, int base)
+grub_strtoul (const char * restrict str, const char ** const restrict end,
+ int base)
{
unsigned long long num;
@@ -357,7 +358,8 @@ grub_strtoul (const char *str, char **end, int base)
}
unsigned long long
-grub_strtoull (const char *str, char **end, int base)
+grub_strtoull (const char * restrict str, const char ** const restrict end,
+ int base)
{
unsigned long long num = 0;
int found = 0;
@@ -855,14 +857,14 @@ grub_vsnprintf_real (char *str, grub_size_t max_len, const char *fmt0,
{
if (fmt[0] == '0')
zerofill = '0';
- format1 = grub_strtoul (fmt, (char **) &fmt, 10);
+ format1 = grub_strtoul (fmt, &fmt, 10);
}
if (*fmt == '.')
fmt++;
if (grub_isdigit (*fmt))
- format2 = grub_strtoul (fmt, (char **) &fmt, 10);
+ format2 = grub_strtoul (fmt, &fmt, 10);
if (*fmt == '$')
{
diff --git a/grub-core/kern/partition.c b/grub-core/kern/partition.c
index e499147cbcb..2c401b866c4 100644
--- a/grub-core/kern/partition.c
+++ b/grub-core/kern/partition.c
@@ -126,7 +126,7 @@ grub_partition_probe (struct grub_disk *disk, const char *str)
while (*ptr && grub_isalpha (*ptr))
ptr++;
partname_end = ptr;
- num = grub_strtoul (ptr, (char **) &ptr, 0) - 1;
+ num = grub_strtoul (ptr, &ptr, 0) - 1;
curpart = 0;
/* Use the first partition map type found. */
diff --git a/grub-core/lib/arg.c b/grub-core/lib/arg.c
index fd7744a6ff6..ccc185017ee 100644
--- a/grub-core/lib/arg.c
+++ b/grub-core/lib/arg.c
@@ -375,7 +375,7 @@ grub_arg_parse (grub_extcmd_t cmd, int argc, char **argv,
case ARG_TYPE_INT:
{
- char *tail;
+ const char * tail;
grub_strtoull (option, &tail, 0);
if (tail == 0 || tail == option || *tail != '\0' || grub_errno)
diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
index 412f26f17f0..15c0d99491c 100644
--- a/grub-core/lib/json/json.c
+++ b/grub-core/lib/json/json.c
@@ -227,7 +227,7 @@ grub_json_getuint64 (grub_uint64_t *out, const grub_json_t *parent, const char *
{
grub_json_type_t type;
const char *value;
- char *end;
+ const char *end;
grub_err_t ret;
ret = get_value (&type, &value, parent, key);
@@ -249,7 +249,7 @@ grub_json_getint64 (grub_int64_t *out, const grub_json_t *parent, const char *ke
{
grub_json_type_t type;
const char *value;
- char *end;
+ const char *end;
grub_err_t ret;
ret = get_value (&type, &value, parent, key);
diff --git a/grub-core/lib/legacy_parse.c b/grub-core/lib/legacy_parse.c
index ef56150ac77..05719ab2ccb 100644
--- a/grub-core/lib/legacy_parse.c
+++ b/grub-core/lib/legacy_parse.c
@@ -418,7 +418,7 @@ adjust_file (const char *in, grub_size_t len)
}
if (*comma != ',')
return grub_legacy_escape (in, len);
- part = grub_strtoull (comma + 1, (char **) &rest, 0);
+ part = grub_strtoull (comma + 1, &rest, 0);
if (rest[0] == ',' && rest[1] >= 'a' && rest[1] <= 'z')
{
subpart = rest[1] - 'a';
diff --git a/grub-core/lib/syslinux_parse.c b/grub-core/lib/syslinux_parse.c
index 4afa99279a2..de9fda06f52 100644
--- a/grub-core/lib/syslinux_parse.c
+++ b/grub-core/lib/syslinux_parse.c
@@ -1062,7 +1062,7 @@ write_entry (struct output_buffer *outbuf,
if (ptr[0] == 'h' && ptr[1] == 'd')
{
is_fd = 0;
- devn = grub_strtoul (ptr + 2, &ptr, 0);
+ devn = grub_strtoul (ptr + 2, (const char **)&ptr, 0);
continue;
}
if (grub_strncasecmp (ptr, "file=", 5) == 0)
@@ -1086,12 +1086,12 @@ write_entry (struct output_buffer *outbuf,
if (ptr[0] == 'f' && ptr[1] == 'd')
{
is_fd = 1;
- devn = grub_strtoul (ptr + 2, &ptr, 0);
+ devn = grub_strtoul (ptr + 2, (const char **)&ptr, 0);
continue;
}
if (grub_isdigit (ptr[0]))
{
- part = grub_strtoul (ptr, &ptr, 0);
+ part = grub_strtoul (ptr, (const char **)&ptr, 0);
continue;
}
/* FIXME: isolinux, ntldr, cmldr, *dos, seg, hide
diff --git a/grub-core/loader/i386/bsd.c b/grub-core/loader/i386/bsd.c
index 3730ed38247..eb82391dbea 100644
--- a/grub-core/loader/i386/bsd.c
+++ b/grub-core/loader/i386/bsd.c
@@ -1615,7 +1615,7 @@ grub_cmd_openbsd (grub_extcmd_context_t ctxt, int argc, char *argv[])
return grub_error (GRUB_ERR_BAD_ARGUMENT,
"unknown disk type name");
- unit = grub_strtoul (arg, (char **) &arg, 10);
+ unit = grub_strtoul (arg, &arg, 10);
if (! (arg && *arg >= 'a' && *arg <= 'z'))
return grub_error (GRUB_ERR_BAD_ARGUMENT,
"only device specifications of form "
@@ -1633,7 +1633,7 @@ grub_cmd_openbsd (grub_extcmd_context_t ctxt, int argc, char *argv[])
if (ctxt->state[OPENBSD_SERIAL_ARG].set)
{
struct grub_openbsd_bootarg_console serial;
- char *ptr;
+ const char *ptr;
unsigned port = 0;
unsigned speed = 9600;
@@ -1735,7 +1735,7 @@ grub_cmd_netbsd (grub_extcmd_context_t ctxt, int argc, char *argv[])
if (ctxt->state[NETBSD_SERIAL_ARG].set)
{
struct grub_netbsd_btinfo_serial serial;
- char *ptr;
+ const char *ptr;
grub_memset (&serial, 0, sizeof (serial));
grub_strcpy (serial.devname, "com");
diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
index ee95cd374b7..ac1fae72e3b 100644
--- a/grub-core/loader/i386/linux.c
+++ b/grub-core/loader/i386/linux.c
@@ -944,7 +944,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
#endif /* GRUB_MACHINE_PCBIOS */
if (grub_memcmp (argv[i], "mem=", 4) == 0)
{
- char *val = argv[i] + 4;
+ const char *val = argv[i] + 4;
linux_mem_size = grub_strtoul (val, &val, 0);
diff --git a/grub-core/loader/i386/pc/linux.c b/grub-core/loader/i386/pc/linux.c
index 47ea2945e4f..f56af7c501f 100644
--- a/grub-core/loader/i386/pc/linux.c
+++ b/grub-core/loader/i386/pc/linux.c
@@ -259,7 +259,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
}
else if (grub_memcmp (argv[i], "mem=", 4) == 0)
{
- char *val = argv[i] + 4;
+ const char *val = argv[i] + 4;
linux_mem_size = grub_strtoul (val, &val, 0);
diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c
index 6329ec01038..27afcaacbce 100644
--- a/grub-core/loader/i386/xen_fileXX.c
+++ b/grub-core/loader/i386/xen_fileXX.c
@@ -25,7 +25,7 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info *xi,
grub_off_t off, grub_size_t sz)
{
char *buf;
- char *ptr;
+ const char *ptr;
int has_paddr = 0;
grub_errno = GRUB_ERR_NONE;
diff --git a/grub-core/mmap/mmap.c b/grub-core/mmap/mmap.c
index 6a31cbae325..b569cb23b5a 100644
--- a/grub-core/mmap/mmap.c
+++ b/grub-core/mmap/mmap.c
@@ -423,7 +423,7 @@ static grub_err_t
grub_cmd_badram (grub_command_t cmd __attribute__ ((unused)),
int argc, char **args)
{
- char * str;
+ const char *str;
struct badram_entry entry;
if (argc != 1)
@@ -465,7 +465,7 @@ static grub_uint64_t
parsemem (const char *str)
{
grub_uint64_t ret;
- char *ptr;
+ const char *ptr;
ret = grub_strtoul (str, &ptr, 0);
diff --git a/grub-core/net/http.c b/grub-core/net/http.c
index ec3647f9a0c..b616cf40b1e 100644
--- a/grub-core/net/http.c
+++ b/grub-core/net/http.c
@@ -107,7 +107,7 @@ parse_line (grub_file_t file, http_data_t data, char *ptr, grub_size_t len)
return GRUB_ERR_NONE;
}
ptr += sizeof ("HTTP/1.1 ") - 1;
- code = grub_strtoul (ptr, &ptr, 10);
+ code = grub_strtoul (ptr, (const char **)&ptr, 10);
if (grub_errno)
return grub_errno;
switch (code)
@@ -134,7 +134,7 @@ parse_line (grub_file_t file, http_data_t data, char *ptr, grub_size_t len)
== 0 && !data->size_recv)
{
ptr += sizeof ("Content-Length: ") - 1;
- file->size = grub_strtoull (ptr, &ptr, 10);
+ file->size = grub_strtoull (ptr, (const char **)&ptr, 10);
data->size_recv = 1;
return GRUB_ERR_NONE;
}
diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index f82ac15b4ec..c42f0f4f71d 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -406,7 +406,7 @@ parse_ip (const char *val, grub_uint32_t *ip, const char **rest)
for (i = 0; i < 4; i++)
{
unsigned long t;
- t = grub_strtoul (ptr, (char **) &ptr, 0);
+ t = grub_strtoul (ptr, &ptr, 0);
if (grub_errno)
{
grub_errno = GRUB_ERR_NONE;
@@ -453,7 +453,7 @@ parse_ip6 (const char *val, grub_uint64_t *ip, const char **rest)
ptr++;
continue;
}
- t = grub_strtoul (ptr, (char **) &ptr, 16);
+ t = grub_strtoul (ptr, &ptr, 16);
if (grub_errno)
{
grub_errno = GRUB_ERR_NONE;
@@ -563,7 +563,7 @@ grub_net_resolve_net_address (const char *name,
addr->type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV4;
if (*rest == '/')
{
- addr->ipv4.masksize = grub_strtoul (rest + 1, (char **) &rest, 0);
+ addr->ipv4.masksize = grub_strtoul (rest + 1, &rest, 0);
if (!grub_errno && *rest == 0)
return GRUB_ERR_NONE;
grub_errno = GRUB_ERR_NONE;
@@ -579,7 +579,7 @@ grub_net_resolve_net_address (const char *name,
addr->type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
if (*rest == '/')
{
- addr->ipv6.masksize = grub_strtoul (rest + 1, (char **) &rest, 0);
+ addr->ipv6.masksize = grub_strtoul (rest + 1, &rest, 0);
if (!grub_errno && *rest == 0)
return GRUB_ERR_NONE;
grub_errno = GRUB_ERR_NONE;
--git a/grub-core/normal/menu.c b/grub-core/normal/menu.c
index d5e0c79a70e..da66ad89180 100644
--- a/grub-core/normal/menu.c
+++ b/grub-core/normal/menu.c
@@ -170,8 +170,7 @@ grub_menu_set_timeout (int timeout)
static int
get_and_remove_first_entry_number (const char *name)
{
- const char *val;
- char *tail;
+ const char *val, *tail;
int entry;
val = grub_env_get (name);
diff --git a/grub-core/osdep/aros/hostdisk.c b/grub-core/osdep/aros/hostdisk.c
index 2be654ca3bd..3b2c9de2496 100644
--- a/grub-core/osdep/aros/hostdisk.c
+++ b/grub-core/osdep/aros/hostdisk.c
@@ -194,7 +194,7 @@ grub_util_fd_open (const char *dev, int flg)
p1 = dev + strlen (dev);
else
{
- unit = grub_strtoul (p1 + 1, (char **) &p2, 16);
+ unit = grub_strtoul (p1 + 1, &p2, 16);
if (p2 && *p2 == '/')
flags = grub_strtoul (p2 + 1, 0, 16);
}
diff --git a/grub-core/osdep/devmapper/hostdisk.c b/grub-core/osdep/devmapper/hostdisk.c
index a697bcb4d8d..a8afc0c9408 100644
--- a/grub-core/osdep/devmapper/hostdisk.c
+++ b/grub-core/osdep/devmapper/hostdisk.c
@@ -113,7 +113,7 @@ grub_util_get_dm_node_linear_info (dev_t dev,
void *next = NULL;
uint64_t length, start;
char *target, *params;
- char *ptr;
+ const char *ptr;
int major = 0, minor = 0;
int first = 1;
grub_disk_addr_t partstart = 0;
diff --git a/grub-core/script/execute.c b/grub-core/script/execute.c
index ee299fd0ea6..8cc61ee7d78 100644
--- a/grub-core/script/execute.c
+++ b/grub-core/script/execute.c
@@ -122,7 +122,7 @@ replace_scope (struct grub_script_scope *new_scope)
grub_err_t
grub_script_break (grub_command_t cmd, int argc, char *argv[])
{
- char *p = 0;
+ const char *p = NULL;
unsigned long count;
if (argc == 0)
@@ -154,7 +154,7 @@ grub_err_t
grub_script_shift (grub_command_t cmd __attribute__((unused)),
int argc, char *argv[])
{
- char *p = 0;
+ const char *p = NULL;
unsigned long n = 0;
if (! scope)
@@ -215,7 +215,7 @@ grub_err_t
grub_script_return (grub_command_t cmd __attribute__((unused)),
int argc, char *argv[])
{
- char *p;
+ const char *p = NULL;
unsigned long n;
if (! scope || argc > 1)
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index db80b3ba0fb..f9271b09239 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -269,7 +269,7 @@ grub_cmd_serial (grub_extcmd_context_t ctxt, int argc, char **args)
if (state[OPTION_BASE_CLOCK].set)
{
- char *ptr;
+ const char *ptr;
config.base_clock = grub_strtoull (state[OPTION_BASE_CLOCK].arg, &ptr, 0);
if (grub_errno)
return grub_errno;
diff --git a/grub-core/term/terminfo.c b/grub-core/term/terminfo.c
index d317efa368d..0e9de7f8fa9 100644
--- a/grub-core/term/terminfo.c
+++ b/grub-core/term/terminfo.c
@@ -737,7 +737,7 @@ grub_cmd_terminfo (grub_extcmd_context_t ctxt, int argc, char **args)
if (state[OPTION_GEOMETRY].set)
{
- char *ptr = state[OPTION_GEOMETRY].arg;
+ const char *ptr = state[OPTION_GEOMETRY].arg;
w = grub_strtoul (ptr, &ptr, 0);
if (grub_errno)
return grub_errno;
diff --git a/grub-core/tests/strtoull_test.c b/grub-core/tests/strtoull_test.c
index 7da615ff33e..5488ab26b43 100644
--- a/grub-core/tests/strtoull_test.c
+++ b/grub-core/tests/strtoull_test.c
@@ -25,7 +25,7 @@ static void
strtoull_testcase (const char *input, int base, unsigned long long expected,
int num_digits, grub_err_t error)
{
- char *output;
+ const char *output;
unsigned long long value;
grub_errno = 0;
value = grub_strtoull(input, &output, base);
diff --git a/include/grub/misc.h b/include/grub/misc.h
index ee48eb7a726..cd5a8b4ae82 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -243,11 +243,29 @@ grub_strncasecmp (const char *s1, const char *s2, grub_size_t n)
- (int) grub_tolower ((grub_uint8_t) *s2);
}
-unsigned long EXPORT_FUNC(grub_strtoul) (const char *str, char **end, int base);
-unsigned long long EXPORT_FUNC(grub_strtoull) (const char *str, char **end, int base);
+/*
+ * Note that these differ from the C standard's definitions of strtol,
+ * strtoul(), and strtoull() by the addition of two const qualifiers on the end
+ * pointer, which make the declaration match the *semantic* requirements of
+ * their behavior. This means that instead of:
+ *
+ * char *s = "1234 abcd";
+ * char *end;
+ * unsigned long l;
+ *
+ * l = grub_strtoul(s, &end, 10);
+ *
+ * We must one of:
+ *
+ * const char *end;
+ * ... or ...
+ * l = grub_strtoul(s, (const char ** const)&end, 10);
+ */
+unsigned long EXPORT_FUNC(grub_strtoul) (const char * restrict str, const char ** const restrict end, int base);
+unsigned long long EXPORT_FUNC(grub_strtoull) (const char * restrict str, const char ** const restrict end, int base);
static inline long
-grub_strtol (const char *str, char **end, int base)
+grub_strtol (const char * restrict str, const char ** const restrict end, int base)
{
int negative = 0;
unsigned long long magnitude;
diff --git a/util/grub-fstest.c b/util/grub-fstest.c
index f14e02d9727..7f14620cd49 100644
--- a/util/grub-fstest.c
+++ b/util/grub-fstest.c
@@ -538,7 +538,7 @@ void (*argp_program_version_hook) (FILE *, struct argp_state *) = print_version;
static error_t
argp_parser (int key, char *arg, struct argp_state *state)
{
- char *p;
+ const char *p;
switch (key)
{
--
2.24.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Make grub_strtol() "end" pointers have safer const qualifiers. (v2)
2020-02-21 21:39 ` [PATCH] Make grub_strtol() "end" pointers have safer const qualifiers. (v2) Peter Jones
@ 2020-02-24 9:42 ` Daniel Kiper
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Kiper @ 2020-02-24 9:42 UTC (permalink / raw)
To: Peter Jones; +Cc: grub-devel
On Fri, Feb 21, 2020 at 04:39:33PM -0500, Peter Jones wrote:
> Currently the string functions grub_strtol(), grub_strtoul(), and
> grub_strtoull() don't declare the "end" pointer in such a way as to
> require the pointer itself or the character array to be immutable to the
> implementation, nor does the C standard do so in its similar functions,
> though it does require us not to change any of it.
>
> The typical declarations of these functions follow this pattern:
>
> long
> strtol(const char * restrict nptr, char ** restrict endptr, int base);
>
> Much of the reason for this is historic, and a discussion of that
> follows below, after the explanation of this change. (GRUB currently
> does not include the "restrict" qualifiers, and we name the arguments a
> bit differently.)
>
> The implementation is semantically required to treat the character array
> as immutable, but such accidental modifications aren't stopped by the
> compiler, and the semantics for both the callers and the implementation
> of these functions are sometimes also helped by adding that requirement.
>
> This patch changes these declarations to follow this pattern instead:
>
> long
> strtol(const char * restrict nptr,
> const char ** const restrict endptr,
> int base);
>
> This means that if any modification to these functions accidentally
> introduces either an errant modification to the underlying character
> array, or an accidental assignment to endptr rather than *endptr, the
> compiler should generate an error. (The two uses of "restrict" in this
> case basically mean strtol() isn't allowed to modify the character array
> by going through *endptr, and endptr isn't allowed to point inside the
> array.)
>
> It also means the typical use case changes to:
>
> char *s = ...;
> const char *end;
> long l;
>
> l = strtol(s, &end, 10);
>
> Or even:
>
> const char *p = str;
> while (p && *p) {
> long l = strtol(p, &p, 10);
> ...
> }
>
> This fixes 26 places where we discard our attempts at treating the data
> safely by doing:
>
> const char *p = str;
> long l;
>
> l = strtol(p, (char **)&ptr, 10);
>
> It also adds 5 places where we do:
>
> char *p = str;
> while (p && *p) {
> long l = strtol(p, (const char ** const)&p, 10);
> ...
> /* more calls that need p not to be pointer-to-const */
> }
>
> While moderately distasteful, this is a better problem to have.
>
> With one minor exception, I have tested that all of this compiles
> without relevant warnings or errors, and that /much/ of it behaves
> correctly, with gcc 9 using 'gcc -W -Wall -Wextra'. The one exception
> is the changes in grub-core/osdep/aros/hostdisk.c , which I have no idea
> how to build.
>
> Because the C standard defined type-qualifiers in a way that can be
> confusing, in the past there's been a slow but fairly regular stream of
> churn within our patches, which add and remove the const qualifier in many
> of the users of these functions. This change should help avoid that in
> the future, and in order to help ensure this, I've added an explanation
> in misc.h so that when someone does get a compiler warning about a type
> error, they have the fix at hand.
>
> The reason we don't have "const" in these calls in the standard is
> purely anachronistic: C78 (de facto) did not have type qualifiers in the
> syntax, and the "const" type qualifier was added for C89 (I think; it
> may have been later). strtol() appears to date from 4.3BSD in 1986,
> which means it could not be added to those functions in the standard
> without breaking compatibility, which is usually avoided.
>
> The syntax chosen for type qualifiers is what has led to the churn
> regarding usage of const, and is especially confusing on string
> functions due to the lack of a string type. Quoting from C99, the
> syntax is:
>
> declarator:
> pointer[opt] direct-declarator
> direct-declarator:
> identifier
> ( declarator )
> direct-declarator [ type-qualifier-list[opt] assignment-expression[opt] ]
> ...
> direct-declarator [ type-qualifier-list[opt] * ]
> ...
> pointer:
> * type-qualifier-list[opt]
> * type-qualifier-list[opt] pointer
> type-qualifier-list:
> type-qualifier
> type-qualifier-list type-qualifier
> ...
> type-qualifier:
> const
> restrict
> volatile
>
> So the examples go like:
>
> const char foo; // immutable object
> const char *foo; // mutable pointer to object
> char * const foo; // immutable pointer to mutable object
> const char * const foo; // immutable pointer to immutable object
> const char const * const foo; // XXX extra const keyword in the middle
> const char * const * const foo; // immutable pointer to immutable
> // pointer to immutable object
> const char ** const foo; // immutable pointer to mutable pointer
> // to immutable object
>
> Making const left-associative for * and right-associative for everything
> else may not have been the best choice ever, but here we are, and the
> inevitable result is people using trying to use const (as they should!),
> putting it at the wrong place, fighting with the compiler for a bit, and
> then either removing it or typecasting something in a bad way. I won't
> go into describing restrict, but its syntax has exactly the same issue
> as with const.
>
> Anyway, the last example above actually represents the *behavior* that's
> required of strtol()-like functions, so that's our choice for the "end"
> pointer.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
I went through all the discussions for earlier version and v2 commit
message. Even though it is not perfect solution I think the benefits
outweigh the losses. So, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
If there are no strong technical objections I will push the patch by the
end of this week.
Daniel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-02-24 9:43 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-04 21:02 [PATCH] Make grub_strtoul() "end" pointer have the right const params Peter Jones
2020-02-05 1:04 ` Nicholas Vinson
2020-02-19 0:32 ` Peter Jones
2020-02-19 3:39 ` Nicholas Vinson
2020-02-21 21:35 ` Peter Jones
2020-02-21 21:39 ` [PATCH] Make grub_strtol() "end" pointers have safer const qualifiers. (v2) Peter Jones
2020-02-24 9:42 ` Daniel Kiper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.