Re: [Intel-gfx] [PATCH i-g-t] lib/igt_debugfs: Update documentation and clenup

[Martin Peres]

clenup -> cleanup

On 17/07/17 18:05, Liviu Dudau wrote:

On Mon, Jul 17, 2017 at 04:59:48PM +0300, Arkadiusz Hiler wrote:

The documentation was lying. The igt_crc_to_string() is threadsafe and
does not return a pointer to an internal buffer.

Actually the caller is responsible for the memory that is allocated (and
they are for all the current cases), so let's put that in the doc too.

While I was at it I got rid of strdup() in favor of an early allocation.

Cc: Martin Peres 
Cc: Liviu Dudau 


Reviewed-by: Liviu Dudau 


Signed-off-by: Arkadiusz Hiler 
---
  lib/igt_debugfs.c | 9 -
  1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
index 80f25c6..5e4d72c 100644
--- a/lib/igt_debugfs.c
+++ b/lib/igt_debugfs.c
@@ -304,21 +304,20 @@ void igt_assert_crc_equal(const igt_crc_t *a, const 
igt_crc_t *b)
   * igt_crc_to_string:
   * @crc: pipe CRC value to print
   *
- * This formats @crc into a string buffer which is owned by 
igt_crc_to_string().
- * The next call will override the buffer again, which makes this 
multithreading
- * unsafe.
+ * This formats @crc into a string. Function allocates memory - the caller is
+ * in charge of freeing it.


These sentences are a little strange from a grammatical point of view. 
How about:


This function allocates a string and formats @crc into it. The caller is 
responsible for freeing the string.



   *
   * This should only ever be used for diagnostic debug output.
   */
  char *igt_crc_to_string(igt_crc_t *crc)
  {
int i;
-   char buf[128] = { 0 };
+   char *buf = calloc(128, sizeof(char));


To mimic the previous code's behaviour, please add the following:

if (!buf)
return NULL;

  
  	for (i = 0; i < crc->n_words; i++)

sprintf(buf + strlen(buf), "%08x ", crc->crc[i]);
  
-	return strdup(buf);

+   return buf;
  }
  
  #define MAX_CRC_ENTRIES 10

--
2.9.4





With at least the title and code fixed, this is:
Reviewed-by: Martin Peres 

Thanks for doing this :)
Martin
-
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 


This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] lib/igt_debugfs: Update documentation and clenup

[Liviu Dudau]

On Mon, Jul 17, 2017 at 04:59:48PM +0300, Arkadiusz Hiler wrote:
> The documentation was lying. The igt_crc_to_string() is threadsafe and
> does not return a pointer to an internal buffer.
> 
> Actually the caller is responsible for the memory that is allocated (and
> they are for all the current cases), so let's put that in the doc too.
> 
> While I was at it I got rid of strdup() in favor of an early allocation.
> 
> Cc: Martin Peres 
> Cc: Liviu Dudau 

Reviewed-by: Liviu Dudau 

> Signed-off-by: Arkadiusz Hiler 
> ---
>  lib/igt_debugfs.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> index 80f25c6..5e4d72c 100644
> --- a/lib/igt_debugfs.c
> +++ b/lib/igt_debugfs.c
> @@ -304,21 +304,20 @@ void igt_assert_crc_equal(const igt_crc_t *a, const 
> igt_crc_t *b)
>   * igt_crc_to_string:
>   * @crc: pipe CRC value to print
>   *
> - * This formats @crc into a string buffer which is owned by 
> igt_crc_to_string().
> - * The next call will override the buffer again, which makes this 
> multithreading
> - * unsafe.
> + * This formats @crc into a string. Function allocates memory - the caller is
> + * in charge of freeing it.
>   *
>   * This should only ever be used for diagnostic debug output.
>   */
>  char *igt_crc_to_string(igt_crc_t *crc)
>  {
>   int i;
> - char buf[128] = { 0 };
> + char *buf = calloc(128, sizeof(char));
>  
>   for (i = 0; i < crc->n_words; i++)
>   sprintf(buf + strlen(buf), "%08x ", crc->crc[i]);
>  
> - return strdup(buf);
> + return buf;
>  }
>  
>  #define MAX_CRC_ENTRIES 10
> -- 
> 2.9.4
> 

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx