diff mbox series

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

Message ID 20220315121251.2280317-8-alex.bennee@linaro.org
State Superseded
Headers show
Series misc testing, i386, docs, gitdm, gitlab | expand

Commit Message

Alex Bennée March 15, 2022, 12:12 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé March 15, 2022, 12:31 p.m. UTC | #1
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;
Alex Bennée March 15, 2022, 1:59 p.m. UTC | #2
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;
Daniel P. Berrangé March 15, 2022, 2:15 p.m. UTC | #3
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
Peter Maydell March 15, 2022, 2:20 p.m. UTC | #4
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
Philippe Mathieu-Daudé March 21, 2022, 9:55 p.m. UTC | #5
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 mbox series

Patch

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;