Re: [PATCH v1 7/8] semihosting: clean up handling of expanded argv

[Peter Maydell]

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