Re: [Qemu-devel] [PATCH v2] gdbstub: Implement Xfer:auxv:read

[Peter Maydell]

On 28 July 2015 at 11:58, Bhushan Attarde <address@hidden> wrote:
> Implementation of "Xfer:auxv:read" to provide auxiliary vector information
> to clients which relies on it.
>
> For example: AT_ENTRY in auxiliary vector provides the entry point 
> information.
> Client can use this information to compare it with entry point mentioned in
> executable to calculate load offset and then update the load addresses
> accordingly.
>
> Signed-off-by: Bhushan Attarde <address@hidden>
> ---
>
> Notes:
>     Changes for v2:
>         - Xfer:auxv:read and Xfer:features:read now shares the code for 
> parsing out arguments rather than duplicating it.
>
>  gdbstub.c | 71 
> ++++++++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 54 insertions(+), 17 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 92b2f81..a6a79dc 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1150,42 +1150,73 @@ static int gdb_handle_packet(GDBState *s, const char 
> *line_buf)
>              if (cc->gdb_core_xml_file != NULL) {
>                  pstrcat(buf, sizeof(buf), ";qXfer:features:read+");
>              }
> +#ifdef CONFIG_USER_ONLY
> +            pstrcat(buf, sizeof(buf), ";qXfer:auxv:read+");
> +#endif
>              put_packet(s, buf);
>              break;
>          }
> -        if (strncmp(p, "Xfer:features:read:", 19) == 0) {
> +        if (strncmp(p, "Xfer:", 5) == 0) {
> +#ifdef CONFIG_USER_ONLY
> +            target_ulong auxv = 0;
> +#endif
> +            target_ulong total_len = 0;
> +            char *ptr = NULL;
>              const char *xml;
> -            target_ulong total_len;
>
> -            cc = CPU_GET_CLASS(first_cpu);
> -            if (cc->gdb_core_xml_file == NULL) {
> +            if (strncmp(p + 5, "features:read:", 14) == 0) {
> +                cc = CPU_GET_CLASS(first_cpu);
> +                if (cc->gdb_core_xml_file == NULL) {
> +                    goto unknown_command;
> +                }
> +                gdb_has_xml = true;
> +                p += 19;
> +                xml = get_feature_xml(p, &p, cc);
> +                if (!xml) {
> +                    snprintf(buf, sizeof(buf), "E00");
> +                    put_packet(s, buf);
> +                    break;
> +                }
> +                total_len = strlen(xml);
> +            }
> +#ifdef CONFIG_USER_ONLY
> +            else if (strncmp(p + 5, "auxv:read:", 10) == 0) {
> +                TaskState *ts = s->c_cpu->opaque;
> +                auxv = ts->info->saved_auxv;
> +                total_len = ts->info->auxv_len;
> +                p += 15;
> +                ptr = lock_user(VERIFY_READ, auxv, total_len, 0);
> +                if (ptr == NULL) {
> +                    break;
> +                }
> +                xml = (char *) ptr;
> +            }
> +#endif
> +            else {
>                  goto unknown_command;
>              }
>
> -            gdb_has_xml = true;
> -            p += 19;
> -            xml = get_feature_xml(p, &p, cc);
> -            if (!xml) {
> -                snprintf(buf, sizeof(buf), "E00");
> -                put_packet(s, buf);
> -                break;
> +            while (*p && *p != ':') {
> +                p++;
>              }
> +            p++;
>
> -            if (*p == ':')
> -                p++;
>              addr = strtoul(p, (char **)&p, 16);
> -            if (*p == ',')
> +            if (*p == ',') {
>                  p++;
> +            }
>              len = strtoul(p, (char **)&p, 16);
>
> -            total_len = strlen(xml);
> -            if (addr > total_len) {
> +            if ((ptr != NULL && addr > len)
> +                || (ptr == NULL && addr > total_len)) {

This if is rather confusing. Why should ptr == or != NULL
make a difference? What is ptr == NULL actually encoding?

>                  snprintf(buf, sizeof(buf), "E00");
>                  put_packet(s, buf);
>                  break;
>              }
> -            if (len > (MAX_PACKET_LENGTH - 5) / 2)
> +
> +            if (len > (MAX_PACKET_LENGTH - 5) / 2) {
>                  len = (MAX_PACKET_LENGTH - 5) / 2;
> +            }
>              if (len < total_len - addr) {
>                  buf[0] = 'm';
>                  len = memtox(buf + 1, xml + addr, len);

> @@ -1194,6 +1225,12 @@ static int gdb_handle_packet(GDBState *s, const char 
> *line_buf)
>                  len = memtox(buf + 1, xml + addr, total_len - addr);
>              }
>              put_packet_binary(s, buf, len + 1);
> +
> +#ifdef CONFIG_USER_ONLY
> +            if (ptr != NULL) {
> +                unlock_user(ptr, auxv, len);
> +            }
> +#endif

This hunk is pretty ugly and rather subtle too since "ptr" is a
very generic variable name. This is implicitly placing requirements
on the code further up to behave in a particular way (use ptr
and only ptr as locked-memory in user mode). Isn't there some
way to write this that abstracts stuff out into separate functions
or a 'register an Xfer read handler' pattern somehow?

-- PMM