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

[Bhushan Attarde]

Hi Peter,

Thanks for the review.

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

I think it is enough just to check if (addr > total_len) where,
"addr" is the offset (specified in packet) into target data area we are 
reading, and "total_len" is the byte size of that data area.
Checking if (addr > len) seems incorrect to me. I will change this in v3.

>> +#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).

Yes, "ptr" seems a very generic variable name, so we can use some 
unique/meaningful name for variable that represents locked-memory in user mode.
The same is applicable for 2nd argument of unlock_user (it represents guest 
address pointer).

Thanks,
Bhushan


-----Original Message-----
From: Peter Maydell [mailto:address@hidden 
Sent: 28 July 2015 16:54
To: Bhushan Attarde
Cc: QEMU Developers; Yongbok Kim; Jaydeep Patil
Subject: Re: [PATCH v2] gdbstub: Implement Xfer:auxv:read

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