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

Jan Stancek
2016-06-16T07:08:18Z

----- 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