| Message ID | 20230824163910.1737079-11-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gdbstub and testing fixes for 8.2 | expand |
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~
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~
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' */ + char *term = g_strstr_len(p, -1, ":"); + size_t len = term - 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) { + if (!process->target_xml) { GDBRegisterState *r; + GString *xml = g_string_new("<?xml version=\"1.0\"?>"); + + g_string_append(xml, + "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">" + "<target>"); - pstrcat(buf, buf_sz, - "<?xml version=\"1.0\"?>" - "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">" - "<target>"); if (cc->gdb_arch_name) { - gchar *arch = cc->gdb_arch_name(cpu); - pstrcat(buf, buf_sz, "<architecture>"); - pstrcat(buf, buf_sz, arch); - pstrcat(buf, buf_sz, "</architecture>"); - g_free(arch); + g_autofree gchar *arch = cc->gdb_arch_name(cpu); + g_string_append_printf(xml, + "<architecture>%s</architecture>", + arch); } - pstrcat(buf, buf_sz, "<xi:include href=\""); - pstrcat(buf, buf_sz, cc->gdb_core_xml_file); - pstrcat(buf, buf_sz, "\"/>"); + g_string_append(xml, "<xi:include href=\""); + g_string_append(xml, cc->gdb_core_xml_file); + g_string_append(xml, "\"/>"); for (r = cpu->gdb_regs; r; r = r->next) { - pstrcat(buf, buf_sz, "<xi:include href=\""); - pstrcat(buf, buf_sz, r->xml); - pstrcat(buf, buf_sz, "\"/>"); + g_string_append(xml, "<xi:include href=\""); + g_string_append(xml, r->xml); + g_string_append(xml, "\"/>"); } - pstrcat(buf, buf_sz, "</target>"); + g_string_append(xml, "</target>"); + + process->target_xml = g_string_free(xml, false); + return process->target_xml; } - return buf; } 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); if (xml) { return xml; } @@ -2245,6 +2242,6 @@ void gdb_create_default_process(GDBState *s) process = &s->processes[s->process_num - 1]; process->pid = pid; process->attached = false; - process->target_xml[0] = '\0'; + process->target_xml = NULL; }
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(-)