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:peter.mayd...@linaro.org]
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 <bhushan.atta...@imgtec.com> 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 <bhushan.atta...@imgtec.com>
> ---
>
> 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
