Re: [LTP] [PATCH v4] syscalls/mlock05: add mlock test for locking and pre-faulting of memory

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +static unsigned long get_proc_smaps_field(unsigned long desired_mapping_address, char *desired_field)
> +{
> +	bool mapping_found = false;
> +	char buffer[LINELEN] = "";
> +	FILE *file = NULL;
> +	int ret = 0;

There is no point in initializing buffer, file and ret these are written
to before we use the value.

> +	file = fopen("/proc/self/smaps", "r");
> +	if (file == NULL) {
> +		tst_brk(TBROK | TERRNO, "cannot open file proc/self/smaps");
> +		return 0;
> +	}

Use SAFE_FOPEN() please.

> +	// find desired mapping
> +	while (fgets(buffer, LINELEN, file) != NULL) {
> +		unsigned long mapping_address;
> +
> +		// check the starting address

I do not find this comment to be useful, we do have a rule in LTP not to
comment obvious and I would argue that this comment does so.

> +		ret = sscanf(buffer, "%lx[^-]", &mapping_address);
> +
> +		if ((ret == 1) && (mapping_address == desired_mapping_address)) {
> +			mapping_found = true;
> +			break;
> +		}
> +	}
> +
> +	if (!mapping_found) {
> +		fclose(file);

SAFE_FCLOSE() please and in the rest of the cases below.

> +		tst_brk(TBROK, "cannot find mapping %lx in proc/self/smaps", desired_mapping_address);
> +		return 0;
> +	}
> +
> +	// find desired field
> +	while (fgets(buffer, LINELEN, file) != NULL) {
> +		if (strstr(buffer, desired_field) != NULL) {

Can we use strncmp() to match the prefix rather a substring? At the
moment "Locked" and "Rss" are not substrings of other keys in that file
but that may change at some point.

> +			unsigned long desired_value;
> +
> +			// extract the value for the requested field

Here as well, commenting the obvious.

> +			ret = sscanf(buffer, "%*[^0-9]%lu%*[^0-9]", &desired_value);

The key value is divided by : and thge entries we are interested in are
in kB so "%*[^:]%lu kB" should be a bit safer.

> +			fclose(file);
> +
> +			if (ret != 1) {
> +				tst_brk(TBROK, "failure occured while reading field %s", desired_field);
> +				return 0;
> +			}
> +
> +			return desired_value;
> +		}
> +	}
> +
> +	fclose(file);
> +	tst_brk(TBROK, "cannot find %s field", desired_field);
> +
> +	return 0;
> +}
> +
> +static void verify_mlock(void)
> +{
> +	unsigned long Locked;
> +	unsigned long Rss;
> +	char *buf;
> +
> +	buf = SAFE_MMAP(NULL, MMAPLEN, PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +	SAFE_MLOCK(buf, MMAPLEN);
> +
> +	Rss = get_proc_smaps_field((unsigned long)buf, "Rss");
> +	Locked = get_proc_smaps_field((unsigned long)buf, "Locked");

Hmm, we do parse the whole file twice here, just the get to a nearly
same line. Why can't we just point two pointrs to unsigned long to the
function and collect both while we read the file?

> +	// Convertion from KiB to B
> +	Rss *= 1024;
> +	Locked *= 1024;
> +
> +	TST_EXP_EQ_LU(Rss, MMAPLEN);
> +	TST_EXP_EQ_LU(Locked, MMAPLEN);
> +
> +	SAFE_MUNLOCK(buf, MMAPLEN);
> +	SAFE_MUNMAP(buf, MMAPLEN);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = verify_mlock,
> +};

-- 
Cyril Hrubis
chrubis@suse.cz

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