[PATCH v1 1/4] [makedumpfile] Add read_string routine

[kumagai-atsushi@xxxxxxxxxxxxxxxxx (Atsushi Kumagai)]

Hello Suzuki,

On Thu, 16 Feb 2012 09:05:40 +0530
"Suzuki K. Poulose" <suzuki at in.ibm.com> wrote:

> This patch adds a read_string() wrapper around readmem().
> 
> int read_string(int type_addr, unsigned long long addr, char *bufptr, size_t len);
> 
> The functionality is similar to that of readmem(), except that we stop reading
> when we find an end-of-string marker than reading 'len' bytes.
> 
> This will be useful for reading the name of the 'powerpc' platform which is
> needed for dynamic page address definition bits.
[...]
> +int
> +read_string(int type_addr, unsigned long long addr, char *buf, size_t len)
> +{
> +	int i;
> +
> +	memset(buf, 0, len);
> +	for (i = 0; i < len; i ++)
> +		if (!readmem(VADDR, addr + i, &buf[i], 1))
> +			break;
> +	return i;
> +}

Sorry for late reply. I reviewed your patches and I have question about
read_string().

You explained that read_string() stops reading when end-of-string appear,
but your code seems to read just 'len' bytes regardless of end-of-string. 
Maybe, do you expect readmem() to stop reading when end-of-string appear ?

Additionally, it is inefficient to call readmem() to read each byte of data.

Therefore, the code below may satisfy you ?

---------------------------------------------------------------------------
diff --git a/makedumpfile.c b/makedumpfile.c
index fea26af..5743d74 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -389,13 +389,29 @@ error:
 int
 read_string(int type_addr, unsigned long long addr, char *buf, size_t len)
 {
-       int i;
+       int i, remnant;
+       const int region = 512;

        memset(buf, 0, len);
-       for (i = 0; i < len; i ++)
-               if (!readmem(VADDR, addr + i, &buf[i], 1))
+       for (i = 0; i < len; i += region) {
+               /*
+                * buf[len] must be end-of-string.
+                */
+               remnant = len - i;
+               if (remnant == 1) {
                        break;
-       return i;
+               } else if (remnant <= region) {
+                       if (!readmem(type_addr, addr + i, &buf[i], remnant - 1 ))
+                               return ERROR;
+                       break;
+               }
+
+               if (!readmem(type_addr, addr + i, &buf[i], region))
+                       return ERROR;
+               if (strlen(buf) != i + region)
+                       break;
+       }
+       return strlen(buf);
 }


Thanks
Atsushi Kumagai