| Message ID | 20220315121251.2280317-8-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | misc testing, i386, docs, gitdm, gitlab | expand |
On 15/3/22 13:12, Alex Bennée wrote: > Another cleanup patch tripped over the fact we weren't being careful > in our casting. Fix the casts, allow for a non-const and switch from > g_realloc to g_renew. > > The whole semihosting argument handling could do with some tests > though. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > semihosting/config.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/semihosting/config.c b/semihosting/config.c > index 137171b717..50d82108e6 100644 > --- a/semihosting/config.c > +++ b/semihosting/config.c > @@ -51,7 +51,7 @@ typedef struct SemihostingConfig { > bool enabled; > SemihostingTarget target; > Chardev *chardev; > - const char **argv; > + char **argv; > int argc; > const char *cmdline; /* concatenated argv */ > } SemihostingConfig; > @@ -98,8 +98,8 @@ static int add_semihosting_arg(void *opaque, > if (strcmp(name, "arg") == 0) { > s->argc++; > /* one extra element as g_strjoinv() expects NULL-terminated array */ > - s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *)); > - s->argv[s->argc - 1] = val; > + s->argv = g_renew(char *, s->argv, s->argc + 1); > + s->argv[s->argc - 1] = g_strdup(val); Why strdup()? > s->argv[s->argc] = NULL; > } > return 0;
Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> writes: > On 15/3/22 13:12, Alex Bennée wrote: >> Another cleanup patch tripped over the fact we weren't being careful >> in our casting. Fix the casts, allow for a non-const and switch from >> g_realloc to g_renew. >> The whole semihosting argument handling could do with some tests >> though. >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> semihosting/config.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> diff --git a/semihosting/config.c b/semihosting/config.c >> index 137171b717..50d82108e6 100644 >> --- a/semihosting/config.c >> +++ b/semihosting/config.c >> @@ -51,7 +51,7 @@ typedef struct SemihostingConfig { >> bool enabled; >> SemihostingTarget target; >> Chardev *chardev; >> - const char **argv; >> + char **argv; >> int argc; >> const char *cmdline; /* concatenated argv */ >> } SemihostingConfig; >> @@ -98,8 +98,8 @@ static int add_semihosting_arg(void *opaque, >> if (strcmp(name, "arg") == 0) { >> s->argc++; >> /* one extra element as g_strjoinv() expects NULL-terminated array */ >> - s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *)); >> - s->argv[s->argc - 1] = val; >> + s->argv = g_renew(char *, s->argv, s->argc + 1); >> + s->argv[s->argc - 1] = g_strdup(val); > > Why strdup()? The compiler was having issues with adding a const char * into the array and it was the quickest way to stop it complaining. I'm not sure what guarantees you can make about a const char * after you leave the scope of the function. > >> s->argv[s->argc] = NULL; >> } >> return 0;
On Tue, Mar 15, 2022 at 01:59:59PM +0000, Alex Bennée wrote: > > Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> writes: > > > On 15/3/22 13:12, Alex Bennée wrote: > >> Another cleanup patch tripped over the fact we weren't being careful > >> in our casting. Fix the casts, allow for a non-const and switch from > >> g_realloc to g_renew. > >> The whole semihosting argument handling could do with some tests > >> though. > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > >> --- > >> semihosting/config.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> diff --git a/semihosting/config.c b/semihosting/config.c > >> index 137171b717..50d82108e6 100644 > >> --- a/semihosting/config.c > >> +++ b/semihosting/config.c > >> @@ -51,7 +51,7 @@ typedef struct SemihostingConfig { > >> bool enabled; > >> SemihostingTarget target; > >> Chardev *chardev; > >> - const char **argv; > >> + char **argv; > >> int argc; > >> const char *cmdline; /* concatenated argv */ > >> } SemihostingConfig; > >> @@ -98,8 +98,8 @@ static int add_semihosting_arg(void *opaque, > >> if (strcmp(name, "arg") == 0) { > >> s->argc++; > >> /* one extra element as g_strjoinv() expects NULL-terminated array */ > >> - s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *)); > >> - s->argv[s->argc - 1] = val; > >> + s->argv = g_renew(char *, s->argv, s->argc + 1); > >> + s->argv[s->argc - 1] = g_strdup(val); > > > > Why strdup()? > > The compiler was having issues with adding a const char * into the array > and it was the quickest way to stop it complaining. I'm not sure what > guarantees you can make about a const char * after you leave the scope > of the function. No guarantees at all. This method was implicitly relying on the caller never free'ing the const arg it passed in. That is indeed the case here, because the arg came from a QemuOpts list. It is bad practice to rely on such things though, so adding the strdup is sane IMHO. I would have split the strdup out from the realloc -> renew change though, since it is an independent cleanup. Regards, Daniel
On Tue, 15 Mar 2022 at 14:16, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Tue, Mar 15, 2022 at 01:59:59PM +0000, Alex Bennée wrote: > > > > Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> writes: > > > > > On 15/3/22 13:12, Alex Bennée wrote: > > >> Another cleanup patch tripped over the fact we weren't being careful > > >> in our casting. Fix the casts, allow for a non-const and switch from > > >> g_realloc to g_renew. > > >> The whole semihosting argument handling could do with some tests > > >> though. > > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > >> --- > > >> semihosting/config.c | 6 +++--- > > >> 1 file changed, 3 insertions(+), 3 deletions(-) > > >> diff --git a/semihosting/config.c b/semihosting/config.c > > >> index 137171b717..50d82108e6 100644 > > >> --- a/semihosting/config.c > > >> +++ b/semihosting/config.c > > >> @@ -51,7 +51,7 @@ typedef struct SemihostingConfig { > > >> bool enabled; > > >> SemihostingTarget target; > > >> Chardev *chardev; > > >> - const char **argv; > > >> + char **argv; > > >> int argc; > > >> const char *cmdline; /* concatenated argv */ > > >> } SemihostingConfig; > > >> @@ -98,8 +98,8 @@ static int add_semihosting_arg(void *opaque, > > >> if (strcmp(name, "arg") == 0) { > > >> s->argc++; > > >> /* one extra element as g_strjoinv() expects NULL-terminated array */ > > >> - s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *)); > > >> - s->argv[s->argc - 1] = val; > > >> + s->argv = g_renew(char *, s->argv, s->argc + 1); > > >> + s->argv[s->argc - 1] = g_strdup(val); > > > > > > Why strdup()? > > > > The compiler was having issues with adding a const char * into the array > > and it was the quickest way to stop it complaining. I'm not sure what > > guarantees you can make about a const char * after you leave the scope > > of the function. > > No guarantees at all. This method was implicitly relying on the caller > never free'ing the const arg it passed in. That is indeed the case here, > because the arg came from a QemuOpts list. It is bad practice to rely > on such things though, so adding the strdup is sane IMHO. > > I would have split the strdup out from the realloc -> renew change > though, since it is an independent cleanup. If we ever move the glib minimum-version up to 2.68, we could use the g_strv_builder_new()/g_strv_builder_add() APIs in glib which do exactly this job of "build up a NULL-terminated array of strings, one string at a time". -- PMM
On 15/3/22 15:15, Daniel P. Berrangé wrote: > On Tue, Mar 15, 2022 at 01:59:59PM +0000, Alex Bennée wrote: >> >> Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> writes: >> >>> On 15/3/22 13:12, Alex Bennée wrote: >>>> Another cleanup patch tripped over the fact we weren't being careful >>>> in our casting. Fix the casts, allow for a non-const and switch from >>>> g_realloc to g_renew. >>>> The whole semihosting argument handling could do with some tests >>>> though. >>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>>> --- >>>> semihosting/config.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> diff --git a/semihosting/config.c b/semihosting/config.c >>>> index 137171b717..50d82108e6 100644 >>>> --- a/semihosting/config.c >>>> +++ b/semihosting/config.c >>>> @@ -51,7 +51,7 @@ typedef struct SemihostingConfig { >>>> bool enabled; >>>> SemihostingTarget target; >>>> Chardev *chardev; >>>> - const char **argv; >>>> + char **argv; >>>> int argc; >>>> const char *cmdline; /* concatenated argv */ >>>> } SemihostingConfig; >>>> @@ -98,8 +98,8 @@ static int add_semihosting_arg(void *opaque, >>>> if (strcmp(name, "arg") == 0) { >>>> s->argc++; >>>> /* one extra element as g_strjoinv() expects NULL-terminated array */ >>>> - s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *)); >>>> - s->argv[s->argc - 1] = val; >>>> + s->argv = g_renew(char *, s->argv, s->argc + 1); >>>> + s->argv[s->argc - 1] = g_strdup(val); >>> >>> Why strdup()? >> >> The compiler was having issues with adding a const char * into the array >> and it was the quickest way to stop it complaining. I'm not sure what >> guarantees you can make about a const char * after you leave the scope >> of the function. > > No guarantees at all. This method was implicitly relying on the caller > never free'ing the const arg it passed in. That is indeed the case here, > because the arg came from a QemuOpts list. It is bad practice to rely > on such things though, so adding the strdup is sane IMHO. I see, thanks. Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
diff --git a/semihosting/config.c b/semihosting/config.c index 137171b717..50d82108e6 100644 --- a/semihosting/config.c +++ b/semihosting/config.c @@ -51,7 +51,7 @@ typedef struct SemihostingConfig { bool enabled; SemihostingTarget target; Chardev *chardev; - const char **argv; + char **argv; int argc; const char *cmdline; /* concatenated argv */ } SemihostingConfig; @@ -98,8 +98,8 @@ static int add_semihosting_arg(void *opaque, if (strcmp(name, "arg") == 0) { s->argc++; /* one extra element as g_strjoinv() expects NULL-terminated array */ - s->argv = g_realloc(s->argv, (s->argc + 1) * sizeof(void *)); - s->argv[s->argc - 1] = val; + s->argv = g_renew(char *, s->argv, s->argc + 1); + s->argv[s->argc - 1] = g_strdup(val); s->argv[s->argc] = NULL; } return 0;
Another cleanup patch tripped over the fact we weren't being careful in our casting. Fix the casts, allow for a non-const and switch from g_realloc to g_renew. The whole semihosting argument handling could do with some tests though. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- semihosting/config.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)