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

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> 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.

I figured by extracting annex upfront I would simplify the checks
further down. Anyway reverted and more clean-up applied.

>
>>       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~


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro