Re: [PATCH v2 10/12] gdbstub: refactor get_feature_xml

[Richard Henderson <richard.henderson@linaro.org>]

On 8/24/23 09:39, Alex Bennée wrote:
> Try to bring up the code to more modern standards by:
> 
>    - use dynamic GString built xml over a fixed buffer
>    - use autofree to save on explicit g_free() calls
>    - don't hand hack strstr to find the delimiter
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - avoid needless g_strndup for copy of annex
> ---
>   gdbstub/internals.h |  2 +-
>   gdbstub/gdbstub.c   | 63 +++++++++++++++++++++------------------------
>   2 files changed, 31 insertions(+), 34 deletions(-)
> 
> diff --git a/gdbstub/internals.h b/gdbstub/internals.h
> index f2b46cce41..4876ebd74f 100644
> --- a/gdbstub/internals.h
> +++ b/gdbstub/internals.h
> @@ -33,7 +33,7 @@ typedef struct GDBProcess {
>       uint32_t pid;
>       bool attached;
>   
> -    char target_xml[1024];
> +    char *target_xml;
>   } GDBProcess;
>   
>   enum RSState {
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 8e9bc17e07..31a2451f27 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -354,54 +354,51 @@ static CPUState *gdb_get_cpu(uint32_t pid, uint32_t tid)
>   static const char *get_feature_xml(const char *p, const char **newp,
>                                      GDBProcess *process)
>   {
> -    size_t len;
>       int i;
>       const char *name;
>       CPUState *cpu = gdb_get_first_cpu_in_process(process);
>       CPUClass *cc = CPU_GET_CLASS(cpu);
>   
> -    len = 0;
> -    while (p[len] && p[len] != ':')
> -        len++;
> -    *newp = p + len;
> +    /* ‘qXfer:features:read:annex:offset,length' */

This is misleading, because "...:read:" has already been consumed.

> +    char *term = g_strstr_len(p, -1, ":");

This is strchr(p, ':').

> +    g_autofree char *annex = g_strndup(p, len);
> +    /* leave newp at offset,length for the rest of the params */
> +    *newp = term + 1;
>   
> -    name = NULL;
> -    if (strncmp(p, "target.xml", len) == 0) {
> -        char *buf = process->target_xml;
> -        const size_t buf_sz = sizeof(process->target_xml);
>   
> -        /* Generate the XML description for this CPU.  */
> -        if (!buf[0]) {
> +    name = NULL;
> +    if (g_strcmp0(annex, "target.xml") == 0) {

Why the change to g_strcmp0?  There's no null pointer to be handled.
If you keep the strncmp, you don't have to allocate memory early.

>       if (cc->gdb_get_dynamic_xml) {
> -        char *xmlname = g_strndup(p, len);
> -        const char *xml = cc->gdb_get_dynamic_xml(cpu, xmlname);
> -
> -        g_free(xmlname);
> +        const char *xml = cc->gdb_get_dynamic_xml(cpu, annex);

You can leave the g_strndup (and autofree) to here.



r~