Re: [LTP] [PATCH 1/3] Adding new function read_maps required for hugemmap40.

[Petr Vorel <pvorel@suse.cz>]

Hi Pavithra, Geetika,

> Function to read and parse the '/proc/self/maps' file to debug memory-related issues.

First of all, do you plan to use read_maps() in other hugemmap tests? Otherwise
it does not make sense to add it to hugetlb.c library.

> Signed-off-by: Pavithra <pavrampu@linux.ibm.com>
> ---
>  testcases/kernel/mem/hugetlb/lib/hugetlb.c | 42 ++++++++++++++++++++++
>  testcases/kernel/mem/hugetlb/lib/hugetlb.h |  1 +
>  2 files changed, 43 insertions(+)

> diff --git a/testcases/kernel/mem/hugetlb/lib/hugetlb.c b/testcases/kernel/mem/hugetlb/lib/hugetlb.c
> index 6a2976a53..fdd745eda 100644
> --- a/testcases/kernel/mem/hugetlb/lib/hugetlb.c
> +++ b/testcases/kernel/mem/hugetlb/lib/hugetlb.c
> @@ -141,3 +141,45 @@ void update_shm_size(size_t * shm_size)
>  		*shm_size = shmmax;
>  	}
>  }
> +
> +#define MAPS_BUF_SZ 4096
NOTE: we usually use BUFSIZ (from <stdio.h>) for these purposes.
nit: we usually have definitions at the top. (I know this file has #define
RANDOM_CONSTANT 0x1234ABCD, but this file is outdated, not following LTP
standards.)

> +int read_maps(unsigned long addr, char *buf)
> +{
> +        FILE *f;
> +        char line[MAPS_BUF_SZ];
> +        char *tmp;
> +
> +        f = fopen("/proc/self/maps", "r");

> +        if (!f) {
nit: we have SAFE_FOPEN(), this check is not needed. See my commit today:
https://github.com/linux-test-project/ltp/commit/f3803d4bfabe4da10d9fc1824df5b10c249dbed6
and please rebase your next version.

> +                tst_res(TFAIL, "Failed to open /proc/self/maps: %s\n", strerror(errno));
Even we did not have not have safe function, failure like this we consider hard
and use tst_brk(TBROK | TERRNO) to quit whole testing.
> +                return -1;
> +        }
> +
> +        while (1) {
> +                unsigned long start, end, off, ino;
> +                int ret;
> +
> +                tmp = fgets(line, MAPS_BUF_SZ, f);
> +                if (!tmp)
> +                        break;

Using
while (fgets(line, BUFSIZ, f) != NULL) {

is much simpler than having char *tmp just for checking.

> +
> +                buf[0] = '\0';
Why this? That's IMHO unnecessary.

> +                ret = sscanf(line, "%lx-%lx %*s %lx %*s %ld %255s",
> +                             &start, &end, &off, &ino,
> +                             buf);
> +                if ((ret < 4) || (ret > 5)) {
> +                        tst_res(TFAIL, "Couldn't parse /proc/self/maps line: %s\n",
> +                                        line);
> +                        fclose(f);
> +                        return -1;

FYI we can also have FILE_LINES_SCANF() and SAFE_FILE_LINES_SCANF() macros,
which wraps file_lines_scanf() from lib/safe_file_ops.c which uses vsscanf().
I believe it could be used for the task you want to achieve.
> +                }
> +
> +                if ((start <= addr) && (addr < end)) {
> +                        fclose(f);
> +                        return 1;
> +                }
> +        }
> +
> +        fclose(f);
> +        return 0;
> +}

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp