Re: [LTP] [PATCH v3] fw_load: new test of device firmware loading

[<ale...@or...>]

On 06/18/2013 04:28 PM, ch...@su... wrote:
>
> Also you initialize PROCEED variable but then go ahead and use lowercase
> proceed instead. You should stick to uppercase.
OK, I'll fix it.
> And isn't the proceed value set if make target is clean replaced with
> whatever we get later in the ifdefs? Did you mean proceed ?= $(shell
> ...) there?
>
I thought it is not necessary because WITH_MODULES won't be set if make
target is clean.
I think, I will need to remove PROCEED initialization, if I set 
query-assignment.
>
>> +
>> +	if (access(module_name, F_OK) == -1) {
>> +		tst_brkm(TCONF, NULL,
>> +			"Test requires kernel module '%s'", module_name);
>> +	}
> Here you expect that the module is in current directory, which is not
> robust enough.
>
> What about creating module_exists() module_load() and module_unload()
> library functions?
Yeah, it is possible. Could I use tst_resource files for that? Or perhaps,
would it be better to create tst_modules files?
> The path to the module should be determined from LTPROOT env variable,
> see lib/tst_resource.c and lib/tst_resource.h that handles similar
> situation for files needed by the tests.
>
>
>> +
>> +static void cleanup(void)
>> +{
>> +	if (skip_cleanup)
>> +		return;
>> +	int i;
>> +	for (i = fw_num - 1; i>= 0; --i) {
> Is there any reason for the loop going backward?
>
It is the only way to successfully remove created directories.
I need to start from the last created file or/and directory and move
upward.
>> +		if (fw[i].remove_file&&  remove(fw[i].file) == -1)
>> +			tst_resm(TWARN, "Can't remove: %s", fw[i].file);
>> +
>> +		if (fw[i].remove_dir&&  remove(fw[i].dir) == -1)
>> +			tst_resm(TWARN, "Can't remove %s", fw[i].dir);
>> +	}
>> +
>> +	if (module_registered) {
>> +		char cmd[MAX_CMD_LEN];
>> +		snprintf(cmd, MAX_CMD_LEN, "rmmod %s", module_name);
>> +		if (system(cmd) != 0)
>> +			tst_brkm(TBROK, NULL, "Can't remove %s", module_name);
>> +	}
>> +
>> +	TEST_CLEANUP;
>> +}
>> +
>> +static void create_firmware(const char *path)
>> +{
>> +	int i;
>> +	for (i = 0; i<  ARRAY_SIZE(fw_paths); ++i) {
>> +		struct fw_file_info *f =&fw[fw_num];
>> +		snprintf(f->dir, PATH_MAX, "%s/%s", path, fw_paths[i]);
>> +		if (access(f->dir, X_OK) == -1) {
>> +			/* create dir */
>> +			SAFE_MKDIR(cleanup, f->dir, 0755);
>> +			f->remove_dir = 1;
>> +		}
>> +
>> +		/* create test firmware file */
>> +		snprintf(f->file, PATH_MAX, "%s/n%d_%s",
>> +			f->dir, fw_num, fw_name);
>> +
>> +		FILE *fd = fopen(f->file, "w");
> Naming FILE * variable fd is a little confusing as it suggests file
> descriptor. I would settle on simple f instead.
>> +		if (fd == NULL)
>> +			tst_brkm(TBROK, cleanup, "Failed to create firmware");
>> +		int k;
>> +		for (k = 0; k<  fw_size; ++k)
>> +			fputc(fw_num, fd);
>> +		fclose(fd);
> It's important to check the return value from fclose() here, as the
> output is buffered and the data flushed at the fclose() call. I've
> looked into the safe_macros.h and it looks like we do not have
> SAFE_FCLOSE() yet. I will add the FILE * calls there so that you can use
> here.
>
OK