Re: [LTP] [PATCH 3/3] testcaes/lib: Add shell loader

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> > +static void metadata_append(const char *line)
> > +{
> > +	size_t linelen = strlen(line);
> > +
> > +	if (metadata_size - metadata_used < linelen + 1) {
> > +		metadata_size += 128;
> 
> This seems like a very small amount to bother allocating.

That is what I have left there after the testing phase where I wanted to
trigger the reallocation. I will change this to 4k.

> > +		metadata = SAFE_REALLOC(metadata, metadata_size);
> > +	}
> > +
> > +	strcpy(metadata + metadata_used, line);
> > +	metadata_used += linelen;
> > +}
> > +
> > +static ujson_obj_attr test_attrs[] = {
> > +	UJSON_OBJ_ATTR("all_filesystems", UJSON_BOOL),
> > +	UJSON_OBJ_ATTR("dev_min_size", UJSON_INT),
> > +	UJSON_OBJ_ATTR("filesystems", UJSON_ARR),
> > +	UJSON_OBJ_ATTR("format_device", UJSON_BOOL),
> > +	UJSON_OBJ_ATTR("min_cpus", UJSON_INT),
> > +	UJSON_OBJ_ATTR("min_mem_avail", UJSON_INT),
> > +	UJSON_OBJ_ATTR("min_kver", UJSON_STR),
> > +	UJSON_OBJ_ATTR("min_swap_avail", UJSON_INT),
> > +	UJSON_OBJ_ATTR("mntpoint", UJSON_STR),
> > +	UJSON_OBJ_ATTR("mount_device", UJSON_BOOL),
> > +	UJSON_OBJ_ATTR("needs_abi_bits", UJSON_INT),
> > +	UJSON_OBJ_ATTR("needs_devfs", UJSON_BOOL),
> > +	UJSON_OBJ_ATTR("needs_device", UJSON_BOOL),
> > +	UJSON_OBJ_ATTR("needs_hugetlbfs", UJSON_BOOL),
> > +	UJSON_OBJ_ATTR("needs_rofs", UJSON_BOOL),
> > +	UJSON_OBJ_ATTR("needs_root", UJSON_BOOL),
> > +	UJSON_OBJ_ATTR("needs_tmpdir", UJSON_BOOL),
> > +	UJSON_OBJ_ATTR("restore_wallclock", UJSON_BOOL),
> > +	UJSON_OBJ_ATTR("skip_filesystems", UJSON_ARR),
> > +	UJSON_OBJ_ATTR("skip_in_compat", UJSON_BOOL),
> > +	UJSON_OBJ_ATTR("skip_in_lockdown", UJSON_BOOL),
> > +	UJSON_OBJ_ATTR("skip_in_secureboot", UJSON_BOOL),
> > +	UJSON_OBJ_ATTR("supported_archs", UJSON_ARR),
> > +};
> > +
> > +static ujson_obj test_obj = {
> > +	.attrs = test_attrs,
> > +	.attr_cnt = UJSON_ARRAY_SIZE(test_attrs),
> > +};
> > +
> > +/* Must match the order of test_attrs. */
> 
> You could use the index syntax like [ALL_FILESYSTEMS] = UJASON_OBJ_ATTR...
> IIRC.
> 
> Then the order can't be messed up

Good idea, will do. Maybe it makes sense to embedded this into the
UJSON_OBJ_ATTR() macro as a second parameter so that we have better
syntax too.

> > +	while (fgets(line, sizeof(line), f)) {
> > +		if (in_json)
> > +			metadata_append(line + 2);
> > +
> > +		if (in_json) {
> > +			if (!strcmp(line, "# }\n"))
> > +				in_json = 0;
> > +		} else {
> > +			if (!strcmp(line, "# TEST = {\n")) {
> > +				metadata_append("{\n");
> > +				in_json = 1;
> > +			}
> > +		}
> 
> This is maybe a little bit too rigid, even if you want this exact
> formatting to be the only valid one. People will get frustrated when
> parsing fails due to whitespace and don't see a clear indication why
> it failed.
> 
> Alternativey you could adopt the standard markdown meta-data format
> where the meta-data is enclosed with three dashes like
> 
> ---
> "meta": "data"
> ---

That may be better, but we would need some markers for the asciidoc
documentation as well, so we need some kind of identifier too.

So maybe:

# ---
# doc
#
# [Description]
#
# This test does bla bla.
# ---
#
# ---
# env
#
# {
#  "needs_root": 1,
# }
# ---

Getting the delimiters right when embedding data is
hard...

> at the top of the file. I suppose you could change it to have comments
> if you want the script to run outside LTP's runner.

We actually need it to be in the comments, because we run the script by
bash first, which bootstraps the enviroment from the shell script we
source first, then reexecutes the script in the right environment.

> In any case using three dots removes any whitespace between tokens which
> is harder to trim.
> 
> The whole patchset looks good though.

Thanks a lot for the review, I will work on the small bits and send v3.

-- 
Cyril Hrubis
chrubis@suse.cz

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