Richard Henderson <richard.hender...@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.ben...@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
