On Tue, 15 Mar 2022 at 14:16, Daniel P. Berrangé <berra...@redhat.com> wrote:
>
> On Tue, Mar 15, 2022 at 01:59:59PM +0000, Alex Bennée wrote:
> >
> > Philippe Mathieu-Daudé <philippe.mathieu.da...@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.ben...@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
