[LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem

[LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem

[Jan Stancek]

This patch adds 2 new functions that both parse data
from /proc/meminfo.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 include/tst_mem.h | 36 ++++++++++++++++++++++++++++++++
 lib/tst_mem.c     | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+)
 create mode 100644 include/tst_mem.h
 create mode 100644 lib/tst_mem.c

Julio Cruz reported couple issues when running LTP on systems with
limited RAM. To avoid duplicating get_avail_mem() in all of them,
I propose we add such function to lib.

diff --git a/include/tst_mem.h b/include/tst_mem.h
new file mode 100644
index 000000000000..a37aaa088103
--- /dev/null
+++ b/include/tst_mem.h
@@ -0,0 +1,36 @@
+/*
+ * Copyright (c) 2016 Linux Test Project
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef TST_MEM_H__
+#define TST_MEM_H__
+
+/*
+ * Returns number of kB for /proc/meminfo "item" or -1
+ * if "item" can not be found.
+ *
+ * @item: name of /proc/meminfo item, e.g. MemFree
+ * @strict: if true and "item" can not be found,
+ *          function calls tst_brk(TBROK,..)
+ */
+long tst_read_meminfo(const char *item, int strict);
+
+/*
+ * Returns number of kB of available memory.
+ */
+long tst_get_avail_mem(void);
+
+#endif /* TST_MEM_H__ */
diff --git a/lib/tst_mem.c b/lib/tst_mem.c
new file mode 100644
index 000000000000..d1a08f78b026
--- /dev/null
+++ b/lib/tst_mem.c
@@ -0,0 +1,62 @@
+/*
+ * Copyright (c) 2016 Linux Test Project
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <stdio.h>
+#include "tst_test.h"
+#include "tst_mem.h"
+
+static const char *path_meminfo = "/proc/meminfo";
+
+long tst_read_meminfo(const char *item, int strict)
+{
+	FILE *fp;
+	char line[BUFSIZ], buf[BUFSIZ];
+	long val;
+
+	fp = fopen(path_meminfo, "r");
+	if (fp == NULL)
+		tst_brk(TBROK | TERRNO, "fopen %s", path_meminfo);
+
+	while (fgets(line, BUFSIZ, fp) != NULL) {
+		if (sscanf(line, "%64s %ld", buf, &val) == 2)
+			if (strcmp(buf, item) == 0) {
+				fclose(fp);
+				return val;
+			}
+		continue;
+	}
+	fclose(fp);
+
+	if (strict)
+		tst_brk(TBROK, "cannot find \"%s\" in %s",
+			item, path_meminfo);
+
+	return -1;
+}
+
+long tst_get_avail_mem(void)
+{
+	long mem_available;
+
+	mem_available = tst_read_meminfo("MemAvailable:", 0);
+	if (mem_available == -1) {
+		mem_available = tst_read_meminfo("MemFree:", 1);
+		mem_available += tst_read_meminfo("Cached:", 1);
+	}
+
+	return mem_available;
+}
-- 
1.8.3.1

[LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem

[Li Wang]

Hi Jan,

On Tue, Jun 14, 2016 at 7:27 PM, Jan Stancek <jstancek@redhat.com> wrote:
> This patch adds 2 new functions that both parse data
> from /proc/meminfo.

It is a good proposal, but the achieved function seems a little bit
narrow for ltp-lib. Can we
rename the tst_mem.h to tst_proc.h, and makes the whole items of
procfs could be readable via the same interfaces? And the strict
parameter is not necessary for the function i think.

How about something like this?

-----------------------------------------
long read_proc(char *path, char *item)
{
    FILE *fp;
    char line[BUFSIZ], buf[BUFSIZ];
    long val;

    fp = fopen(path, "r");
    if (fp == NULL)
        tst_brkm(TBROK | TERRNO, cleanup, "fopen %s", path);

    while (fgets(line, BUFSIZ, fp) != NULL) {
        if (sscanf(line, "%64s %ld", buf, &val) == 2)
            if (strcmp(buf, item) == 0) {
                fclose(fp);
                return val;
            }
        continue;
    }
    fclose(fp);

    tst_res(TINFO, "cannot find \"%s\" in %s", item, path);

    return -1;
}

long tst_read_meminfo(char *item)
{

    return read_proc("/proc/meminfo", item);
}

long tst_read_self_status(char *item)
{
    return read_proc("/proc/self/status", item);
}

// we can extend this interface easily by uniform prototype
long tst_read_blablabla(char *item)
{
    return read_proc("/proc/blablabla...", item);
}
...

long tst_get_avail_mem(void)
{
       long mem_available;

       mem_available = tst_read_meminfo("memavailable:");
       if (mem_available == -1) {
               mem_available = tst_read_meminfo("memfree:");
               mem_available += tst_read_meminfo("cached:");
       }

       return mem_available;
}


Regards,
Li Wang

[LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem

[Jan Stancek]

----- Original Message -----
> From: "Li Wang" <liwang@redhat.com>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Tuesday, 14 June, 2016 4:37:29 PM
> Subject: Re: [LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem
> 
> Hi Jan,
> 
> On Tue, Jun 14, 2016 at 7:27 PM, Jan Stancek <jstancek@redhat.com> wrote:
> > This patch adds 2 new functions that both parse data
> > from /proc/meminfo.
> 
> It is a good proposal, but the achieved function seems a little bit
> narrow for ltp-lib.

OK, let me try to come up with something more generic.

> Can we
> rename the tst_mem.h to tst_proc.h, and makes the whole items of
> procfs could be readable via the same interfaces?

They don't all share same format, so there have to be some limitations.

> And the strict
> parameter is not necessary for the function i think.

We could leave that up to caller.

Regards,
Jan

[LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem

[Jan Stancek]

On 06/15/2016 09:34 AM, Jan Stancek wrote:
> 
> 
> 
> ----- Original Message -----
>> From: "Li Wang" <liwang@redhat.com>
>> To: "Jan Stancek" <jstancek@redhat.com>
>> Cc: ltp@lists.linux.it
>> Sent: Tuesday, 14 June, 2016 4:37:29 PM
>> Subject: Re: [LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem
>>
>> It is a good proposal, but the achieved function seems a little bit
>> narrow for ltp-lib.
> 
> OK, let me try to come up with something more generic.

How about something like this?

#include <stdio.h>
#include "tst_proc.h"

#define TST_NO_DEFAULT_MAIN
#include "tst_test.h"

static const char *path_meminfo = "/proc/meminfo";

typedef int walk_func(const char *line, void *priv);

/*
 * Go through all lines in a text file, calling walk_func() on each
 * until it returns 1 or file hits EOF. Returns last return value of
 * walk_func().
 *
 * @filename: name of text file to walk
 * @walk_func: function called for each line
 * @func_priv: walk_func's specific struct representing in/out data
 * */
static int walk_file_lines(const char *filename, walk_func func,
			   void *func_priv)
{
	FILE *fp;
	char line[BUFSIZ];

	fp = fopen(filename, "r");
	if (fp == NULL)
		tst_brk(TBROK | TERRNO, "fopen %s", path_meminfo);

	while (fgets(line, BUFSIZ, fp) != NULL) {
		if (func(line, func_priv) == 1) {
			fclose(fp);
			return 1;
		}
		continue;
	}
	fclose(fp);

	return 0;
}

struct colonpair_str_long
{
	const char *key;
	long value;
};

/*
 *  Returns 1 if "line" matches pattern: "Key: Value" and
 *  Key is equal to colonpair_str_long->key, otherwise returns 0.
 *
 *  @line: line searched for pattern
 *  @priv: pointer to colonpair_str_long struct where
 *         .key - Key to search for
 *         .value - stores "Value" if "Key" is found
 */
static int match_colonpair_str_long(const char *line, void *priv)
{
	struct colonpair_str_long *s = priv;
	char buf[BUFSIZ];
	long val;

	if (sscanf(line, "%64s %ld", buf, &val) == 2) {
		/* strip colon for comparison with key */
		buf[strlen(buf) - 1] = '\0';

		if (strcmp(buf, s->key) == 0) {
			s->value = val;
			return 1;
		}
	}

	return 0;
}

static long read_colonpair_str_long(const char *filename, const char *key,
				    long ret_for_missing)
{
	struct colonpair_str_long data = {
		.key = key,
	};

	if (walk_file_lines(filename, match_colonpair_str_long, &data))
		return data.value;

	return ret_for_missing;
}

/*
 * Search for Key: Value pair in /proc/meminfo, if found
 * return Value, otherwise return -1.
 */
long tst_get_meminfo(const char *key)
{
	return read_colonpair_str_long(path_meminfo, key, -1);
}

long tst_get_avail_mem(void)
{
	long mem_available;

	mem_available = tst_get_meminfo("MemAvailable");
	if (mem_available == -1) {
		mem_available = tst_get_meminfo("MemFree");
		mem_available += tst_get_meminfo("Cached");
	}

	return mem_available;
}

[LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem

[Li Wang]

On Wed, Jun 15, 2016 at 02:51:33PM +0200, Jan Stancek wrote:
> On 06/15/2016 09:34 AM, Jan Stancek wrote:
> 
> How about something like this?

This one looks better, just have a little comments as below:

> static int walk_file_lines(const char *filename, walk_func func,
> 			   void *func_priv)
> {
> 	FILE *fp;
> 	char line[BUFSIZ];
> 
> 	fp = fopen(filename, "r");
> 	if (fp == NULL)
> 		tst_brk(TBROK | TERRNO, "fopen %s", path_meminfo);

A typo here, the 'path_meminfo' should be replaced by 'filename'.

> static int match_colonpair_str_long(const char *line, void *priv)
> {
> 	struct colonpair_str_long *s = priv;
> 	char buf[BUFSIZ];
> 	long val;
> 
> 	if (sscanf(line, "%64s %ld", buf, &val) == 2) {
> 		/* strip colon for comparison with key */
> 		buf[strlen(buf) - 1] = '\0';

If manual set the last character as '\0', there will be limit this
match function area.

e.g.
  # cat /proc/stat | grep processes
      processes 129493

A caller can not get correct value of the processes, since the
string saved in buf[] as: 'p' 'r' 'o' 'c' 'e' 's' 's' 'e' '\0',
it will disturb the mathing result and return 0.

My proposal is to have a simple choice here:

/* strip colon for comparison with key */
	if (buf[strlen(buf) -1] == ':')
		buf[strlen(buf) -1] = '\0';
> 
> long tst_get_avail_mem(void)
> {
> 	long mem_available;
> 
> 	mem_available = tst_get_meminfo("MemAvailable");
> 	if (mem_available == -1) {
> 		mem_available = tst_get_meminfo("MemFree");
> 		mem_available += tst_get_meminfo("Cached");
> 	}
> 
> 	return mem_available;
> }

I'd like to add new callers for reading '/proc/stat' and
'/proc/self/status' to verify read_colonpair_str_long() function.

static const char *path_proc_stat = "/proc/stat";
static const char *path_self_status = "/proc/self/status";

long tst_get_stat(const char *key)
{
	return read_colonpair_str_long(path_proc_stat, key, -1);
}

long tst_get_self_status(const char *key)
{
	return read_colonpair_str_long(path_self_status, key, -1);
}

a simple test program:
---------------------
# cat newlib_tests/test14.c

#include "tst_test.h"
#include "tst_proc.h"

static void do_test(void)
{
	tst_res(TINFO, "MemTotal = %ld kB", tst_get_meminfo("MemTotal"));
	tst_res(TINFO, "MemAvailable = %ld kB", tst_get_meminfo("MemAvailable"));
	tst_res(TINFO, "MemAvailable = %ld kB", tst_get_avail_mem());
	tst_res(TINFO, "Shmem = %ld kB", tst_get_meminfo("Shmem"));
	tst_res(TINFO, "SwapFree = %ld kB", tst_get_meminfo("SwapFree"));

	tst_res(TINFO, "btime = %ld", %tst_get_stat("btime"));
	tst_res(TINFO, "processes = %ld", %tst_get_stat("processes"));
	tst_res(TINFO, "VmPeak = %ld kB", tst_get_self_status("VmPeak"));

	tst_res(TPASS, "Test proc read file pass");
}

static struct tst_test test =
{
	.tid = "test14",
	.test_all = do_test,
};


# ./test14
tst_test.c:699: INFO: Timeout per run is 300s
test14.c:25: INFO: MemTotal = 16230660 kB
test14.c:26: INFO: MemAvailable = 13310720 kB
test14.c:26: INFO: MemAvailable = 13310720 kB
test14.c:27: INFO: Shmem = 8588 kB
test14.c:28: INFO: SwapFree = 8156220 kB
test14.c:30: INFO: btime = 1465882713
test14.c:31: INFO: processes = 288403
test14.c:33: INFO: VmPeak = 4368 kB
test14.c:35: PASS: Test proc read file pass

Summary:
passed   1
failed   0
skipped  0
warnings 0


Regards,
Li Wang

[LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem

[Jan Stancek]

----- Original Message -----
> From: "Li Wang" <liwang@redhat.com>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Thursday, 16 June, 2016 7:25:44 AM
> Subject: Re: [LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem
> 
> On Wed, Jun 15, 2016 at 02:51:33PM +0200, Jan Stancek wrote:
> > On 06/15/2016 09:34 AM, Jan Stancek wrote:
> > 
> > How about something like this?
> 
> This one looks better, just have a little comments as below:
> 
> > static int walk_file_lines(const char *filename, walk_func func,
> > 			   void *func_priv)
> > {
> > 	FILE *fp;
> > 	char line[BUFSIZ];
> > 
> > 	fp = fopen(filename, "r");
> > 	if (fp == NULL)
> > 		tst_brk(TBROK | TERRNO, "fopen %s", path_meminfo);
> 
> A typo here, the 'path_meminfo' should be replaced by 'filename'.
> 
> > static int match_colonpair_str_long(const char *line, void *priv)
> > {
> > 	struct colonpair_str_long *s = priv;
> > 	char buf[BUFSIZ];
> > 	long val;
> > 
> > 	if (sscanf(line, "%64s %ld", buf, &val) == 2) {
> > 		/* strip colon for comparison with key */
> > 		buf[strlen(buf) - 1] = '\0';
> 
> If manual set the last character as '\0', there will be limit this
> match function area.
> 
> e.g.
>   # cat /proc/stat | grep processes
>       processes 129493
> 
> A caller can not get correct value of the processes, since the
> string saved in buf[] as: 'p' 'r' 'o' 'c' 'e' 's' 's' 'e' '\0',
> it will disturb the mathing result and return 0.

That was expected, since you're using it to parse a different format.

> 
> My proposal is to have a simple choice here:
> 
> /* strip colon for comparison with key */
> 	if (buf[strlen(buf) -1] == ':')
> 		buf[strlen(buf) -1] = '\0';
> > 
> > long tst_get_avail_mem(void)
> > {
> > 	long mem_available;
> > 
> > 	mem_available = tst_get_meminfo("MemAvailable");
> > 	if (mem_available == -1) {
> > 		mem_available = tst_get_meminfo("MemFree");
> > 		mem_available += tst_get_meminfo("Cached");
> > 	}
> > 
> > 	return mem_available;
> > }
> 
> I'd like to add new callers for reading '/proc/stat' and
> '/proc/self/status' to verify read_colonpair_str_long() function.
> 
> static const char *path_proc_stat = "/proc/stat";
> static const char *path_self_status = "/proc/self/status";
> 
> long tst_get_stat(const char *key)
> {
> 	return read_colonpair_str_long(path_proc_stat, key, -1);
> }
> 
> long tst_get_self_status(const char *key)
> {
> 	return read_colonpair_str_long(path_self_status, key, -1);
> }

I'm now considering exposing just a parse function, I'd like to
avoid having separate function for each proc file. And possibly
more than one since those 2 proc files you mentioned don't have
uniform format for all content.

Regards,
Jan

[LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem

[Li Wang]

On Thu, Jun 16, 2016 at 3:08 PM, Jan Stancek <jstancek@redhat.com> wrote:
>
> I'm now considering exposing just a parse function, I'd like to
> avoid having separate function for each proc file. And possibly
> more than one since those 2 proc files you mentioned don't have
> uniform format for all content.

Yes, achieve one parse function for whole proc files is unpractical.
IMO, we could take more consideration for the files frequently opened
in ltp general test.

Regards,
Li Wang

[LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem

[Li Wang]

Jan,

On Thu, Jun 16, 2016 at 03:08:18AM -0400, Jan Stancek wrote:
> 
> I'm now considering exposing just a parse function, I'd like to
> avoid having separate function for each proc file. And possibly
> more than one since those 2 proc files you mentioned don't have
> uniform format for all content.

How is the parse function going?

As there comes more and more places need get_mem_avaliable(), my opinion
is that we can apply your walk_file_lines() patch first, and let the
/proc/meminfo could be read by this lib. Then if we need more proc/files info
in future, adding new separate function is also accepted.

After all, we had discussed that one parse function for each proc file
is very hard to achive.

Regards,
Li Wang

[LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem

[Jan Stancek]

On 07/13/2016 11:35 AM, Li Wang wrote:
> Jan,
> 
> On Thu, Jun 16, 2016 at 03:08:18AM -0400, Jan Stancek wrote:
>>
>> I'm now considering exposing just a parse function, I'd like to
>> avoid having separate function for each proc file. And possibly
>> more than one since those 2 proc files you mentioned don't have
>> uniform format for all content.
> 
> How is the parse function going?

Latest idea attached.

It's similar to SAFE_FILE_SCANF, but it scanfs each line and first
matching all scanf directives wins and terminates search, otherwise
you get non-zero ret code. For example:

if (SAFE_FILE_LINES_SCANF("/proc/meminfo", "MemFree: %ld", &free))
        tst_brk(TBROK, "Could not parse MemFree");

Regards,
Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lib-add-SAFE_FILE_LINES_SCANF.patch
Type: text/x-patch
Size: 5623 bytes
Desc: not available
URL: <http://lists.linux.it/pipermail/ltp/attachments/20160713/54918c2b/attachment.bin>

[LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem

[Cyril Hrubis]

Hi!
> Latest idea attached.
> 
> It's similar to SAFE_FILE_SCANF, but it scanfs each line and first
> matching all scanf directives wins and terminates search, otherwise
> you get non-zero ret code. For example:
> 
> if (SAFE_FILE_LINES_SCANF("/proc/meminfo", "MemFree: %ld", &free))
>         tst_brk(TBROK, "Could not parse MemFree");

The API looks good to me. Nice work.

We should call this version FILE_LINES_SCANF() since the SAFE_ variants
call tst_brkm() on failure.

> diff --git a/lib/safe_file_ops.c b/lib/safe_file_ops.c
> index dff85cd83fec..db1660b1f684 100644
> --- a/lib/safe_file_ops.c
> +++ b/lib/safe_file_ops.c
> @@ -169,6 +169,49 @@ void safe_file_scanf(const char *file, const int lineno,
>  	}
>  }
>  
> +
> +/*
> + * Try to parse each line from file specified by 'path' according
> + * to scanf format 'fmt'. If all fields could be parsed, stop and
> + * return 0, otherwise continue or return 1 if EOF is reached.
> + */
> +int safe_file_lines_scanf(const char *file, const int lineno,
> +			  void (*cleanup_fn)(void),
> +			  const char *path, const char *fmt, ...)
> +{
> +	FILE *fp;
> +	int ret = 0;
> +	int arg_count = 0;
> +	char line[BUFSIZ];
> +	va_list ap;
> +
> +	if (!fmt) {
> +		tst_brkm(TBROK, cleanup_fn, "pattern is NULL, %s:%d",
> +			file, lineno);
> +	}
> +
> +	fp = fopen(path, "r");
> +	if (fp == NULL) {
> +		tst_brkm(TBROK | TERRNO, cleanup_fn,
> +			"Failed to open FILE '%s' for reading at %s:%d",
> +			path, file, lineno);
> +	}
> +
> +	arg_count = count_scanf_conversions(fmt);
> +
> +	while (fgets(line, BUFSIZ, fp) != NULL) {
> +		va_start(ap, fmt);
> +		ret = vsscanf(line, fmt, ap);
> +		va_end(ap);
> +
> +		if (ret == arg_count)
> +			break;
> +	}
> +	fclose(fp);

I wonder if it would be easier to take the fmt and append "%*[^\n]\n" to
it so that we can use fscanf() directly without the line buffer...

I guess that both of these are similarily complicated.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: "Li Wang" <liwang@redhat.com>, ltp@lists.linux.it
> Sent: Wednesday, 13 July, 2016 3:54:28 PM
> Subject: Re: [LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem
> 
> Hi!
> > Latest idea attached.
> > 
> > It's similar to SAFE_FILE_SCANF, but it scanfs each line and first
> > matching all scanf directives wins and terminates search, otherwise
> > you get non-zero ret code. For example:
> > 
> > if (SAFE_FILE_LINES_SCANF("/proc/meminfo", "MemFree: %ld", &free))
> >         tst_brk(TBROK, "Could not parse MemFree");
> 
> The API looks good to me. Nice work.
> 
> We should call this version FILE_LINES_SCANF() since the SAFE_ variants
> call tst_brkm() on failure.

I was hesitant about SAFE_ part, but since it still calls brk on some
occasions I left it there.

There is also option to remove brkm calls, turn it into tst_file_lines_scanf(),
and let user deal with possible errors.


> 
> > diff --git a/lib/safe_file_ops.c b/lib/safe_file_ops.c
> > index dff85cd83fec..db1660b1f684 100644
> > --- a/lib/safe_file_ops.c
> > +++ b/lib/safe_file_ops.c
> > @@ -169,6 +169,49 @@ void safe_file_scanf(const char *file, const int
> > lineno,
> >  	}
> >  }
> >  
> > +
> > +/*
> > + * Try to parse each line from file specified by 'path' according
> > + * to scanf format 'fmt'. If all fields could be parsed, stop and
> > + * return 0, otherwise continue or return 1 if EOF is reached.
> > + */
> > +int safe_file_lines_scanf(const char *file, const int lineno,
> > +			  void (*cleanup_fn)(void),
> > +			  const char *path, const char *fmt, ...)
> > +{
> > +	FILE *fp;
> > +	int ret = 0;
> > +	int arg_count = 0;
> > +	char line[BUFSIZ];
> > +	va_list ap;
> > +
> > +	if (!fmt) {
> > +		tst_brkm(TBROK, cleanup_fn, "pattern is NULL, %s:%d",
> > +			file, lineno);
> > +	}
> > +
> > +	fp = fopen(path, "r");
> > +	if (fp == NULL) {
> > +		tst_brkm(TBROK | TERRNO, cleanup_fn,
> > +			"Failed to open FILE '%s' for reading at %s:%d",
> > +			path, file, lineno);
> > +	}
> > +
> > +	arg_count = count_scanf_conversions(fmt);
> > +
> > +	while (fgets(line, BUFSIZ, fp) != NULL) {
> > +		va_start(ap, fmt);
> > +		ret = vsscanf(line, fmt, ap);
> > +		va_end(ap);
> > +
> > +		if (ret == arg_count)
> > +			break;
> > +	}
> > +	fclose(fp);
> 
> I wonder if it would be easier to take the fmt and append "%*[^\n]\n" to
> it so that we can use fscanf() directly without the line buffer...
> 
> I guess that both of these are similarily complicated.
> 
> --
> Cyril Hrubis
> chrubis@suse.cz
> 

[LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem

[Cyril Hrubis]

Hi!
> > > It's similar to SAFE_FILE_SCANF, but it scanfs each line and first
> > > matching all scanf directives wins and terminates search, otherwise
> > > you get non-zero ret code. For example:
> > > 
> > > if (SAFE_FILE_LINES_SCANF("/proc/meminfo", "MemFree: %ld", &free))
> > >         tst_brk(TBROK, "Could not parse MemFree");
> > 
> > The API looks good to me. Nice work.
> > 
> > We should call this version FILE_LINES_SCANF() since the SAFE_ variants
> > call tst_brkm() on failure.
> 
> I was hesitant about SAFE_ part, but since it still calls brk on some
> occasions I left it there.
> 
> There is also option to remove brkm calls, turn it into tst_file_lines_scanf(),
> and let user deal with possible errors.

Thinking about this, there are only two errors that we care about.
Either the file we are trying to open does not exist, which may be
reason to fail the test with TCONF instead or use different code path.
And the second one is when we couldn't match the line(s) which has the
same implications.

We can safely call tst_brkm(TBROK,) for the rest of the cases. And even
the 'file does not exist' could be easily handled in test setup with
single access() call.

So I would say that we can keep the code as it is and just drop the
SAFE_ prefix and possibly add SAFE_FILE_LINES_SCANF() as well for cases
where the user is not interested in handling the returned error himself.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] lib: add tst_read_meminfo / tst_get_avail_mem

[Li Wang]

On Wed, Jul 13, 2016 at 03:12:08PM +0200, Jan Stancek wrote:
> 
> Latest idea attached.
> 
> It's similar to SAFE_FILE_SCANF, but it scanfs each line and first
> matching all scanf directives wins and terminates search, otherwise
> you get non-zero ret code. For example:
> 
> if (SAFE_FILE_LINES_SCANF("/proc/meminfo", "MemFree: %ld", &free))
>         tst_brk(TBROK, "Could not parse MemFree");

I like this method! And I tend to think that we probably need achive two
macors for different occasions. In fact as the FILE_SCANF() do that.

FILE_LINES_SCANF() and SAFE_FILE_LINES_SCANF()


Regards,
Li Wang