[PATCH 1/2] fwts_efi_module: add fwts library helper fuctions for efi_runtime module

[IvanHu]

Hi Colin,
Thanks for your detail review and helpful comments.
I'll modify the code and resend it.


>> +int fwts_lib_efi_runtime_load_module(fwts_framework *fw)
>> +{
>> +    struct stat statbuf;
>> +    fwts_list *output;
>> +
>> +    if (check_module_loaded() != FWTS_OK) {
>> +        fwts_log_error(fw, "Could not open /proc/modules for checking
>> module loaded.");
>> +        return FWTS_ERROR;
>> +    }
>> +
>> +    if (!module_already_loaded) {
>> +        if (fwts_pipe_exec("modprobe efi_runtime", &output) !=
>> FWTS_OK) {
>> +            fwts_log_error(fw, "Load efi_runtime module error.");
>> +            return FWTS_ERROR;
>> +        } else {
>
> I see a memory leak. You need:
>              if (output)
>                  fwts_text_list_free(output);
>
>> +            check_module_loaded();
>
> since we discard the return from check_module_loaded() perhaps we should
> just give a hint using:
>
>              (void)check_module_loaded();
>
>> +            if (!module_already_loaded) {
>> +                fwts_log_error(fw, "Could not load efi_runtime
>> module.");
>> +                return FWTS_ERROR;
>> +            }
>> +        }
>> +    }
>> +
>> +    if (stat("/dev/efi_runtime", &statbuf)) {
>> +        fwts_log_error(fw, "Loaded efi_runtime module but
>> /dev/efi_runtime is not present.");
>> +        return FWTS_ERROR;
>> +    }
>
> I'm going to be pedantic here. We should also check if this is a char
> device too (it is a char device isn't it?), so
>      if (!S_ISCHR(statbuf.st_mode) {
>          fwts_log_error(fw, "Loaded efi_runtime module but
> /dev/efi_runtime is not a char device.");
>          return FWTS_ERROR;
>      }
>
>
> I'm not sure, but will there be a race between the module being loaded
> and the /dev/efi_runtime being created?  We may get some false positives
> here, perhaps we need to be careful.
>

yup. thanks for pointing out this. Actually I've tested couple other 
modules with running this code by changing the modules and dev node. The 
results seems no race problem, but I think it is worth keeping it in 
mind. :)

Cheers,
Ivan