On 11/19/21 14:41, Jinoh Kang wrote:
+struct qxfer +{
- const char* name;
- enum packet_return (*handler)(struct gdb_context* gdbctx);
+} qxfer_handlers[] = +{
- {"libraries", packet_query_libraries},
- {"threads" , packet_query_threads },
- {"features" , packet_query_features },
+};
+static BOOL parse_xfer_read(const char* ptr, char* name_buf, char* annex_buf, unsigned int* offp, unsigned int* lenp) +{
- int n;
- /* Xfer:<object>:read:<annex>:<offset-hex>,<length-hex> */
- if (sscanf(ptr, "Xfer:%" STRINGIFY(MAX_QX_NAME_LEN) "[^:]:read:%n", name_buf, &n) != 1)
return FALSE;- ptr += (unsigned int)n;
- if (sscanf(ptr, "%" STRINGIFY(MAX_QX_ANNEX_LEN) "[^:]%n", annex_buf, &n) != 1)
- {
annex_buf[0] = '\0';n = 0;- }
- ptr += (unsigned int)n;
- return sscanf(ptr, ":%x,%x", offp, lenp) == 2;
+}
I feel that this is much more complicated than it needs to be, expecially with the stringify to get buffer lengths. We usually just use MAX_PATH uness we really need something bigger, and then hardcoding the length in sscanf doesn't seem too bad.
Something like this below instead of this helper should be more readable IMHO:
char object_name[MAX_PATH] = {0}, annex[MAX_PATH] = {0};
/* [...] */
if (sscanf(gdbctx->in_packet, "Xfer:%256[^:]:read:%256[^:]:%x,%x", object_name, annex, &off, &len) == 4 || sscanf(gdbctx->in_packet, "Xfer:%256[^:]:read::%x,%x", object_name, &off, &len) == 3)
static enum packet_return packet_query(struct gdb_context* gdbctx) {
- char object_name[QX_NAME_SIZE], annex[QX_ANNEX_SIZE]; unsigned int off, len;
struct backend_cpu *cpu;
switch (gdbctx->in_packet[0]) {
@@ -2008,11 +2082,16 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) return packet_ok; if (strncmp(gdbctx->in_packet, "Supported", 9) == 0) {
size_t i;packet_reply_open(gdbctx); packet_reply_add(gdbctx, "QStartNoAckMode+;");
packet_reply_add(gdbctx, "qXfer:libraries:read+;");packet_reply_add(gdbctx, "qXfer:threads:read+;");packet_reply_add(gdbctx, "qXfer:features:read+;");
for (i = 0; i < ARRAY_SIZE(qxfer_handlers); i++){packet_reply_add(gdbctx, "qXfer:");packet_reply_add(gdbctx, qxfer_handlers[i].name);packet_reply_add(gdbctx, ":read+;");} packet_reply_close(gdbctx); return packet_done; }@@ -2043,53 +2122,47 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) } break; case 'X':
if (sscanf(gdbctx->in_packet, "Xfer:libraries:read::%x,%x", &off, &len) == 2)
if (parse_xfer_read(gdbctx->in_packet, object_name, annex, &off, &len)) {enum packet_return result;int i; BOOL more;
if (!gdbctx->process) return packet_error;
for (i = 0; ; i++){if (i >= ARRAY_SIZE(qxfer_handlers)){ERR("unhandled qXfer %s read %s %u,%u\n", debugstr_a(object_name), debugstr_a(annex), off, len);return packet_error;}if (strncmp(qxfer_handlers[i].name, object_name, QX_NAME_SIZE) == 0)break;}
I usually prefer the pattern where the loop does lookup and break, and the outcome is checked outside, like:
for (i = 0; i < ARRAY_SIZE(qxfer_handlers); i++) if (!strcmp(qxfer_handlers[i].name, object_name)) break; if (i == ARRAY_SIZE(qxfer_handlers)) { ERR("unhandled qXfer %s read %s %u,%u\n", debugstr_a(object_name), debugstr_a(annex), off, len); return packet_error; }
if (sscanf(gdbctx->in_packet, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2){BOOL more;
gdbctx->qxfer_object_idx = i;lstrcpynA(gdbctx->qxfer_object_annex, annex, QX_ANNEX_SIZE);
I think you can probably assume zero-terminated strings, especially if the buffers are zero initialized and a smaller size is passed to sscanf (but I think it's supposed to add a zero terminator anyway), so you can use strcpy / strcmp everywhere. There's a couple of other calls elsewhere in this patch.